This is an automated email from the ASF dual-hosted git repository. pkarwasz pushed a commit to branch fix/2.x/3819-logback-builder-reuse in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit 05d97d2ac1ed51b071541d53b7e18b8852d4c18a Author: Piotr P. Karwasz <[email protected]> AuthorDate: Mon Jul 14 13:21:24 2025 +0200 fix: Prevent `LogBuilder` memory leak in Log4j API to Logback bridge This change fixes a potential memory leak in the `log4j-to-slf4j` module (Log4j API to Logback bridge) caused by the use of a thread-local `LogBuilder` pool. The leak occurred because the thread-local field was accessed, even when `log4j2.enableThreadlocals` was set to `false`. In servlet environments, this could lead to classloader leaks due to the persistence of thread-local references. With this fix, all access to the `ThreadLocal` is now **properly** gated by the `log4j2.enableThreadlocals` system property—matching how similar pooling behavior is handled in Log4j Core. This preserves the GC-free option for advanced users who explicitly opt in via `log4j2.enableThreadlocals = true`, while avoiding memory leaks for others. Fixes #3819 --- log4j-to-slf4j/pom.xml | 11 ++++++ .../java/org/apache/logging/slf4j/SLF4JLogger.java | 16 +++++--- .../{LoggerTest.java => SLF4JLoggerTest.java} | 43 +++++++++++++++++++++- .../slf4j/{LoggerTest.xml => SLF4JLoggerTest.xml} | 0 .../.2.x.x/3819_logback-builder-reuse.xml | 12 ++++++ 5 files changed, 75 insertions(+), 7 deletions(-) diff --git a/log4j-to-slf4j/pom.xml b/log4j-to-slf4j/pom.xml index 3a6b0d9e85..95e3ec5fc9 100644 --- a/log4j-to-slf4j/pom.xml +++ b/log4j-to-slf4j/pom.xml @@ -153,6 +153,17 @@ </execution> </executions> </plugin> + + <!-- Illegal access is disabled by default in Java 16 due to JEP-396. + We are relaxing it for tests. --> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-surefire-plugin</artifactId> + <configuration> + <argLine>--add-opens java.base/java.lang=ALL-UNNAMED</argLine> + </configuration> + </plugin> + </plugins> </build> </project> diff --git a/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLogger.java b/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLogger.java index 26e94c67b3..ff9410f33e 100644 --- a/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLogger.java +++ b/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLogger.java @@ -42,17 +42,21 @@ public class SLF4JLogger extends AbstractLogger { private final org.slf4j.Logger logger; private final LocationAwareLogger locationAwareLogger; + private final boolean useThreadLocal; public SLF4JLogger(final String name, final MessageFactory messageFactory, final org.slf4j.Logger logger) { - super(name, messageFactory); - this.logger = logger; - this.locationAwareLogger = logger instanceof LocationAwareLogger ? (LocationAwareLogger) logger : null; + this(name, messageFactory, logger, Constants.ENABLE_THREADLOCALS); } public SLF4JLogger(final String name, final org.slf4j.Logger logger) { - super(name); + this(name, null, logger); + } + + SLF4JLogger(String name, MessageFactory messageFactory, org.slf4j.Logger logger, boolean useThreadLocal) { + super(name, messageFactory); this.logger = logger; this.locationAwareLogger = logger instanceof LocationAwareLogger ? (LocationAwareLogger) logger : null; + this.useThreadLocal = useThreadLocal; } private int convertLevel(final Level level) { @@ -364,8 +368,8 @@ public class SLF4JLogger extends AbstractLogger { @Override protected LogBuilder getLogBuilder(final Level level) { - final SLF4JLogBuilder builder = logBuilder.get(); - return Constants.ENABLE_THREADLOCALS && !builder.isInUse() + SLF4JLogBuilder builder; + return useThreadLocal && !(builder = logBuilder.get()).isInUse() ? builder.reset(this, level) : new SLF4JLogBuilder(this, level); } diff --git a/log4j-to-slf4j/src/test/java/org/apache/logging/slf4j/LoggerTest.java b/log4j-to-slf4j/src/test/java/org/apache/logging/slf4j/SLF4JLoggerTest.java similarity index 82% rename from log4j-to-slf4j/src/test/java/org/apache/logging/slf4j/LoggerTest.java rename to log4j-to-slf4j/src/test/java/org/apache/logging/slf4j/SLF4JLoggerTest.java index 6c378ad958..9e3a5856a0 100644 --- a/log4j-to-slf4j/src/test/java/org/apache/logging/slf4j/LoggerTest.java +++ b/log4j-to-slf4j/src/test/java/org/apache/logging/slf4j/SLF4JLoggerTest.java @@ -32,10 +32,13 @@ import static org.junit.jupiter.api.Assertions.fail; import ch.qos.logback.classic.LoggerContext; import ch.qos.logback.classic.spi.ILoggingEvent; import ch.qos.logback.core.testUtil.StringListAppender; +import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.lang.reflect.Proxy; import java.util.Date; import java.util.List; +import org.apache.logging.log4j.LogBuilder; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.ThreadContext; @@ -45,13 +48,17 @@ import org.apache.logging.log4j.message.StringFormatterMessageFactory; import org.apache.logging.log4j.spi.AbstractLogger; import org.apache.logging.log4j.spi.MessageFactory2Adapter; import org.apache.logging.log4j.test.junit.UsingStatusListener; +import org.assertj.core.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.junitpioneer.jupiter.Issue; import org.slf4j.MDC; @UsingStatusListener @LoggerContextSource -class LoggerTest { +class SLF4JLoggerTest { private static final Object OBJ = new Object(); // Log4j objects @@ -265,4 +272,38 @@ class LoggerTest { // expected } } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + @Issue("https://github.com/apache/logging-log4j2/issues/3819";) + void threadLocalUsage(boolean useThreadLocal) throws ReflectiveOperationException { + // Reset the static ThreadLocal in SLF4JLogger + getLogBuilderThreadLocal().remove(); + final org.slf4j.Logger slf4jLogger = context.getLogger(getClass()); + Logger logger = new SLF4JLogger(slf4jLogger.getName(), null, slf4jLogger, useThreadLocal); + LogBuilder builder1 = logger.atInfo(); + builder1.log("Test message"); + LogBuilder builder2 = logger.atInfo(); + builder2.log("Another test message"); + // Check if the same builder is reused based on the useThreadLocal flag + Assertions.assertThat(isThreadLocalPresent()) + .as("ThreadLocal should be present iff useThreadLocal is enabled") + .isEqualTo(useThreadLocal); + Assertions.assertThat(builder2 == builder1) + .as("Builder2 should be the same as Builder1 iff useThreadLocal is enabled") + .isEqualTo(useThreadLocal); + } + + private static boolean isThreadLocalPresent() throws ReflectiveOperationException { + Method isPresentMethod = ThreadLocal.class.getDeclaredMethod("isPresent"); + isPresentMethod.setAccessible(true); + ThreadLocal<?> threadLocal = getLogBuilderThreadLocal(); + return (boolean) isPresentMethod.invoke(threadLocal); + } + + private static ThreadLocal<?> getLogBuilderThreadLocal() throws ReflectiveOperationException { + Field logBuilderField = SLF4JLogger.class.getDeclaredField("logBuilder"); + logBuilderField.setAccessible(true); + return (ThreadLocal<?>) logBuilderField.get(null); + } } diff --git a/log4j-to-slf4j/src/test/resources/org/apache/logging/slf4j/LoggerTest.xml b/log4j-to-slf4j/src/test/resources/org/apache/logging/slf4j/SLF4JLoggerTest.xml similarity index 100% rename from log4j-to-slf4j/src/test/resources/org/apache/logging/slf4j/LoggerTest.xml rename to log4j-to-slf4j/src/test/resources/org/apache/logging/slf4j/SLF4JLoggerTest.xml diff --git a/src/changelog/.2.x.x/3819_logback-builder-reuse.xml b/src/changelog/.2.x.x/3819_logback-builder-reuse.xml new file mode 100644 index 0000000000..ea67aa50d0 --- /dev/null +++ b/src/changelog/.2.x.x/3819_logback-builder-reuse.xml @@ -0,0 +1,12 @@ +<?xml version="1.0" encoding="UTF-8"?> +<entry xmlns="https://logging.apache.org/xml/ns"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation=" + https://logging.apache.org/xml/ns + https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"; + type="fixed"> + <issue id="3819" link="https://github.com/apache/logging-log4j2/issues/3819"/> + <description format="asciidoc"> + Fix potential memory leak involving `LogBuilder` in Log4j API to Logback bridge. + </description> +</entry>
