This is an automated email from the ASF dual-hosted git repository. vy pushed a commit to branch 2.x-fix-api-recursive-formatting in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit a258a589234d3fe11da7f0f7656462ad714ed1ff Author: Volkan Yazıcı <[email protected]> AuthorDate: Fri Feb 9 10:48:06 2024 +0100 Fix `StringBuilder` cache corruption on recursive access --- .../logging/log4j/internal/ArrayQueueTest.java | 101 +++++++++++++++++++++ ...eterizedMessageRecursiveFormattingTestBase.java | 53 +++++++++++ ...ageRecursiveFormattingWithThreadLocalsTest.java | 31 +++++++ ...RecursiveFormattingWithoutThreadLocalsTest.java | 31 +++++++ .../apache/logging/log4j/internal/ArrayQueue.java | 101 +++++++++++++++++++++ .../log4j/internal/DummyStringBuilderRecycler.java | 35 +++++++ .../QueueingThreadLocalStringBuilderRecycler.java | 55 +++++++++++ .../log4j/internal/StringBuilderRecycler.java | 61 +++++++++++++ .../internal/ThreadLocalStringBuilderRecycler.java | 54 +++++++++++ .../log4j/message/ParameterizedMessage.java | 50 +++++----- .../org/apache/logging/log4j/util/Strings.java | 11 ++- .../.2.x.x/fix_api_recursive_formatting.xml | 9 ++ 12 files changed, 562 insertions(+), 30 deletions(-) diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/internal/ArrayQueueTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/internal/ArrayQueueTest.java new file mode 100644 index 0000000000..e063e7f81c --- /dev/null +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/internal/ArrayQueueTest.java @@ -0,0 +1,101 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.internal; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.Queue; +import java.util.Random; +import java.util.concurrent.ArrayBlockingQueue; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; + +public class ArrayQueueTest { + + @ParameterizedTest + @ValueSource(ints = {-1, 0}) + void invalid_capacity_should_not_be_allowed(final int invalidCapacity) { + assertThatThrownBy(() -> new ArrayQueue<>(invalidCapacity)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("invalid capacity: " + invalidCapacity); + } + + @Test + void should_work_with_capacity_1() { + + // Verify initials + final Queue<String> queue = new ArrayQueue<>(1); + assertThat(queue.size()).isEqualTo(0); + assertThat(queue.peek()).isNull(); + assertThat(queue.poll()).isNull(); + assertThat(queue).isEmpty(); + + // Verify enqueue & deque + assertThat(queue.offer("foo")).isTrue(); + assertThat(queue.offer("bar")).isFalse(); + assertThat(queue.size()).isEqualTo(1); + assertThat(queue).containsOnly("foo"); + assertThat(queue.peek()).isEqualTo("foo"); + assertThat(queue.poll()).isEqualTo("foo"); + + // Verify final state + assertThat(queue.size()).isEqualTo(0); + assertThat(queue.peek()).isNull(); + assertThat(queue.poll()).isNull(); + assertThat(queue).isEmpty(); + } + + @ParameterizedTest + @CsvSource({ + "1,0.3", "1,0.5", "1,0.8", "2,0.3", "2,0.5", "2,0.8", "3,0.3", "3,0.5", "3,0.8", "4,0.3", "4,0.5", "4,0.8" + }) + void ops_should_match_with_std_lib(final int capacity, final double pollRatio) { + + // Set the stage + final Random random = new Random(0); + final int opCount = random.nextInt(100); + final Queue<String> queueRef = new ArrayBlockingQueue<>(capacity); + final Queue<String> queueTarget = new ArrayQueue<>(capacity); + + for (int opIndex = 0; opIndex < opCount; opIndex++) { + + // Verify entry + assertThat(queueTarget.size()).isEqualTo(queueRef.size()); + assertThat(queueTarget.peek()).isEqualTo(queueRef.peek()); + assertThat(queueTarget).containsExactlyElementsOf(queueRef); + + // Is this a `poll()`? + if (pollRatio >= random.nextDouble()) { + assertThat(queueTarget.poll()).isEqualTo(queueRef.poll()); + } + + // Then this is an `offer()` + else { + final String item = "op@" + opIndex; + assertThat(queueTarget.offer(item)).isEqualTo(queueRef.offer(item)); + } + + // Verify exit + assertThat(queueTarget.size()).isEqualTo(queueRef.size()); + assertThat(queueTarget.peek()).isEqualTo(queueRef.peek()); + assertThat(queueTarget).containsExactlyElementsOf(queueRef); + } + } +} diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageRecursiveFormattingTestBase.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageRecursiveFormattingTestBase.java new file mode 100644 index 0000000000..072696029a --- /dev/null +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageRecursiveFormattingTestBase.java @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.message; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.apache.logging.log4j.util.Constants; +import org.junit.jupiter.api.Test; + +/** + * Tests {@link ParameterizedMessage#getFormattedMessage()} when formatted arguments causes another {@code ParameterizedMessage#getFormattedMessage()} (i.e., recursive) invocation. + */ +abstract class ParameterizedMessageRecursiveFormattingTestBase { + + private final boolean threadLocalsEnabled; + + ParameterizedMessageRecursiveFormattingTestBase(boolean threadLocalsEnabled) { + this.threadLocalsEnabled = threadLocalsEnabled; + } + + @Test + void thread_locals_toggle_should_match() { + assertThat(Constants.ENABLE_THREADLOCALS).isEqualTo(threadLocalsEnabled); + } + + @Test + void recursion_should_not_corrupt_formatting() { + final Object argInvokingParameterizedMessageFormatting = new Object() { + @Override + public String toString() { + return new ParameterizedMessage("bar {}", "baz").getFormattedMessage(); + } + }; + final ParameterizedMessage message = + new ParameterizedMessage("foo {}", argInvokingParameterizedMessageFormatting); + final String actualText = message.getFormattedMessage(); + assertThat(actualText).isEqualTo("foo bar baz"); + } +} diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageRecursiveFormattingWithThreadLocalsTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageRecursiveFormattingWithThreadLocalsTest.java new file mode 100644 index 0000000000..6496a5f12f --- /dev/null +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageRecursiveFormattingWithThreadLocalsTest.java @@ -0,0 +1,31 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.message; + +import org.junitpioneer.jupiter.SetSystemProperty; + +/** + * {@link ParameterizedMessageRecursiveFormattingTestBase} subclass with thread locals disabled, i.e, with {@link StringBuilder} recycling. + */ +@SetSystemProperty(key = "log4j2.enable.threadlocals", value = "true") +class ParameterizedMessageRecursiveFormattingWithThreadLocalsTest + extends ParameterizedMessageRecursiveFormattingTestBase { + + ParameterizedMessageRecursiveFormattingWithThreadLocalsTest() { + super(true); + } +} diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageRecursiveFormattingWithoutThreadLocalsTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageRecursiveFormattingWithoutThreadLocalsTest.java new file mode 100644 index 0000000000..456d271d87 --- /dev/null +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageRecursiveFormattingWithoutThreadLocalsTest.java @@ -0,0 +1,31 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.message; + +import org.junitpioneer.jupiter.SetSystemProperty; + +/** + * {@link ParameterizedMessageRecursiveFormattingTestBase} subclass with thread locals disabled, i.e, no {@link StringBuilder} recycling. + */ +@SetSystemProperty(key = "log4j2.enable.threadlocals", value = "false") +class ParameterizedMessageRecursiveFormattingWithoutThreadLocalsTest + extends ParameterizedMessageRecursiveFormattingTestBase { + + ParameterizedMessageRecursiveFormattingWithoutThreadLocalsTest() { + super(false); + } +} diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/internal/ArrayQueue.java b/log4j-api/src/main/java/org/apache/logging/log4j/internal/ArrayQueue.java new file mode 100644 index 0000000000..6777d56930 --- /dev/null +++ b/log4j-api/src/main/java/org/apache/logging/log4j/internal/ArrayQueue.java @@ -0,0 +1,101 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.internal; + +import java.util.AbstractQueue; +import java.util.Iterator; +import java.util.stream.IntStream; +import javax.annotation.concurrent.NotThreadSafe; +import org.apache.logging.log4j.util.InternalApi; + +/** + * An array-backed, fixed-length, not-thread-safe {@link java.util.Queue} implementation. + * + * @param <E> the element type + * @since 2.23.0 + */ +@InternalApi +@NotThreadSafe +final class ArrayQueue<E> extends AbstractQueue<E> { + + private final E[] buffer; + + private int head; + + private int tail; + + private int size; + + @SuppressWarnings("unchecked") + ArrayQueue(final int capacity) { + if (capacity < 1) { + throw new IllegalArgumentException("invalid capacity: " + capacity); + } + buffer = (E[]) new Object[capacity]; + head = 0; + tail = -1; + size = 0; + } + + @Override + public Iterator<E> iterator() { + int[] i = {head}; + return IntStream.range(0, size) + .mapToObj(ignored -> { + final E item = buffer[i[0]]; + i[0] = (i[0] + 1) % buffer.length; + return item; + }) + .iterator(); + } + + @Override + public boolean offer(final E item) { + if (size == buffer.length) { + return false; + } + tail = (tail + 1) % buffer.length; + buffer[tail] = item; + size++; + return true; + } + + @Override + public E poll() { + if (isEmpty()) { + return null; + } + final E item = buffer[head]; + buffer[head] = null; // Clear refs for GC + head = (head + 1) % buffer.length; + size--; + return item; + } + + @Override + public E peek() { + if (isEmpty()) { + return null; + } + return buffer[head]; + } + + @Override + public int size() { + return size; + } +} diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/internal/DummyStringBuilderRecycler.java b/log4j-api/src/main/java/org/apache/logging/log4j/internal/DummyStringBuilderRecycler.java new file mode 100644 index 0000000000..9deecccce5 --- /dev/null +++ b/log4j-api/src/main/java/org/apache/logging/log4j/internal/DummyStringBuilderRecycler.java @@ -0,0 +1,35 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.internal; + +/** + * A {@link StringBuilderRecycler} that <b>always</b> allocates a new instance. + * + * @since 2.23.0 + */ +final class DummyStringBuilderRecycler implements StringBuilderRecycler { + + DummyStringBuilderRecycler() {} + + @Override + public StringBuilder acquire() { + return new StringBuilder(); + } + + @Override + public void release(final StringBuilder stringBuilder) {} +} diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/internal/QueueingThreadLocalStringBuilderRecycler.java b/log4j-api/src/main/java/org/apache/logging/log4j/internal/QueueingThreadLocalStringBuilderRecycler.java new file mode 100644 index 0000000000..d867a8df28 --- /dev/null +++ b/log4j-api/src/main/java/org/apache/logging/log4j/internal/QueueingThreadLocalStringBuilderRecycler.java @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.internal; + +import edu.umd.cs.findbugs.annotations.Nullable; +import java.util.Queue; +import org.apache.logging.log4j.util.StringBuilders; + +/** + * A {@link StringBuilderRecycler} that keeps a {@link ThreadLocal} queue to recycle instances. + * <p> + * When the queue capacity is exceeded, {@link #acquire()} starts allocating new instances. + * The queue provides convenience when recursive {@link #acquire()} calls are expected. + * If not, use {@link ThreadLocalStringBuilderRecycler} instead to avoid the cost of maintaining a queue. + * </p> + * + * @since 2.23.0 + */ +final class QueueingThreadLocalStringBuilderRecycler implements StringBuilderRecycler { + + private final ThreadLocal<Queue<StringBuilder>> queueHolder; + + QueueingThreadLocalStringBuilderRecycler(final int threadLocalQueueCapacity) { + this.queueHolder = ThreadLocal.withInitial(() -> new ArrayQueue<>(threadLocalQueueCapacity)); + } + + @Override + public StringBuilder acquire() { + Queue<StringBuilder> queue = queueHolder.get(); + @Nullable final StringBuilder stringBuilder = queue.poll(); + return stringBuilder != null ? stringBuilder : new StringBuilder(); + } + + @Override + public void release(final StringBuilder stringBuilder) { + StringBuilders.trimToMaxSize(stringBuilder, MAX_STRING_BUILDER_CAPACITY); + stringBuilder.setLength(0); + final Queue<StringBuilder> queue = queueHolder.get(); + queue.offer(stringBuilder); + } +} diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/internal/StringBuilderRecycler.java b/log4j-api/src/main/java/org/apache/logging/log4j/internal/StringBuilderRecycler.java new file mode 100644 index 0000000000..3ff96c1152 --- /dev/null +++ b/log4j-api/src/main/java/org/apache/logging/log4j/internal/StringBuilderRecycler.java @@ -0,0 +1,61 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.internal; + +import org.apache.logging.log4j.util.Constants; +import org.apache.logging.log4j.util.InternalApi; + +/** + * An <b>internal</b> tool to recycle {@link StringBuilder} instances to implement garbage-free formatting. + * <h1>Internal usage only!</h1> + * <p> + * This interface is only intended to be used for internals of the Log4j API. + * Log4j API can break this contract at any time to adapt to its needs. + * <b>{@link StringBuilderRecycler} should not be used by any other code except the Log4j API!</b> + * </p> + * + * @since 2.23.0 + */ +@InternalApi +public interface StringBuilderRecycler { + + int MAX_STRING_BUILDER_CAPACITY = Constants.MAX_REUSABLE_MESSAGE_SIZE; + + StringBuilder acquire(); + + void release(final StringBuilder stringBuilder); + + /** + * Creates a {@link StringBuilderRecycler} using the current environment, e.g., {@link Constants}, {@link org.apache.logging.log4j.util.PropertiesUtil}, etc. + * + * @param recursionDepth This value indicates the maximum recursion depth to be supported before falling back to allocating new instances. + * For instance, {@link QueueingThreadLocalStringBuilderRecycler} uses this to set the capacity of the instances to be pooled in a {@link ThreadLocal}. + * @return a new {@link StringBuilderRecycler} instance + */ + static StringBuilderRecycler ofEnvironment(final int recursionDepth) { + if (recursionDepth < 0) { + throw new IllegalArgumentException("was expecting a positive `recursionDepth`, found: " + recursionDepth); + } + if (!Constants.ENABLE_THREADLOCALS) { + return new DummyStringBuilderRecycler(); + } else if (recursionDepth == 0) { + return new ThreadLocalStringBuilderRecycler(); + } else { + return new QueueingThreadLocalStringBuilderRecycler(recursionDepth); + } + } +} diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/internal/ThreadLocalStringBuilderRecycler.java b/log4j-api/src/main/java/org/apache/logging/log4j/internal/ThreadLocalStringBuilderRecycler.java new file mode 100644 index 0000000000..6638fb6e6a --- /dev/null +++ b/log4j-api/src/main/java/org/apache/logging/log4j/internal/ThreadLocalStringBuilderRecycler.java @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.internal; + +import edu.umd.cs.findbugs.annotations.Nullable; +import org.apache.logging.log4j.util.StringBuilders; + +/** + * A {@link StringBuilderRecycler} that stores a single {@link StringBuilder} in a {@link ThreadLocal}. + * <p> + * This implementation causes new instance creation when {@link #acquire()} is called recursively. + * If your use case needs recursion support, prefer {@link QueueingThreadLocalStringBuilderRecycler} instead. + * </p> + * + * @since 2.23.0 + */ +final class ThreadLocalStringBuilderRecycler implements StringBuilderRecycler { + + private final ThreadLocal<StringBuilder> stringBuilderHolder = ThreadLocal.withInitial(StringBuilder::new); + + ThreadLocalStringBuilderRecycler() {} + + @Override + public StringBuilder acquire() { + @Nullable StringBuilder stringBuilder = stringBuilderHolder.get(); + if (stringBuilder == null) { + return new StringBuilder(); + } + // noinspection ThreadLocalSetWithNull + stringBuilderHolder.set(null); + return stringBuilder; + } + + @Override + public void release(final StringBuilder stringBuilder) { + StringBuilders.trimToMaxSize(stringBuilder, MAX_STRING_BUILDER_CAPACITY); + stringBuilder.setLength(0); + stringBuilderHolder.set(stringBuilder); + } +} diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMessage.java b/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMessage.java index db325327b7..30677a8565 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMessage.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMessage.java @@ -25,10 +25,9 @@ import java.io.ObjectOutputStream; import java.io.Serializable; import java.util.Arrays; import java.util.Objects; +import org.apache.logging.log4j.internal.StringBuilderRecycler; import org.apache.logging.log4j.message.ParameterFormatter.MessagePatternAnalysis; -import org.apache.logging.log4j.util.Constants; import org.apache.logging.log4j.util.StringBuilderFormattable; -import org.apache.logging.log4j.util.StringBuilders; import org.apache.logging.log4j.util.internal.SerializationUtil; /** @@ -44,10 +43,6 @@ import org.apache.logging.log4j.util.internal.SerializationUtil; * {bar * {buzz} * </pre> - * <p> - * This class was originally written for <a href="http://lilithapp.com/";>Lilith</a> by Jörn Huxhorn and licensed under the LGPL. - * It has been relicensed here with his permission providing that this attribution remain. - * </p> */ public class ParameterizedMessage implements Message, StringBuilderFormattable { @@ -83,9 +78,15 @@ public class ParameterizedMessage implements Message, StringBuilderFormattable { private static final long serialVersionUID = -665975803997290697L; - private static final ThreadLocal<StringBuilder> STRING_BUILDER_HOLDER = Constants.ENABLE_THREADLOCALS - ? ThreadLocal.withInitial(() -> new StringBuilder(Constants.MAX_REUSABLE_MESSAGE_SIZE)) - : null; + private static final StringBuilderRecycler STRING_BUILDER_RECYCLER = StringBuilderRecycler.ofEnvironment( + // This value indicates the maximum recursion depth supported before the recycler starts creating new + // instances. + // Consider a `ParameterizedMessage` containing an argument whose `toString()` causes another + // `ParameterizedMessage` formatting. + // This value indicates the depth we support garbage-free formatting in such nested formatting situations. + // When this depth is exceeded, code still works, but starts generating garbage due to new `StringBuilder` + // allocations. + 3); private final String pattern; @@ -242,16 +243,13 @@ public class ParameterizedMessage implements Message, StringBuilderFormattable { @Override public String getFormattedMessage() { if (formattedMessage == null) { - if (STRING_BUILDER_HOLDER != null) { - final StringBuilder buffer = STRING_BUILDER_HOLDER.get(); + final StringBuilder buffer = STRING_BUILDER_RECYCLER.acquire(); + try { buffer.setLength(0); formatTo(buffer); formattedMessage = buffer.toString(); - StringBuilders.trimToMaxSize(buffer, Constants.MAX_REUSABLE_MESSAGE_SIZE); - } else { - final StringBuilder buffer = new StringBuilder(); - formatTo(buffer); - formattedMessage = buffer.toString(); + } finally { + STRING_BUILDER_RECYCLER.release(buffer); } } return formattedMessage; @@ -355,26 +353,28 @@ public class ParameterizedMessage implements Message, StringBuilderFormattable { @Override public String toString() { - return "ParameterizedMessage[messagePattern=" + pattern + ", stringArgs=" + Arrays.toString(args) - + ", throwable=" + throwable + ']'; + // Avoid formatting arguments! + // It can cause recursion, which can become pretty unpleasant while troubleshooting. + return "ParameterizedMessage[messagePattern=" + pattern + ", argCount=" + args.length + ", throwableProvided=" + + (throwable != null) + ']'; } private void writeObject(final ObjectOutputStream out) throws IOException { out.defaultWriteObject(); out.writeInt(args.length); - for (int i = 0; i < args.length; i++) { - SerializationUtil.writeWrappedObject( - args[i] instanceof Serializable ? (Serializable) args[i] : String.valueOf(args[i]), out); + for (final Object arg : args) { + final Serializable serializableArg = arg instanceof Serializable ? (Serializable) arg : String.valueOf(arg); + SerializationUtil.writeWrappedObject(serializableArg, out); } } private void readObject(final ObjectInputStream in) throws IOException, ClassNotFoundException { SerializationUtil.assertFiltered(in); in.defaultReadObject(); - final int length = in.readInt(); - args = new Object[length]; - for (int i = 0; i < args.length; i++) { - args[i] = SerializationUtil.readWrappedObject(in); + final int argCount = in.readInt(); + args = new Object[argCount]; + for (int argIndex = 0; argIndex < args.length; argIndex++) { + args[argIndex] = SerializationUtil.readWrappedObject(in); } } } diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/Strings.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/Strings.java index a1a5bb1aab..512784d3c8 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/util/Strings.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/util/Strings.java @@ -19,6 +19,7 @@ package org.apache.logging.log4j.util; import java.util.Iterator; import java.util.Locale; import java.util.Objects; +import org.apache.logging.log4j.internal.StringBuilderRecycler; /** * <em>Consider this class private.</em> @@ -28,7 +29,7 @@ import java.util.Objects; @InternalApi public final class Strings { - private static final ThreadLocal<StringBuilder> tempStr = ThreadLocal.withInitial(StringBuilder::new); + private static final StringBuilderRecycler STRING_BUILDER_RECYCLER = StringBuilderRecycler.ofEnvironment(0); /** * The empty string. @@ -318,11 +319,11 @@ public final class Strings { } else if (isEmpty(str2)) { return str1; } - final StringBuilder sb = tempStr.get(); + final StringBuilder sb = STRING_BUILDER_RECYCLER.acquire(); try { return sb.append(str1).append(str2).toString(); } finally { - sb.setLength(0); + STRING_BUILDER_RECYCLER.release(sb); } } @@ -338,14 +339,14 @@ public final class Strings { if (count < 0) { throw new IllegalArgumentException("count"); } - final StringBuilder sb = tempStr.get(); + final StringBuilder sb = STRING_BUILDER_RECYCLER.acquire(); try { for (int index = 0; index < count; index++) { sb.append(str); } return sb.toString(); } finally { - sb.setLength(0); + STRING_BUILDER_RECYCLER.release(sb); } } } diff --git a/src/changelog/.2.x.x/fix_api_recursive_formatting.xml b/src/changelog/.2.x.x/fix_api_recursive_formatting.xml new file mode 100644 index 0000000000..558152b60c --- /dev/null +++ b/src/changelog/.2.x.x/fix_api_recursive_formatting.xml @@ -0,0 +1,9 @@ +<?xml version="1.0" encoding="UTF-8"?> +<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xmlns="http://logging.apache.org/log4j/changelog"; + xsi:schemaLocation="http://logging.apache.org/log4j/changelog https://logging.apache.org/log4j/changelog-0.1.3.xsd"; + type="fixed"> + <description format="asciidoc"> + Fix `StringBuilder` cache corruption on recursive access + </description> +</entry>
