You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@logging.apache.org by vy...@apache.org on 2024/09/04 09:37:06 UTC
(logging-log4j2) 01/02: Avoid surrogate character handling in `JsonWriter`
This is an automated email from the ASF dual-hosted git repository.
vy pushed a commit to branch fuzzing
in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit e9edabfa13e2c316de92070ed2ecb8592a738508
Author: Volkan Yazıcı <vo...@yazi.ci>
AuthorDate: Wed Sep 4 10:43:55 2024 +0200
Avoid surrogate character handling in `JsonWriter`
This change effectively reverts LOG4J2-2998 and #457.
Previously, we were omitting the last character if it is
found to be a high-surrogate. This was to ensure that the
string ending was correctly encoded. Though this doesn't
make sense in the absence of a character encoding. Consider
the following example:
Is `"\uD800"` a valid JSON document?`
If the character encoding is UTF-8, then it is a valid JSON
document. If it is UTF-16, then it is an *invalid* JSON
document; since `\uD800` is a high-surrogate character and, by
definition, is supposed to be followed by a low-surrogate
character to form a surrogate pair corresponding to a code point.
Hence, given the absence of the character encoding, we should not
attempt to fix surrogate character issues. As a matter of fact,
as of date, neither the reference implementation[1] provides
specialization for surrogates.
See `java.lang.Character` documentation[2] for details.
[1] https://github.com/stleary/JSON-java/blob/14f71274f79ac50afb316845539f4195facc33ee/src/main/java/org/json/JSONTokener.java
[2] https://docs.oracle.com/en%2Fjava%2Fjavase%2F17%2Fdocs%2Fapi%2F%2F/java.base/java/lang/Character.html
---
.../layout/template/json/util/JsonWriterTest.java | 96 ----------------------
.../layout/template/json/util/JsonWriter.java | 7 +-
.../.2.x.x/fix_JsonWriter_surrogate_handling.xml | 7 ++
3 files changed, 9 insertions(+), 101 deletions(-)
diff --git a/log4j-layout-template-json-test/src/test/java/org/apache/logging/log4j/layout/template/json/util/JsonWriterTest.java b/log4j-layout-template-json-test/src/test/java/org/apache/logging/log4j/layout/template/json/util/JsonWriterTest.java
index bdada9f49a..11607acb2d 100644
--- a/log4j-layout-template-json-test/src/test/java/org/apache/logging/log4j/layout/template/json/util/JsonWriterTest.java
+++ b/log4j-layout-template-json-test/src/test/java/org/apache/logging/log4j/layout/template/json/util/JsonWriterTest.java
@@ -49,19 +49,6 @@ 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) {
return consumer.apply(WRITER);
@@ -99,16 +86,6 @@ class JsonWriterTest {
Assertions.assertThat(writer.getStringBuilder().length()).isEqualTo(0);
}
- @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_use_null_Runnable() {
Assertions.assertThatThrownBy(() -> withLockedWriter(writer -> writer.use(null)))
@@ -436,25 +413,6 @@ 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);
- assertFormattableBufferReset(writer);
- });
- }
-
@Test
void test_writeString_null_formattable() {
expectNull(writer -> writer.writeString((StringBuilderFormattable) null));
@@ -482,25 +440,6 @@ 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);
- assertFormattableBufferReset(writer);
- });
- }
-
private static void assertFormattableBufferReset(final JsonWriter writer) {
final StringBuilder formattableBuffer = getFormattableBuffer(writer);
Assertions.assertThat(formattableBuffer.capacity()).isEqualTo(writer.getMaxStringLength());
@@ -552,23 +491,6 @@ 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 {
final char[] surrogates = new char[2];
@@ -617,24 +539,6 @@ 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[2];
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 4316d731fd..749eb1519e 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
@@ -579,9 +579,7 @@ public final class JsonWriter implements AutoCloseable, Cloneable {
* Quote text contents using JSON standard quoting.
*/
private void quoteString(final CharSequence seq, final int offset, final int length) {
- final int surrogateCorrection =
- length > 0 && Character.isHighSurrogate(seq.charAt(offset + length - 1)) ? -1 : 0;
- final int limit = offset + length + surrogateCorrection;
+ final int limit = offset + length;
int i = offset;
outer:
while (i < limit) {
@@ -643,8 +641,7 @@ public final class JsonWriter implements AutoCloseable, Cloneable {
* Quote text contents using JSON standard quoting.
*/
private void quoteString(final char[] buffer, final int offset, final int length) {
- final int surrogateCorrection = length > 0 && Character.isHighSurrogate(buffer[offset + length - 1]) ? -1 : 0;
- final int limit = offset + length + surrogateCorrection;
+ final int limit = offset + length;
int i = offset;
outer:
while (i < limit) {
diff --git a/src/changelog/.2.x.x/fix_JsonWriter_surrogate_handling.xml b/src/changelog/.2.x.x/fix_JsonWriter_surrogate_handling.xml
new file mode 100644
index 0000000000..d3610aaec4
--- /dev/null
+++ b/src/changelog/.2.x.x/fix_JsonWriter_surrogate_handling.xml
@@ -0,0 +1,7 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xmlns="https://logging.apache.org/xml/ns"
+ xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+ type="fixed">
+ <description format="asciidoc">Avoid surrogate character handling in `JsonWriter`</description>
+</entry>