This is an automated email from the ASF dual-hosted git repository. vy pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit f96cf24aa5cb1e1f1ca13fe308f8a8f8ed8c5d81 Author: Volkan Yazici <[email protected]> AuthorDate: Thu Jan 14 17:28:45 2021 +0100 LOG4J2-2998 Fix truncation of excessive strings ending with a high surrogate in JsonWriter. (#457) --- .../layout/template/json/util/JsonWriter.java | 12 +- .../layout/template/json/util/JsonWriterTest.java | 142 +++++++++++++++++++-- src/changes/changes.xml | 7 +- 3 files changed, 144 insertions(+), 17 deletions(-) diff --git a/log4j-layout-template-json/src/main/java/org/apache/logging/log4j/layout/template/json/util/JsonWriter.java b/log4j-layout-template-json/src/main/java/org/apache/logging/log4j/layout/template/json/util/JsonWriter.java index ac35468..7c928f1 100644 --- a/log4j-layout-template-json/src/main/java/org/apache/logging/log4j/layout/template/json/util/JsonWriter.java +++ b/log4j-layout-template-json/src/main/java/org/apache/logging/log4j/layout/template/json/util/JsonWriter.java @@ -581,7 +581,11 @@ public final class JsonWriter implements AutoCloseable, Cloneable { final CharSequence seq, final int offset, final int length) { - final int limit = offset + length; + final int surrogateCorrection = + length > 0 && Character.isHighSurrogate(seq.charAt(offset + length - 1)) + ? -1 + : 0; + final int limit = offset + length + surrogateCorrection; int i = offset; outer: while (i < limit) { @@ -654,7 +658,11 @@ public final class JsonWriter implements AutoCloseable, Cloneable { final char[] buffer, final int offset, final int length) { - final int limit = offset + length; + final int surrogateCorrection = + length > 0 && Character.isHighSurrogate(buffer[offset + length - 1]) + ? -1 + : 0; + final int limit = offset + length + surrogateCorrection; int i = offset; outer: while (i < limit) { diff --git a/log4j-layout-template-json/src/test/java/org/apache/logging/log4j/layout/template/json/util/JsonWriterTest.java b/log4j-layout-template-json/src/test/java/org/apache/logging/log4j/layout/template/json/util/JsonWriterTest.java index b18368e..c7c9c64 100644 --- a/log4j-layout-template-json/src/test/java/org/apache/logging/log4j/layout/template/json/util/JsonWriterTest.java +++ b/log4j-layout-template-json/src/test/java/org/apache/logging/log4j/layout/template/json/util/JsonWriterTest.java @@ -49,6 +49,18 @@ class JsonWriterTest { .setTruncatedStringSuffix("~") .build(); + private static final int SURROGATE_CODE_POINT = 65536; + + private static final char[] SURROGATE_PAIR = new char[2]; + static { + // noinspection ResultOfMethodCallIgnored + Character.toChars(SURROGATE_CODE_POINT, SURROGATE_PAIR, 0); + } + + private static final char HI_SURROGATE = SURROGATE_PAIR[0]; + + private static final char LO_SURROGATE = SURROGATE_PAIR[1]; + private static synchronized <V> V withLockedWriterReturning( final Function<JsonWriter, V> consumer) { synchronized (WRITER) { @@ -64,6 +76,23 @@ class JsonWriterTest { } @Test + void test_surrogate_code_point() { + Assertions + .assertThat(HI_SURROGATE) + .matches(Character::isHighSurrogate, "is high surrogate"); + Assertions + .assertThat(LO_SURROGATE) + .matches(Character::isLowSurrogate, "is low surrogate"); + Assertions + .assertThat(Character.isSurrogatePair(HI_SURROGATE, LO_SURROGATE)) + .as("is surrogate pair") + .isTrue(); + Assertions + .assertThat(SURROGATE_CODE_POINT) + .matches(Character::isDefined, "is defined"); + } + + @Test void test_writeValue_null_Object() { expectNull(writer -> writer.writeValue(null)); } @@ -402,6 +431,27 @@ class JsonWriterTest { } @Test + void test_writeString_emitter_excessive_string_ending_with_high_surrogate() { + withLockedWriter(writer -> { + final int maxStringLength = writer.getMaxStringLength(); + @SuppressWarnings("StringBufferReplaceableByString") + final String excessiveString = new StringBuilder() + .append(Strings.repeat("x", maxStringLength - 1)) + .append(HI_SURROGATE) + .append(LO_SURROGATE) + .toString(); + final String expectedJson = "\"" + + Strings.repeat("x", maxStringLength - 1) + + writer.getTruncatedStringSuffix() + + '"'; + final BiConsumer<StringBuilder, String> emitter = StringBuilder::append; + final String actualJson = + writer.use(() -> writer.writeString(emitter, excessiveString)); + Assertions.assertThat(actualJson).isEqualTo(expectedJson); + }); + } + + @Test void test_writeString_null_formattable() { expectNull(writer -> writer.writeString((StringBuilderFormattable) null)); } @@ -433,6 +483,27 @@ class JsonWriterTest { } @Test + void test_writeString_formattable_excessive_string_ending_with_high_surrogate() { + withLockedWriter(writer -> { + final int maxStringLength = writer.getMaxStringLength(); + @SuppressWarnings("StringBufferReplaceableByString") + final String excessiveString = new StringBuilder() + .append(Strings.repeat("x", maxStringLength - 1)) + .append(HI_SURROGATE) + .append(LO_SURROGATE) + .toString(); + final String expectedJson = "\"" + + Strings.repeat("x", maxStringLength - 1) + + writer.getTruncatedStringSuffix() + + '"'; + final String actualJson = writer.use(() -> + writer.writeString(stringBuilder -> + stringBuilder.append(excessiveString))); + Assertions.assertThat(actualJson).isEqualTo(expectedJson); + }); + } + + @Test void test_writeString_null_seq_1() { expectNull(writer -> writer.writeString((CharSequence) null)); } @@ -474,9 +545,31 @@ class JsonWriterTest { } @Test + void test_writeString_excessive_seq_ending_with_high_surrogate() { + withLockedWriter(writer -> { + final int maxStringLength = writer.getMaxStringLength(); + @SuppressWarnings("StringBufferReplaceableByString") + final CharSequence seq = new StringBuilder() + .append(Strings.repeat("x", maxStringLength - 1)) + .append(HI_SURROGATE) + .append(LO_SURROGATE) + .toString(); + final String expectedJson = "\"" + + Strings.repeat("x", maxStringLength - 1) + + writer.getTruncatedStringSuffix() + + '"'; + final String actualJson = writer.use(() -> writer.writeString(seq)); + Assertions.assertThat(actualJson).isEqualTo(expectedJson); + }); + } + + @Test void test_writeString_seq() throws IOException { - testQuoting((final Character c) -> { - final String s = "" + c; + final char[] surrogates = new char[2]; + testQuoting((final Integer codePoint) -> { + // noinspection ResultOfMethodCallIgnored + Character.toChars(codePoint, surrogates, 0); + final String s = new String(surrogates); return withLockedWriterReturning(writer -> writer.use(() -> writer.writeString(s))); }); @@ -526,31 +619,54 @@ class JsonWriterTest { } @Test + void test_writerString_excessive_buffer_ending_with_high_surrogate() { + withLockedWriter(writer -> { + final int maxStringLength = writer.getMaxStringLength(); + @SuppressWarnings("StringBufferReplaceableByString") + final char[] buffer = new StringBuilder() + .append(Strings.repeat("x", maxStringLength - 1)) + .append(HI_SURROGATE) + .append(LO_SURROGATE) + .toString() + .toCharArray(); + final String expectedJson = "\"" + + Strings.repeat("x", maxStringLength - 1) + + writer.getTruncatedStringSuffix() + + '"'; + final String actualJson = writer.use(() -> writer.writeString(buffer)); + Assertions.assertThat(actualJson).isEqualTo(expectedJson); + }); + } + + @Test void test_writeString_buffer() throws IOException { - final char[] buffer = new char[1]; - testQuoting((final Character c) -> { - buffer[0] = c; + final char[] buffer = new char[2]; + testQuoting((final Integer codePoint) -> { + // noinspection ResultOfMethodCallIgnored + Character.toChars(codePoint, buffer, 0); return withLockedWriterReturning(writer -> writer.use(() -> writer.writeString(buffer))); }); } private static void testQuoting( - final Function<Character, String> quoter) throws IOException { + final Function<Integer, String> quoter) throws IOException { final SoftAssertions assertions = new SoftAssertions(); - for (char c = Character.MIN_VALUE;; c++) { - final String s = "" + c; + final char[] surrogates = new char[2]; + for (int codePoint = Character.MIN_CODE_POINT; + codePoint <= Character.MAX_CODE_POINT; + codePoint++) { + // noinspection ResultOfMethodCallIgnored + Character.toChars(codePoint, surrogates, 0); + final String s = new String(surrogates); final String expectedJson = JacksonFixture .getObjectMapper() .writeValueAsString(s); - final String actualJson = quoter.apply(c); + final String actualJson = quoter.apply(codePoint); assertions .assertThat(actualJson) - .as("c='%c' (%d)", c, (int) c) + .as("codePoint='%s' (%d)", s, codePoint) .isEqualTo(expectedJson); - if (c == Character.MAX_VALUE) { - break; - } } assertions.assertAll(); } diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 229ee48..b78a7c3 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -31,6 +31,9 @@ - "remove" - Removed --> <release version="3.0.0" date="2021-MM-DD" description="GA Release 3.0.0"> + <action issue="LOG4J2-2998" dev="vy" type="fix"> + Fix truncation of excessive strings ending with a high surrogate in JsonWriter. + </action> <action issue="LOG4J2-2967" dev="ckozak" type="fix"> Fix JsonTemplateLayout index based parameter resolution when messages contain too few parameters. </action> @@ -61,9 +64,9 @@ <action issue="LOG4J2-2678" dev="rgoers" type="update" due-to="Federico D'Ambrosio"> Add LogEvent timestamp to ProducerRecord in KafkaAppender. </action> - <actino issue="LOG4J2-2688" dev="rgoers" type="add" due-to="Romain Manni-Bucau"> + <action issue="LOG4J2-2688" dev="rgoers" type="add" due-to="Romain Manni-Bucau"> Allow web lookup of session attributes. - </actino> + </action> <action issue="LOG4J2-2701" dev="rgoers" type="update"> Update Jackson from 2.9.x to 2.10.1. </action>
