From 75388ffffa7e77db65d38c83040257167d56c885 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Thu, 21 May 2026 07:35:36 -0700 Subject: [PATCH 1/6] Close URLConnection on error paths in ConfigurationSource In getConfigurationSource(URL), the URLConnection opened via UrlConnectionFactory.createConnection() was never closed or disconnected when an error occurred (FileNotFoundException, IOException, URISyntaxException). For HttpURLConnection this leaks a socket; for JarURLConnection this leaks a file handle. Fix: use a success flag with try-finally to ensure the connection is cleaned up on all error paths while leaving it open on the success path where the stream is handed off to ConfigurationSource. Also replace the redundant FileUtils.fileFromUri(url.toURI()) call on the return line with the existing 'file' variable. --- .../core/config/ConfigurationSource.java | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java index dd50987f257..ad7bc307f06 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java @@ -23,6 +23,7 @@ import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; +import java.net.HttpURLConnection; import java.net.JarURLConnection; import java.net.MalformedURLException; import java.net.URI; @@ -359,24 +360,39 @@ public String toString() { try { final File file = FileUtils.fileFromUri(url.toURI()); final URLConnection urlConnection = UrlConnectionFactory.createConnection(url); + boolean success = false; try { + final ConfigurationSource source; if (file != null) { - return new ConfigurationSource(urlConnection.getInputStream(), FileUtils.fileFromUri(url.toURI())); + source = new ConfigurationSource(urlConnection.getInputStream(), file); } else if (urlConnection instanceof JarURLConnection) { // Work around https://bugs.openjdk.java.net/browse/JDK-6956385. URL jarFileUrl = ((JarURLConnection) urlConnection).getJarFileURL(); File jarFile = new File(jarFileUrl.getFile()); long lastModified = jarFile.lastModified(); - return new ConfigurationSource(urlConnection.getInputStream(), url, lastModified); + source = new ConfigurationSource(urlConnection.getInputStream(), url, lastModified); } else { - return new ConfigurationSource( + source = new ConfigurationSource( urlConnection.getInputStream(), url, urlConnection.getLastModified()); } - } catch (FileNotFoundException ex) { + success = true; + return source; + } catch (final FileNotFoundException ex) { ConfigurationFactory.LOGGER.info("Unable to locate file {}, ignoring.", url.toString()); return null; + } finally { + if (!success) { + try { + urlConnection.getInputStream().close(); + } catch (final IOException ignored) { + // Best-effort cleanup; the stream may not have been opened + } + if (urlConnection instanceof HttpURLConnection) { + ((HttpURLConnection) urlConnection).disconnect(); + } + } } - } catch (IOException | URISyntaxException ex) { + } catch (final IOException | URISyntaxException ex) { ConfigurationFactory.LOGGER.warn( "Error accessing {} due to {}, ignoring.", url.toString(), ex.getMessage()); return null; From 3fde88bf083a7248c42978d9f6cb9bb373c6a86c Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Thu, 21 May 2026 13:31:36 -0700 Subject: [PATCH 2/6] Add failure test and changelog entry Add testNoJarFileLeakOnFailure to verify that a failed JAR configuration load (missing entry) does not leak file descriptors or hold a lock on the JAR file. Add changelog entry for the ConfigurationSource URL leak fix. --- .../core/config/ConfigurationSourceTest.java | 22 +++++++++++++++++++ .../fix_configuration_source_url_leak.xml | 8 +++++++ 2 files changed, 30 insertions(+) create mode 100644 src/changelog/.2.x.x/fix_configuration_source_url_leak.xml diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java index 3015f4f6271..313fefb138f 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java @@ -104,6 +104,28 @@ void testLoadConfigurationSourceFromJarFile() throws Exception { } } + /** + * Verifies that a failed JAR configuration load (missing entry) does not leak file descriptors + * or hold a lock on the JAR file. + */ + @Test + void testNoJarFileLeakOnFailure() throws Exception { + final Path jarFile = prepareJarConfigURL(tempDir); + // Path inside JAR that does NOT exist + final URL brokenJarConfigURL = new URL("jar:" + jarFile.toUri().toURL() + "!/missing/file.xml"); + final long expectedFdCount = getOpenFileDescriptorCount(); + final ConfigurationSource configSource = ConfigurationSource.fromUri(brokenJarConfigURL.toURI()); + assertNull(configSource, "Source should be null for a missing JAR entry"); + // This can only fail on UNIX + assertEquals(expectedFdCount, getOpenFileDescriptorCount(), "File descriptors leaked after failed JAR load"); + // This can only fail on Windows + try { + Files.delete(jarFile); + } catch (IOException e) { + fail("JAR file locked after failed load: " + e.getMessage()); + } + } + private long getOpenFileDescriptorCount() { final OperatingSystemMXBean os = ManagementFactory.getOperatingSystemMXBean(); if (os instanceof UnixOperatingSystemMXBean) { diff --git a/src/changelog/.2.x.x/fix_configuration_source_url_leak.xml b/src/changelog/.2.x.x/fix_configuration_source_url_leak.xml new file mode 100644 index 00000000000..e7c57d5a1a1 --- /dev/null +++ b/src/changelog/.2.x.x/fix_configuration_source_url_leak.xml @@ -0,0 +1,8 @@ + + + + Fix resource leaks in `ConfigurationSource` when loading configuration via URL fails + From bbed9f27fe97d26ac3867b3b5d0bb069ad48ae55 Mon Sep 17 00:00:00 2001 From: Ramanathan Date: Fri, 22 May 2026 03:18:54 +0530 Subject: [PATCH 3/6] Fix potential URLConnection leak by ensuring InputStream is closed on error paths --- .../core/config/ConfigurationSourceTest.java | 23 ++++++++----------- .../core/config/ConfigurationSource.java | 21 ++++++++++------- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java index 313fefb138f..b9300bb7af3 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java @@ -30,6 +30,7 @@ import java.io.OutputStream; import java.lang.management.ManagementFactory; import java.lang.management.OperatingSystemMXBean; +import java.net.URI; import java.net.URL; import java.nio.file.Files; import java.nio.file.Path; @@ -109,21 +110,17 @@ void testLoadConfigurationSourceFromJarFile() throws Exception { * or hold a lock on the JAR file. */ @Test - void testNoJarFileLeakOnFailure() throws Exception { + void testJarFileLeakOnFailure() throws Exception { final Path jarFile = prepareJarConfigURL(tempDir); - // Path inside JAR that does NOT exist - final URL brokenJarConfigURL = new URL("jar:" + jarFile.toUri().toURL() + "!/missing/file.xml"); + final String jarUriStr = "jar:" + jarFile.toUri().toASCIIString() + "!/" + java.util.UUID.randomUUID(); + final URI jarUri = URI.create(jarUriStr); + final long expectedFdCount = getOpenFileDescriptorCount(); - final ConfigurationSource configSource = ConfigurationSource.fromUri(brokenJarConfigURL.toURI()); - assertNull(configSource, "Source should be null for a missing JAR entry"); - // This can only fail on UNIX - assertEquals(expectedFdCount, getOpenFileDescriptorCount(), "File descriptors leaked after failed JAR load"); - // This can only fail on Windows - try { - Files.delete(jarFile); - } catch (IOException e) { - fail("JAR file locked after failed load: " + e.getMessage()); - } + + assertNull(ConfigurationSource.fromUri(jarUri)); + + assertEquals(expectedFdCount, getOpenFileDescriptorCount()); + Files.delete(jarFile); } private long getOpenFileDescriptorCount() { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java index ad7bc307f06..f8e61daa9a1 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java @@ -361,19 +361,22 @@ public String toString() { final File file = FileUtils.fileFromUri(url.toURI()); final URLConnection urlConnection = UrlConnectionFactory.createConnection(url); boolean success = false; + InputStream openedStream = null; try { final ConfigurationSource source; if (file != null) { - source = new ConfigurationSource(urlConnection.getInputStream(), file); + openedStream = urlConnection.getInputStream(); + source = new ConfigurationSource(openedStream, file); } else if (urlConnection instanceof JarURLConnection) { // Work around https://bugs.openjdk.java.net/browse/JDK-6956385. URL jarFileUrl = ((JarURLConnection) urlConnection).getJarFileURL(); File jarFile = new File(jarFileUrl.getFile()); long lastModified = jarFile.lastModified(); - source = new ConfigurationSource(urlConnection.getInputStream(), url, lastModified); + openedStream = urlConnection.getInputStream(); + source = new ConfigurationSource(openedStream, url, lastModified); } else { - source = new ConfigurationSource( - urlConnection.getInputStream(), url, urlConnection.getLastModified()); + openedStream = urlConnection.getInputStream(); + source = new ConfigurationSource(openedStream, url, urlConnection.getLastModified()); } success = true; return source; @@ -382,10 +385,12 @@ public String toString() { return null; } finally { if (!success) { - try { - urlConnection.getInputStream().close(); - } catch (final IOException ignored) { - // Best-effort cleanup; the stream may not have been opened + if (openedStream != null) { + try { + openedStream.close(); + } catch (final IOException ignored) { + // Best-effort cleanup + } } if (urlConnection instanceof HttpURLConnection) { ((HttpURLConnection) urlConnection).disconnect(); From 4bfa56c1fede9b03a0fe58acfcf3f0afeb9ce1fb Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Thu, 28 May 2026 06:18:09 -0700 Subject: [PATCH 4/6] Add negative test for HttpURLConnection error-path cleanup Verifies that getConfigurationSource() properly disconnects the HttpURLConnection on failure (e.g., 404) and does not call getInputStream() a second time in the finally block. Uses a custom URLStreamHandler to inject a counting HttpURLConnection that throws FileNotFoundException, then asserts: - disconnect() is called (fails against original code with no finally) - getInputStream() is called exactly once (fails against an intermediate fix that re-invoked getInputStream() in the finally block) --- .../core/config/ConfigurationSourceTest.java | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java index b9300bb7af3..1d34031e4a3 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java @@ -25,15 +25,22 @@ import com.sun.management.UnixOperatingSystemMXBean; import java.io.ByteArrayInputStream; +import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.lang.management.ManagementFactory; import java.lang.management.OperatingSystemMXBean; +import java.lang.reflect.Method; +import java.net.HttpURLConnection; import java.net.URI; import java.net.URL; +import java.net.URLConnection; +import java.net.URLStreamHandler; import java.nio.file.Files; import java.nio.file.Path; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import java.util.jar.Attributes; import java.util.jar.JarEntry; import java.util.jar.JarOutputStream; @@ -123,6 +130,90 @@ void testJarFileLeakOnFailure() throws Exception { Files.delete(jarFile); } + /** + * Verifies that the error-path cleanup in {@code getConfigurationSource} + * properly disconnects an {@link HttpURLConnection} on failure without + * calling {@link URLConnection#getInputStream()} a second time. + * + *

This test fails against the original code (no cleanup) because + * {@code disconnect()} is never called, and fails against an intermediate + * fix that added a {@code finally} block calling {@code getInputStream()} + * again to obtain a stream for closing. + */ + @Test + void getConfigurationSource_disconnectsHttpConnection_onError() throws Exception { + final AtomicInteger getInputStreamCalls = new AtomicInteger(); + final AtomicBoolean disconnected = new AtomicBoolean(); + final AtomicInteger openConnectionCalls = new AtomicInteger(); + + final URLStreamHandler handler = new URLStreamHandler() { + @Override + protected URLConnection openConnection(final URL u) { + openConnectionCalls.incrementAndGet(); + return new HttpURLConnection(u) { + @Override + public InputStream getInputStream() throws IOException { + getInputStreamCalls.incrementAndGet(); + throw new FileNotFoundException("Mocked 404"); + } + + @Override + public void disconnect() { + disconnected.set(true); + } + + @Override + public boolean usingProxy() { + return false; + } + + @Override + public void connect() {} + }; + } + }; + + final URL url = new URL("http", "example.com", 80, "/missing.xml", handler); + + // Allow the "http" protocol for this test. + final String propKey = "log4j2.Configuration.allowedProtocols"; + final String previous = System.getProperty(propKey); + System.setProperty(propKey, "file, https, jar, http"); + try { + openConnectionCalls.set(0); + getInputStreamCalls.set(0); + + final Method method = + ConfigurationSource.class.getDeclaredMethod("getConfigurationSource", URL.class); + method.setAccessible(true); + final Object result = method.invoke(null, url); + + assertTrue( + openConnectionCalls.get() > 0, + "Custom URLStreamHandler was not used by UrlConnectionFactory"); + + assertNull(result, "Expected null return for a 404 response"); + + // Negative‐test #1: the original code has no finally block, so + // disconnect() is never called on the HttpURLConnection. + assertTrue(disconnected.get(), "disconnect() must be called on the error path"); + + // Negative‐test #2: an intermediate fix called getInputStream() + // a second time in the finally block instead of closing the + // already-tracked stream reference. + assertEquals( + 1, + getInputStreamCalls.get(), + "getInputStream() should be called exactly once"); + } finally { + if (previous != null) { + System.setProperty(propKey, previous); + } else { + System.clearProperty(propKey); + } + } + } + private long getOpenFileDescriptorCount() { final OperatingSystemMXBean os = ManagementFactory.getOperatingSystemMXBean(); if (os instanceof UnixOperatingSystemMXBean) { From 66b96e27281fdd8224204fec786ca8af4c4cab41 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Fri, 12 Jun 2026 04:41:30 -0700 Subject: [PATCH 5/6] Simplify error-path cleanup per review feedback Remove the success flag and openedStream tracking, which was dead code (openedStream is null on all error paths since getInputStream() throws before assignment). Split into two sequential try-catch blocks: the first resolves file and urlConnection with no cleanup needed, the second calls getInputStream() and disconnects HttpURLConnection only in the catch block. Move getLastModified() before getInputStream() in the generic URL branch so the stream is only opened after all failable calls succeed. Signed-off-by: Sebastien Tardif --- .../core/config/ConfigurationSourceTest.java | 10 --- .../core/config/ConfigurationSource.java | 68 ++++++++----------- 2 files changed, 29 insertions(+), 49 deletions(-) diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java index 1d34031e4a3..d65448eac5f 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java @@ -134,11 +134,6 @@ void testJarFileLeakOnFailure() throws Exception { * Verifies that the error-path cleanup in {@code getConfigurationSource} * properly disconnects an {@link HttpURLConnection} on failure without * calling {@link URLConnection#getInputStream()} a second time. - * - *

This test fails against the original code (no cleanup) because - * {@code disconnect()} is never called, and fails against an intermediate - * fix that added a {@code finally} block calling {@code getInputStream()} - * again to obtain a stream for closing. */ @Test void getConfigurationSource_disconnectsHttpConnection_onError() throws Exception { @@ -194,13 +189,8 @@ public void connect() {} assertNull(result, "Expected null return for a 404 response"); - // Negative‐test #1: the original code has no finally block, so - // disconnect() is never called on the HttpURLConnection. assertTrue(disconnected.get(), "disconnect() must be called on the error path"); - // Negative‐test #2: an intermediate fix called getInputStream() - // a second time in the finally block instead of closing the - // already-tracked stream reference. assertEquals( 1, getInputStreamCalls.get(), diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java index f8e61daa9a1..d2fdf0adf1a 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java @@ -357,50 +357,40 @@ public String toString() { value = "PATH_TRAVERSAL_IN", justification = "The name of the accessed files is based on a configuration value.") private static /*@Nullable*/ ConfigurationSource getConfigurationSource(final URL url) { + final File file; + final URLConnection urlConnection; try { - final File file = FileUtils.fileFromUri(url.toURI()); - final URLConnection urlConnection = UrlConnectionFactory.createConnection(url); - boolean success = false; - InputStream openedStream = null; - try { - final ConfigurationSource source; - if (file != null) { - openedStream = urlConnection.getInputStream(); - source = new ConfigurationSource(openedStream, file); - } else if (urlConnection instanceof JarURLConnection) { - // Work around https://bugs.openjdk.java.net/browse/JDK-6956385. - URL jarFileUrl = ((JarURLConnection) urlConnection).getJarFileURL(); - File jarFile = new File(jarFileUrl.getFile()); - long lastModified = jarFile.lastModified(); - openedStream = urlConnection.getInputStream(); - source = new ConfigurationSource(openedStream, url, lastModified); - } else { - openedStream = urlConnection.getInputStream(); - source = new ConfigurationSource(openedStream, url, urlConnection.getLastModified()); - } - success = true; - return source; - } catch (final FileNotFoundException ex) { - ConfigurationFactory.LOGGER.info("Unable to locate file {}, ignoring.", url.toString()); - return null; - } finally { - if (!success) { - if (openedStream != null) { - try { - openedStream.close(); - } catch (final IOException ignored) { - // Best-effort cleanup - } - } - if (urlConnection instanceof HttpURLConnection) { - ((HttpURLConnection) urlConnection).disconnect(); - } - } - } + file = FileUtils.fileFromUri(url.toURI()); + urlConnection = UrlConnectionFactory.createConnection(url); } catch (final IOException | URISyntaxException ex) { ConfigurationFactory.LOGGER.warn( "Error accessing {} due to {}, ignoring.", url.toString(), ex.getMessage()); return null; } + try { + if (file != null) { + return new ConfigurationSource(urlConnection.getInputStream(), file); + } else if (urlConnection instanceof JarURLConnection) { + // Work around https://bugs.openjdk.java.net/browse/JDK-6956385. + final URL jarFileUrl = ((JarURLConnection) urlConnection).getJarFileURL(); + final File jarFile = new File(jarFileUrl.getFile()); + final long lastModified = jarFile.lastModified(); + return new ConfigurationSource(urlConnection.getInputStream(), url, lastModified); + } else { + final long lastModified = urlConnection.getLastModified(); + return new ConfigurationSource(urlConnection.getInputStream(), url, lastModified); + } + } catch (final IOException ex) { + if (ex instanceof FileNotFoundException) { + ConfigurationFactory.LOGGER.info("Unable to locate file {}, ignoring.", url.toString()); + } else { + ConfigurationFactory.LOGGER.warn( + "Error accessing {} due to {}, ignoring.", url.toString(), ex.getMessage()); + } + if (urlConnection instanceof HttpURLConnection) { + ((HttpURLConnection) urlConnection).disconnect(); + } + return null; + } } } From ebab7a25675cdb432832b6f4cca53b2f02bd6b0a Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Fri, 12 Jun 2026 07:31:22 -0700 Subject: [PATCH 6/6] Fix Spotless formatting in ConfigurationSourceTest Signed-off-by: Sebastien Tardif --- .../log4j/core/config/ConfigurationSourceTest.java | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java index d65448eac5f..9df40fcc4e6 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java @@ -178,23 +178,17 @@ public void connect() {} openConnectionCalls.set(0); getInputStreamCalls.set(0); - final Method method = - ConfigurationSource.class.getDeclaredMethod("getConfigurationSource", URL.class); + final Method method = ConfigurationSource.class.getDeclaredMethod("getConfigurationSource", URL.class); method.setAccessible(true); final Object result = method.invoke(null, url); - assertTrue( - openConnectionCalls.get() > 0, - "Custom URLStreamHandler was not used by UrlConnectionFactory"); + assertTrue(openConnectionCalls.get() > 0, "Custom URLStreamHandler was not used by UrlConnectionFactory"); assertNull(result, "Expected null return for a 404 response"); assertTrue(disconnected.get(), "disconnect() must be called on the error path"); - assertEquals( - 1, - getInputStreamCalls.get(), - "getInputStream() should be called exactly once"); + assertEquals(1, getInputStreamCalls.get(), "getInputStream() should be called exactly once"); } finally { if (previous != null) { System.setProperty(propKey, previous);