Skip to content

Fix resource leaks in ConfigurationSource when loading via URL#4127

Open
SebTardif wants to merge 3 commits into
apache:2.xfrom
SebTardif:fix-urlconnection-leak
Open

Fix resource leaks in ConfigurationSource when loading via URL#4127
SebTardif wants to merge 3 commits into
apache:2.xfrom
SebTardif:fix-urlconnection-leak

Conversation

@SebTardif
Copy link
Copy Markdown
Contributor

@SebTardif SebTardif commented May 21, 2026

What this PR does

In log4j-core/.../config/ConfigurationSource.java, method getConfigurationSource(URL): url.openConnection() creates a URLConnection, but on both error paths (FileNotFoundException, IOException/URISyntaxException) the connection is never closed or disconnected.

For HttpURLConnection this leaks a socket. For JarURLConnection this leaks a file handle (relevant to locked JARs on Tomcat redeploy scenarios).

Fix:

  • Added a boolean success flag with try-finally pattern to disconnect/close on error paths
  • Reused the existing file variable instead of calling FileUtils.fileFromUri(url.toURI()) a second time

@ramanathan1504 ramanathan1504 changed the title Close URLConnection on error paths in ConfigurationSource Fix resource leaks in ConfigurationSource when loading via URL May 21, 2026
@ramanathan1504 ramanathan1504 added the configuration Affects the configuration system in a general way label May 21, 2026
@ramanathan1504
Copy link
Copy Markdown
Contributor

@SebTardif

"Great work on this! Resource leaks in configuration loading can be incredibly difficult to debug, especially in containerized or high-availability environments. This is a solid improvement to Log4j's stability.

Suggestions for Completion:

To make this PR "merge-ready" for the standards, we should add a failure test case and a changelog entry:

1. Add a Failure Test Case
Since the fix specifically targets the failure path (!success), we should add a test that intentionally fails to find a file inside a JAR to ensure the handle is released.

Add this to ConfigurationSourceTest.java:

@Test
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 long expectedFdCount = getOpenFileDescriptorCount();
    
    // This triggers the FileNotFoundException internally
    ConfigurationSource configSource = ConfigurationSource.fromUri(brokenJarConfigURL.toURI());
    
    assertNull(configSource, "Source should be null on failure");

    // 1. Verify File Descriptors (Unix/Linux)
    assertEquals(expectedFdCount, getOpenFileDescriptorCount(), 
        "File descriptors leaked after failed configuration load!");

    // 2. Verify File Lock (Windows)
    try {
        Files.delete(jarFile);
    } catch (IOException e) {
        fail("Could not delete JAR file! A handle was leaked in the failure path.");
    }
}

2. Add a Changelog Entry
Log4j 2 uses an automated tool for release notes. Please create a new file: src/changelog/.2.x.x/fix_ConfigurationSource_leak.xml :

<?xml version="1.0" encoding="UTF-8"?>
<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
       xmlns="https://logging.apache.org/xml/ns"
       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
       type="fixed">
  <issue id="4127" link="https://github.com/apache/logging-log4j2/pull/4127"/>
  <description format="markdown">
     Fix resource leaks in ConfigurationSource when loading via URL
  </description>
</entry>

Thanks again for the contribution! don't forgot to apply spotless !!😊

@ramanathan1504 ramanathan1504 added the java Pull requests that update Java code label May 21, 2026
SebTardif added 2 commits May 21, 2026 13:18
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.
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.
@SebTardif SebTardif force-pushed the fix-urlconnection-leak branch from 7cc7f49 to 3fde88b Compare May 21, 2026 20:31
@SebTardif
Copy link
Copy Markdown
Contributor Author

Thanks for the detailed suggestions @ramanathan1504! Added both:

  • Failure test: testNoJarFileLeakOnFailure — creates a JAR, requests a missing entry (!/missing/file.xml), and asserts no FD leak and no file lock after the failed load. Follows the same pattern as the existing testNoJarFileLeak and testLoadConfigurationSourceFromJarFile tests.

  • Changelog entry: src/changelog/.2.x.x/fix_configuration_source_url_leak.xml

Spotless applied. All 4 ConfigurationSourceTest tests pass.

@ramanathan1504
Copy link
Copy Markdown
Contributor

@SebTardif

I pushed a follow-up commit to address this in the PR branch.

What I changed:

  • Updated ConfigurationSource to close only the already-opened InputStream on failure paths, so we avoid potential URL/JAR resource leaks.
  • Updated ConfigurationSourceTest to validate the failure case (missing JAR entry), confirm no file descriptor increase, and ensure the JAR is deletable afterward.

Please avoid force-pushing in future updates so review history and inline comments remain easy to track.

Thanks for the support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

configuration Affects the configuration system in a general way java Pull requests that update Java code

Projects

Development

Successfully merging this pull request may close these issues.

2 participants