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 2021/06/04 09:02:53 UTC
[logging-log4j2] branch master updated: LOG4J2-3092 Fix JsonWriter
memory leaks due to retained excessive buffer growth.
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
The following commit(s) were added to refs/heads/master by this push:
new 49e0222 LOG4J2-3092 Fix JsonWriter memory leaks due to retained excessive buffer growth.
49e0222 is described below
commit 49e022211d6d09c5459f9c0d8e8b34a18c553ef4
Author: Volkan Yazici <vo...@gmail.com>
AuthorDate: Thu May 27 17:02:07 2021 +0200
LOG4J2-3092 Fix JsonWriter memory leaks due to retained excessive buffer growth.
---
.../layout/template/json/util/JsonWriter.java | 85 +++++++++++++---------
.../layout/template/json/util/JsonWriterTest.java | 77 ++++++++++++++++++++
src/changes/changes.xml | 3 +
3 files changed, 132 insertions(+), 33 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 7c928f1..24c8f57 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
@@ -42,7 +42,7 @@ import java.util.Objects;
* {@link Long})
* <li>{@link Boolean}
* <li>{@link StringBuilderFormattable}
- * <li>arrays of primitve types
+ * <li>arrays of primitive types
* <tt>char/boolean/byte/short/int/long/float/double</tt> and {@link Object}
* <li>{@link CharSequence} and <tt>char[]</tt> with necessary escaping
* </ul>
@@ -61,7 +61,7 @@ public final class JsonWriter implements AutoCloseable, Cloneable {
* Lookup table used for determining which output characters in 7-bit ASCII
* range (i.e., first 128 Unicode code points, single-byte UTF-8 characters)
* need to be quoted.
- *<p>
+ * <p>
* Value of 0 means "no escaping"; other positive values, that value is
* character to use after backslash; and negative values, that generic
* (backslash - u) escaping is to be used.
@@ -100,8 +100,8 @@ public final class JsonWriter implements AutoCloseable, Cloneable {
private JsonWriter(final Builder builder) {
this.quoteBuffer = new char[]{'\\', '-', '0', '0', '-', '-'};
- this.stringBuilder = new StringBuilder();
- this.formattableBuffer = new StringBuilder();
+ this.stringBuilder = new StringBuilder(builder.maxStringLength);
+ this.formattableBuffer = new StringBuilder(builder.maxStringLength);
this.maxStringLength = builder.maxStringLength;
this.truncatedStringSuffix = builder.truncatedStringSuffix;
this.quotedTruncatedStringSuffix = quoteString(builder.truncatedStringSuffix);
@@ -117,13 +117,17 @@ public final class JsonWriter implements AutoCloseable, Cloneable {
return quotedString;
}
- public String use(Runnable runnable) {
+ public String use(final Runnable runnable) {
+ Objects.requireNonNull(runnable, "runnable");
final int startIndex = stringBuilder.length();
- runnable.run();
- final StringBuilder sliceStringBuilder = new StringBuilder();
- sliceStringBuilder.append(stringBuilder, startIndex, stringBuilder.length());
- stringBuilder.setLength(startIndex);
- return sliceStringBuilder.toString();
+ try {
+ runnable.run();
+ final StringBuilder sliceStringBuilder = new StringBuilder();
+ sliceStringBuilder.append(stringBuilder, startIndex, stringBuilder.length());
+ return sliceStringBuilder.toString();
+ } finally {
+ trimStringBuilder(stringBuilder, startIndex);
+ }
}
public StringBuilder getStringBuilder() {
@@ -495,28 +499,8 @@ public final class JsonWriter implements AutoCloseable, Cloneable {
final S state) {
Objects.requireNonNull(emitter, "emitter");
stringBuilder.append('"');
- formattableBuffer.setLength(0);
- emitter.accept(formattableBuffer, state);
- final int length = formattableBuffer.length();
- // Handle max. string length complying input.
- if (length <= maxStringLength) {
- quoteString(formattableBuffer, 0, length);
- }
- // Handle max. string length violating input.
- else {
- quoteString(formattableBuffer, 0, maxStringLength);
- stringBuilder.append(quotedTruncatedStringSuffix);
- }
- stringBuilder.append('"');
- }
-
- public void writeString(final StringBuilderFormattable formattable) {
- if (formattable == null) {
- writeNull();
- } else {
- stringBuilder.append('"');
- formattableBuffer.setLength(0);
- formattable.formatTo(formattableBuffer);
+ try {
+ emitter.accept(formattableBuffer, state);
final int length = formattableBuffer.length();
// Handle max. string length complying input.
if (length <= maxStringLength) {
@@ -528,6 +512,32 @@ public final class JsonWriter implements AutoCloseable, Cloneable {
stringBuilder.append(quotedTruncatedStringSuffix);
}
stringBuilder.append('"');
+ } finally {
+ trimStringBuilder(formattableBuffer, 0);
+ }
+ }
+
+ public void writeString(final StringBuilderFormattable formattable) {
+ if (formattable == null) {
+ writeNull();
+ } else {
+ stringBuilder.append('"');
+ try {
+ formattable.formatTo(formattableBuffer);
+ final int length = formattableBuffer.length();
+ // Handle max. string length complying input.
+ if (length <= maxStringLength) {
+ quoteString(formattableBuffer, 0, length);
+ }
+ // Handle max. string length violating input.
+ else {
+ quoteString(formattableBuffer, 0, maxStringLength);
+ stringBuilder.append(quotedTruncatedStringSuffix);
+ }
+ stringBuilder.append('"');
+ } finally {
+ trimStringBuilder(formattableBuffer, 0);
+ }
}
}
@@ -839,7 +849,16 @@ public final class JsonWriter implements AutoCloseable, Cloneable {
@Override
public void close() {
- stringBuilder.setLength(0);
+ trimStringBuilder(stringBuilder, 0);
+ }
+
+ private void trimStringBuilder(final StringBuilder stringBuilder, final int length) {
+ final int trimLength = Math.max(maxStringLength, length);
+ if (stringBuilder.length() > trimLength) {
+ stringBuilder.setLength(trimLength);
+ stringBuilder.trimToSize();
+ }
+ stringBuilder.setLength(length);
}
@Override
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 66e118e..c519f75 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
@@ -29,6 +29,7 @@ import org.assertj.core.api.SoftAssertions;
import org.junit.jupiter.api.Test;
import java.io.IOException;
+import java.lang.reflect.Field;
import java.math.BigDecimal;
import java.math.BigInteger;
import java.util.Arrays;
@@ -77,6 +78,35 @@ class JsonWriterTest {
}
@Test
+ void test_close() {
+ withLockedWriter(writer -> {
+ writer.writeString("x");
+ writer.close();
+ assertStringBuilderReset(writer);
+ });
+ }
+
+ @Test
+ void test_close_after_excessive_write() {
+ withLockedWriter(writer -> {
+ final String text = Strings.repeat("x", writer.getMaxStringLength());
+ writer.writeString(text);
+ writer.writeString(text);
+ writer.close();
+ assertStringBuilderReset(writer);
+ });
+ }
+
+ private static void assertStringBuilderReset(final JsonWriter writer) {
+ Assertions
+ .assertThat(writer.getStringBuilder().capacity())
+ .isEqualTo(writer.getMaxStringLength());
+ Assertions
+ .assertThat(writer.getStringBuilder().length())
+ .isEqualTo(0);
+ }
+
+ @Test
void test_surrogate_code_point() {
Assertions
.assertThat(HI_SURROGATE)
@@ -94,6 +124,29 @@ class JsonWriterTest {
}
@Test
+ void test_use_null_Runnable() {
+ Assertions
+ .assertThatThrownBy(() -> withLockedWriter(writer -> writer.use(null)))
+ .isInstanceOf(NullPointerException.class)
+ .hasMessageContaining("runnable");
+ }
+
+ @Test
+ void test_use_failing_Runnable() {
+ final RuntimeException exception = new RuntimeException();
+ withLockedWriter(writer -> {
+ final int initialLength = writer.getStringBuilder().length();
+ Assertions
+ .assertThatThrownBy(() -> writer.use(() -> {
+ writer.writeString("extending the buffer");
+ throw exception;
+ }))
+ .isSameAs(exception);
+ Assertions.assertThat(writer.getStringBuilder()).hasSize(initialLength);
+ });
+ }
+
+ @Test
void test_writeValue_null_Object() {
expectNull(writer -> writer.writeValue(null));
}
@@ -428,6 +481,7 @@ class JsonWriterTest {
final String actualJson =
writer.use(() -> writer.writeString(emitter, excessiveString));
Assertions.assertThat(actualJson).isEqualTo(expectedJson);
+ assertFormattableBufferReset(writer);
});
}
@@ -449,6 +503,7 @@ class JsonWriterTest {
final String actualJson =
writer.use(() -> writer.writeString(emitter, excessiveString));
Assertions.assertThat(actualJson).isEqualTo(expectedJson);
+ assertFormattableBufferReset(writer);
});
}
@@ -480,6 +535,7 @@ class JsonWriterTest {
writer.writeString(stringBuilder ->
stringBuilder.append(excessiveString)));
Assertions.assertThat(actualJson).isEqualTo(expectedJson);
+ assertFormattableBufferReset(writer);
});
}
@@ -501,9 +557,30 @@ class JsonWriterTest {
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());
+ Assertions
+ .assertThat(formattableBuffer.length())
+ .isEqualTo(0);
+ }
+
+ private static StringBuilder getFormattableBuffer(final JsonWriter writer) {
+ try {
+ final Field field = JsonWriter.class.getDeclaredField("formattableBuffer");
+ field.setAccessible(true);
+ return (StringBuilder) field.get(writer);
+ } catch (Exception error) {
+ throw new RuntimeException(error);
+ }
+ }
+
@Test
void test_writeString_null_seq_1() {
expectNull(writer -> writer.writeString((CharSequence) null));
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index d5bf690..8e9fad9 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -208,6 +208,9 @@
Allow a PatternSelector to be specified on GelfLayout.
</action>
<!-- FIXES -->
+ <action issue="LOG4J2-3092" dev="vy" type="fix" due-to="xmh51">
+ Fix JsonWriter memory leaks due to retained excessive buffer growth.
+ </action>
<action issue="LOG4J2-3089" dev="vy" type="fix" due-to="Tim Perry">
Fix sporadic JsonTemplateLayoutNullEventDelimiterTest failures on Windows.
</action>