Merge changes in version 2.25.4 back to 2.x (#4085)

Author: ppkarwasz

.github/workflows/build.yaml

@@ -21,6 +21,7 @@ on:
   push:
     branches:
       - "2.x"
+      - "2.25.x"
       - "release/2*"
   pull_request:
 

log4j-1.2-api/src/test/java/org/apache/log4j/layout/Log4j1XmlLayoutTest.java

@@ -72,7 +72,8 @@ void testWithPropertiesAndLocationInfo() {
         final String expected = "<log4j:event logger=\"a.B\" timestamp=\"" + event.getTimeMillis()
                 + "\" level=\"INFO\" thread=\"main\">\r\n"
                 + "<log4j:message><![CDATA[Hello, World]]></log4j:message>\r\n"
-                + "<log4j:locationInfo class=\"pack.MyClass\" method=\"myMethod\" file=\"MyClass.java\" line=\"17\"/>\r\n"
+                + "<log4j:locationInfo class=\"pack.MyClass\" method=\"myMethod\" file=\"MyClass.java\""
+                + " line=\"17\"/>\r\n"
                 + "<log4j:properties>\r\n"
                 + "<log4j:data name=\"key1\" value=\"value1\"/>\r\n"
                 + "<log4j:data name=\"key2\" value=\"value2\"/>\r\n"
@@ -81,4 +82,43 @@ void testWithPropertiesAndLocationInfo() {
 
         assertEquals(expected, result);
     }
+
+    @Test
+    void testWithInvalidXmlCharacters() {
+        final Log4j1XmlLayout layout = Log4j1XmlLayout.createLayout(true, true);
+
+        final String message = "<>'\"&A\uD800B\uDE00C\u0000\u0001\u0002\u0003\uFFFE\uFFFF";
+        final String expectedMessage = "<>'\"&A\uFFFDB\uFFFDC\uFFFD\uFFFD\uFFFD\uFFFD\uFFFD\uFFFD";
+        final String expectedEscapedMessage =
+                "&lt;&gt;&#39;&quot;&amp;A\uFFFDB\uFFFDC\uFFFD\uFFFD\uFFFD\uFFFD\uFFFD\uFFFD";
+
+        final StringMap contextMap = ContextDataFactory.createContextData(1);
+        contextMap.putValue(message, message);
+        final Log4jLogEvent event = Log4jLogEvent.newBuilder()
+                .setLoggerName(message)
+                .setLevel(Level.forName(message, 100))
+                .setMessage(new SimpleMessage(message))
+                .setTimeMillis(System.currentTimeMillis() + 17)
+                .setIncludeLocation(true)
+                .setSource(new StackTraceElement(message, message, message, 17))
+                .setContextData(contextMap)
+                .build();
+
+        final String result = layout.toSerializable(event);
+
+        final String expected =
+                "<log4j:event logger=\"" + expectedEscapedMessage + "\" timestamp=\"" + event.getTimeMillis()
+                        + "\" level=\"" + expectedEscapedMessage + "\" thread=\"main\">\r\n"
+                        + "<log4j:message><![CDATA[" + expectedMessage + "]]></log4j:message>\r\n"
+                        + "<log4j:locationInfo class=\"" + expectedEscapedMessage
+                        + "\" method=\"" + expectedEscapedMessage
+                        + "\" file=\"" + expectedEscapedMessage + "\" line=\"17\"/>\r\n"
+                        + "<log4j:properties>\r\n"
+                        + "<log4j:data name=\"" + expectedEscapedMessage + "\" value=\"" + expectedEscapedMessage
+                        + "\"/>\r\n"
+                        + "</log4j:properties>\r\n"
+                        + "</log4j:event>\r\n\r\n";
+
+        assertEquals(expected, result);
+    }
 }

log4j-api-test/src/test/java/org/apache/logging/log4j/message/MapMessageTest.java

@@ -16,6 +16,7 @@
  */
 package org.apache.logging.log4j.message;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
@@ -72,12 +73,26 @@ void testXML() {
 
     @Test
     void testXMLEscape() {
-        final String testMsg = "Test message <foo>";
+        final String notBmp = new String(Character.toChars(0x10000));
+        final String invalid = "A\uD800B\uDE00C\0\1\2\3";
+        final String expectedInvalid = "A\uFFFDB\uFFFDC\uFFFD\uFFFD\uFFFD\uFFFD";
+        final String key = "k<e&y> '\"\t\r\n" + notBmp + invalid;
+        final String value = "v>al<u& '\"\t\r\n" + notBmp + invalid;
         final StringMapMessage msg = new StringMapMessage();
-        msg.put("message", testMsg);
+        msg.put(key, value);
         final String result = msg.getFormattedMessage(new String[] {"XML"});
-        final String expected = "<Map>\n  <Entry key=\"message\">Test message &lt;foo&gt;</Entry>\n" + "</Map>";
-        assertEquals(expected, result);
+
+        assertThat(result)
+                .isEqualTo(
+                        "<Map>\n" //
+                                + "  <Entry key=\"k&lt;e&amp;y&gt; &apos;&quot;\t\r\n"
+                                + notBmp
+                                + expectedInvalid
+                                + "\">v&gt;al&lt;u&amp; &apos;&quot;\t\r\n"
+                                + notBmp
+                                + expectedInvalid
+                                + "</Entry>\n" //
+                                + "</Map>");
     }
 
     @Test

log4j-api-test/src/test/java/org/apache/logging/log4j/util/StringBuildersTest.java

@@ -16,10 +16,15 @@
  */
 package org.apache.logging.log4j.util;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.util.stream.Stream;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 
 /**
  * Tests the StringBuilders class.
@@ -79,15 +84,37 @@ void escapeJsonCharactersISOControl() {
         assertEquals(jsonValueEscaped, sb.toString());
     }
 
-    @Test
-    void escapeXMLCharactersCorrectly() {
-        final String xmlValueNotEscaped = "<\"Salt&Peppa'\">";
-        final String xmlValueEscaped = "&lt;&quot;Salt&amp;Peppa&apos;&quot;&gt;";
+    static Stream<Arguments> escapeXmlCharactersCorrectly() {
+        final char replacement = '\uFFFD';
+        return Stream.of(
+                // Empty
+                Arguments.of("", ""),
+                // characters that need to be escaped
+                Arguments.of("<\"Salt&Peppa'\">", "&lt;&quot;Salt&amp;Peppa&apos;&quot;&gt;"),
+                // control character replaced with U+FFFD
+                Arguments.of("A" + (char) 0x01 + "B", "A" + replacement + "B"),
+                // standalone low surrogate replaced with U+FFFD
+                Arguments.of("low" + Character.MIN_LOW_SURROGATE + "surrogate", "low" + replacement + "surrogate"),
+                Arguments.of(Character.MIN_LOW_SURROGATE + "low", replacement + "low"),
+                // standalone high surrogate replaced with U+FFFD
+                Arguments.of("high" + Character.MIN_HIGH_SURROGATE + "surrogate", "high" + replacement + "surrogate"),
+                Arguments.of(Character.MIN_HIGH_SURROGATE + "high", replacement + "high"),
+                // FFFE and FFFF
+                Arguments.of("invalid\uFFFEchars", "invalid" + replacement + "chars"),
+                Arguments.of("invalid\uFFFFchars", "invalid" + replacement + "chars"),
+                // whitespace characters are preserved
+                Arguments.of("tab\tnewline\ncr\r", "tab\tnewline\ncr\r"),
+                // character beyond BMP (emoji) preserved as surrogate pair
+                Arguments.of("emoji " + "\uD83D\uDE00" + " end", "emoji " + "\uD83D\uDE00" + " end"));
+    }
 
+    @ParameterizedTest
+    @MethodSource
+    void escapeXmlCharactersCorrectly(final String input, final String expected) {
         final StringBuilder sb = new StringBuilder();
-        sb.append(xmlValueNotEscaped);
-        assertEquals(xmlValueNotEscaped, sb.toString());
+        sb.append(input);
+        assertThat(sb.toString()).isEqualTo(input);
         StringBuilders.escapeXml(sb, 0);
-        assertEquals(xmlValueEscaped, sb.toString());
+        assertThat(sb.toString()).isEqualTo(expected);
     }
 }

log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java

@@ -358,10 +358,14 @@ private StringBuilder format(final MapFormat format, final StringBuilder sb) {
     public void asXml(final StringBuilder sb) {
         sb.append("<Map>\n");
         for (int i = 0; i < data.size(); i++) {
-            sb.append("  <Entry key=\"").append(data.getKeyAt(i)).append("\">");
-            final int size = sb.length();
+            sb.append("  <Entry key=\"");
+            int start = sb.length();
+            sb.append(data.getKeyAt(i));
+            StringBuilders.escapeXml(sb, start);
+            sb.append("\">");
+            start = sb.length();
             ParameterFormatter.recursiveDeepToString(data.getValueAt(i), sb);
-            StringBuilders.escapeXml(sb, size);
+            StringBuilders.escapeXml(sb, start);
             sb.append("</Entry>\n");
         }
         sb.append("</Map>");

log4j-api/src/main/java/org/apache/logging/log4j/util/StringBuilders.java

@@ -27,6 +27,8 @@
 @InternalApi
 public final class StringBuilders {
 
+    private static final char REPLACEMENT_CHAR = '\uFFFD';
+
     private static final Class<?> timeClass;
     private static final Class<?> dateClass;
 
@@ -310,8 +312,8 @@ private static int escapeAndDecrement(final StringBuilder toAppendTo, int lastPo
      */
     public static void escapeXml(final StringBuilder toAppendTo, final int start) {
         int escapeCount = 0;
-        for (int i = start; i < toAppendTo.length(); i++) {
-            final char c = toAppendTo.charAt(i);
+        for (int i = start; i < toAppendTo.length(); ) {
+            final int c = toAppendTo.codePointAt(i);
             switch (c) {
                 case '&':
                     escapeCount += 4;
@@ -323,15 +325,36 @@ public static void escapeXml(final StringBuilder toAppendTo, final int start) {
                 case '"':
                 case '\'':
                     escapeCount += 5;
+                    break;
+                default:
+                    // All invalid XML 1.0 characters have the same length as the replacement character
+                    // Therefore no additional adjustment is needed
             }
+            i += Character.charCount(c);
         }
 
         final int lastChar = toAppendTo.length() - 1;
+        if (lastChar < 0) {
+            return;
+        }
         toAppendTo.setLength(toAppendTo.length() + escapeCount);
         int lastPos = toAppendTo.length() - 1;
 
-        for (int i = lastChar; lastPos > i; i--) {
+        for (int i = lastChar; lastPos >= start; i--) {
             final char c = toAppendTo.charAt(i);
+            // Handle surrogate pairs and invalid low surrogates
+            if (i > 0 && Character.isLowSurrogate(c)) {
+                final char previous = toAppendTo.charAt(i - 1);
+                // Invalid low surrogate
+                if (!Character.isHighSurrogate(previous)) {
+                    toAppendTo.setCharAt(lastPos--, REPLACEMENT_CHAR);
+                } else {
+                    toAppendTo.setCharAt(lastPos--, c);
+                    toAppendTo.setCharAt(lastPos--, previous);
+                    i--;
+                }
+                continue;
+            }
             switch (c) {
                 case '&':
                     toAppendTo.setCharAt(lastPos--, ';');
@@ -369,8 +392,32 @@ public static void escapeXml(final StringBuilder toAppendTo, final int start) {
                     toAppendTo.setCharAt(lastPos--, '&');
                     break;
                 default:
-                    toAppendTo.setCharAt(lastPos--, c);
+                    toAppendTo.setCharAt(lastPos--, isValidXml10(c) ? c : REPLACEMENT_CHAR);
             }
         }
     }
+
+    /**
+     * Checks if a BMP {@code char} is a valid XML 1.0 character.
+     *
+     * <p>This method is restricted to characters in the BMP, i.e. represented by one UTF-16 code unit.</p>
+     *
+     * @param ch a BMP {@code char} to validate
+     * @return {@code true} if it is a valid XML 1.0 character
+     */
+    private static boolean isValidXml10(final char ch) {
+        // XML 1.0 valid characters (Fifth Edition):
+        //   #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | [#x10000-#x10FFFF]
+
+        // [#x20–#xD7FF] (placed early as a fast path for the most common case)
+        return (ch >= ' ' && ch < Character.MIN_SURROGATE)
+                // #x9
+                || ch == '\t'
+                // #xA
+                || ch == '\n'
+                // #xD
+                || ch == '\r'
+                // [#xE000-#xFFFD]
+                || (ch > Character.MAX_SURROGATE && ch <= 0xFFFD);
+    }
 }

log4j-core-test/pom.xml

@@ -64,8 +64,19 @@
     <!-- Additional version of LMAX Disruptor to test -->
     <disruptor4.version>4.0.0</disruptor4.version>
     <json-unit.version>2.40.1</json-unit.version>
+    <bouncycastle.version>1.83</bouncycastle.version>
   </properties>
 
+  <dependencyManagement>
+    <dependencies>
+      <dependency>
+        <groupId>org.bouncycastle</groupId>
+        <artifactId>bcpkix-jdk18on</artifactId>
+        <version>${bouncycastle.version}</version>
+      </dependency>
+    </dependencies>
+  </dependencyManagement>
+
   <dependencies>
 
     <dependency>
@@ -149,6 +160,12 @@
       <scope>test</scope>
     </dependency>
 
+    <dependency>
+      <groupId>org.bouncycastle</groupId>
+      <artifactId>bcpkix-jdk18on</artifactId>
+      <scope>test</scope>
+    </dependency>
+
     <!-- Other -->
     <dependency>
       <groupId>commons-codec</groupId>

log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/package-info.java

@@ -15,7 +15,7 @@
  * limitations under the license.
  */
 @Export
-@Version("2.26.0")
+@Version("2.25.3")
 @BaselineIgnore("2.25.0")
 package org.apache.logging.log4j.core.test;
 

log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/LineReadingTcpServer.java

@@ -58,6 +58,8 @@ final class LineReadingTcpServer implements AutoCloseable {
 
     private volatile boolean running;
 
+    private InetAddress bindAddress = InetAddress.getLoopbackAddress();
+
     private ServerSocket serverSocket;
 
     private Socket clientSocket;
@@ -74,6 +76,11 @@ final class LineReadingTcpServer implements AutoCloseable {
         this.serverSocketFactory = serverSocketFactory;
     }
 
+    // For testing purposes
+    void setBindAddress(final InetAddress bindAddress) {
+        this.bindAddress = bindAddress;
+    }
+
     synchronized void start(final String name, final int port) throws IOException {
         if (!running) {
             running = true;
@@ -83,8 +90,7 @@ synchronized void start(final String name, final int port) throws IOException {
     }
 
     private ServerSocket createServerSocket(final int port) throws IOException {
-        final ServerSocket serverSocket =
-                serverSocketFactory.createServerSocket(port, 1, InetAddress.getLoopbackAddress());
+        final ServerSocket serverSocket = serverSocketFactory.createServerSocket(port, 1, bindAddress);
         serverSocket.setReuseAddress(true);
         serverSocket.setSoTimeout(0); // Zero indicates `accept()` will block indefinitely
         await("server socket binding")
@@ -104,12 +110,12 @@ private Thread createReaderThread(final String name) {
     }
 
     private void acceptClients() {
-        try {
-            while (running) {
+        while (running) {
+            try {
                 acceptClient();
+            } catch (final Exception error) {
+                LOGGER.error("failed accepting client connections", error);
             }
-        } catch (final Exception error) {
-            LOGGER.error("failed accepting client connections", error);
         }
     }