From 05d97d2ac1ed51b071541d53b7e18b8852d4c18a Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 14 Jul 2025 13:21:24 +0200 Subject: [PATCH 1/2] fix: Prevent `LogBuilder` memory leak in Log4j API to Logback bridge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 +++++ .../org/apache/logging/slf4j/SLF4JLogger.java | 16 ++++--- .../{LoggerTest.java => SLF4JLoggerTest.java} | 43 ++++++++++++++++++- .../{LoggerTest.xml => SLF4JLoggerTest.xml} | 0 .../.2.x.x/3819_logback-builder-reuse.xml | 12 ++++++ 5 files changed, 75 insertions(+), 7 deletions(-) rename log4j-to-slf4j/src/test/java/org/apache/logging/slf4j/{LoggerTest.java => SLF4JLoggerTest.java} (82%) rename log4j-to-slf4j/src/test/resources/org/apache/logging/slf4j/{LoggerTest.xml => SLF4JLoggerTest.xml} (100%) create mode 100644 src/changelog/.2.x.x/3819_logback-builder-reuse.xml diff --git a/log4j-to-slf4j/pom.xml b/log4j-to-slf4j/pom.xml index 3a6b0d9e850..95e3ec5fc9d 100644 --- a/log4j-to-slf4j/pom.xml +++ b/log4j-to-slf4j/pom.xml @@ -153,6 +153,17 @@ + + + + org.apache.maven.plugins + maven-surefire-plugin + + --add-opens java.base/java.lang=ALL-UNNAMED + + + 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 26e94c67b35..ff9410f33e1 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 LogBuilder atFatal() { @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 6c378ad9583..9e3a5856a06 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 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.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 @@ void mdcCannotContainNullKey() { // 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 00000000000..ea67aa50d04 --- /dev/null +++ b/src/changelog/.2.x.x/3819_logback-builder-reuse.xml @@ -0,0 +1,12 @@ + + + + + Fix potential memory leak involving `LogBuilder` in Log4j API to Logback bridge. + + From a792145fdceded9bda43570eca7900067182f730 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 14 Jul 2025 14:29:33 +0200 Subject: [PATCH 2/2] fix(build): Move `--add-opens` option to Maven profile The `--add-opens` JVM option is now included in a dedicated Maven profile to avoid applying it during CI builds on JRE 8, where it is unsupported and unnecessary. --- log4j-to-slf4j/pom.xml | 48 +++++++++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 10 deletions(-) diff --git a/log4j-to-slf4j/pom.xml b/log4j-to-slf4j/pom.xml index 95e3ec5fc9d..a2d6f0a6647 100644 --- a/log4j-to-slf4j/pom.xml +++ b/log4j-to-slf4j/pom.xml @@ -154,16 +154,44 @@ - - - org.apache.maven.plugins - maven-surefire-plugin - - --add-opens java.base/java.lang=ALL-UNNAMED - - - + + + + + + + java8-incompat-fixes + + + + + !env.CI + + + + + + + + org.apache.maven.plugins + maven-surefire-plugin + + --add-opens java.base/java.lang=ALL-UNNAMED + + + + + + + +