Skip to content

Commit f10c986

Browse files
SebTardiframanathan1504ppkarwaszvy
authored
Fix resource leaks in ConfigurationSource when loading via URL (#4127)
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca> Co-authored-by: Ramanathan <ramanathanbscmca@gmail.com> Co-authored-by: Piotr P. Karwasz <pkarwasz-github@apache.org> Co-authored-by: Volkan Yazıcı <volkan@yazi.ci>
1 parent 823d53d commit f10c986

3 files changed

Lines changed: 133 additions & 20 deletions

File tree

‎log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,22 @@
2525

2626
import com.sun.management.UnixOperatingSystemMXBean;
2727
import java.io.ByteArrayInputStream;
28+
import java.io.FileNotFoundException;
2829
import java.io.IOException;
2930
import java.io.InputStream;
3031
import java.io.OutputStream;
3132
import java.lang.management.ManagementFactory;
3233
import java.lang.management.OperatingSystemMXBean;
34+
import java.lang.reflect.Method;
35+
import java.net.HttpURLConnection;
36+
import java.net.URI;
3337
import java.net.URL;
38+
import java.net.URLConnection;
39+
import java.net.URLStreamHandler;
3440
import java.nio.file.Files;
3541
import java.nio.file.Path;
42+
import java.util.concurrent.atomic.AtomicBoolean;
43+
import java.util.concurrent.atomic.AtomicInteger;
3644
import java.util.jar.Attributes;
3745
import java.util.jar.JarEntry;
3846
import java.util.jar.JarOutputStream;
@@ -104,6 +112,92 @@ void testLoadConfigurationSourceFromJarFile() throws Exception {
104112
}
105113
}
106114

115+
/**
116+
* Verifies that a failed JAR configuration load (missing entry) does not leak file descriptors
117+
* or hold a lock on the JAR file.
118+
*/
119+
@Test
120+
void testJarFileLeakOnFailure() throws Exception {
121+
final Path jarFile = prepareJarConfigURL(tempDir);
122+
final String jarUriStr = "jar:" + jarFile.toUri().toASCIIString() + "!/" + java.util.UUID.randomUUID();
123+
final URI jarUri = URI.create(jarUriStr);
124+
125+
final long expectedFdCount = getOpenFileDescriptorCount();
126+
127+
assertNull(ConfigurationSource.fromUri(jarUri));
128+
129+
assertEquals(expectedFdCount, getOpenFileDescriptorCount());
130+
Files.delete(jarFile);
131+
}
132+
133+
/**
134+
* Verifies that the error-path cleanup in {@code getConfigurationSource}
135+
* properly disconnects an {@link HttpURLConnection} on failure without
136+
* calling {@link URLConnection#getInputStream()} a second time.
137+
*/
138+
@Test
139+
void getConfigurationSource_disconnectsHttpConnection_onError() throws Exception {
140+
final AtomicInteger getInputStreamCalls = new AtomicInteger();
141+
final AtomicBoolean disconnected = new AtomicBoolean();
142+
final AtomicInteger openConnectionCalls = new AtomicInteger();
143+
144+
final URLStreamHandler handler = new URLStreamHandler() {
145+
@Override
146+
protected URLConnection openConnection(final URL u) {
147+
openConnectionCalls.incrementAndGet();
148+
return new HttpURLConnection(u) {
149+
@Override
150+
public InputStream getInputStream() throws IOException {
151+
getInputStreamCalls.incrementAndGet();
152+
throw new FileNotFoundException("Mocked 404");
153+
}
154+
155+
@Override
156+
public void disconnect() {
157+
disconnected.set(true);
158+
}
159+
160+
@Override
161+
public boolean usingProxy() {
162+
return false;
163+
}
164+
165+
@Override
166+
public void connect() {}
167+
};
168+
}
169+
};
170+
171+
final URL url = new URL("http", "example.com", 80, "/missing.xml", handler);
172+
173+
// Allow the "http" protocol for this test.
174+
final String propKey = "log4j2.Configuration.allowedProtocols";
175+
final String previous = System.getProperty(propKey);
176+
System.setProperty(propKey, "file, https, jar, http");
177+
try {
178+
openConnectionCalls.set(0);
179+
getInputStreamCalls.set(0);
180+
181+
final Method method = ConfigurationSource.class.getDeclaredMethod("getConfigurationSource", URL.class);
182+
method.setAccessible(true);
183+
final Object result = method.invoke(null, url);
184+
185+
assertTrue(openConnectionCalls.get() > 0, "Custom URLStreamHandler was not used by UrlConnectionFactory");
186+
187+
assertNull(result, "Expected null return for a 404 response");
188+
189+
assertTrue(disconnected.get(), "disconnect() must be called on the error path");
190+
191+
assertEquals(1, getInputStreamCalls.get(), "getInputStream() should be called exactly once");
192+
} finally {
193+
if (previous != null) {
194+
System.setProperty(propKey, previous);
195+
} else {
196+
System.clearProperty(propKey);
197+
}
198+
}
199+
}
200+
107201
private long getOpenFileDescriptorCount() {
108202
final OperatingSystemMXBean os = ManagementFactory.getOperatingSystemMXBean();
109203
if (os instanceof UnixOperatingSystemMXBean) {

‎log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java‎

Lines changed: 31 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.io.FileNotFoundException;
2424
import java.io.IOException;
2525
import java.io.InputStream;
26+
import java.net.HttpURLConnection;
2627
import java.net.JarURLConnection;
2728
import java.net.MalformedURLException;
2829
import java.net.URI;
@@ -356,30 +357,40 @@ public String toString() {
356357
value = "PATH_TRAVERSAL_IN",
357358
justification = "The name of the accessed files is based on a configuration value.")
358359
private static /*@Nullable*/ ConfigurationSource getConfigurationSource(final URL url) {
360+
final File file;
361+
final URLConnection urlConnection;
359362
try {
360-
final File file = FileUtils.fileFromUri(url.toURI());
361-
final URLConnection urlConnection = UrlConnectionFactory.createConnection(url);
362-
try {
363-
if (file != null) {
364-
return new ConfigurationSource(urlConnection.getInputStream(), FileUtils.fileFromUri(url.toURI()));
365-
} else if (urlConnection instanceof JarURLConnection) {
366-
// Work around https://bugs.openjdk.java.net/browse/JDK-6956385.
367-
URL jarFileUrl = ((JarURLConnection) urlConnection).getJarFileURL();
368-
File jarFile = new File(jarFileUrl.getFile());
369-
long lastModified = jarFile.lastModified();
370-
return new ConfigurationSource(urlConnection.getInputStream(), url, lastModified);
371-
} else {
372-
return new ConfigurationSource(
373-
urlConnection.getInputStream(), url, urlConnection.getLastModified());
374-
}
375-
} catch (FileNotFoundException ex) {
376-
ConfigurationFactory.LOGGER.info("Unable to locate file {}, ignoring.", url.toString());
377-
return null;
378-
}
379-
} catch (IOException | URISyntaxException ex) {
363+
file = FileUtils.fileFromUri(url.toURI());
364+
urlConnection = UrlConnectionFactory.createConnection(url);
365+
} catch (final IOException | URISyntaxException ex) {
380366
ConfigurationFactory.LOGGER.warn(
381367
"Error accessing {} due to {}, ignoring.", url.toString(), ex.getMessage());
382368
return null;
383369
}
370+
try {
371+
if (file != null) {
372+
return new ConfigurationSource(urlConnection.getInputStream(), file);
373+
} else if (urlConnection instanceof JarURLConnection) {
374+
// Work around https://bugs.openjdk.java.net/browse/JDK-6956385.
375+
final URL jarFileUrl = ((JarURLConnection) urlConnection).getJarFileURL();
376+
final File jarFile = new File(jarFileUrl.getFile());
377+
final long lastModified = jarFile.lastModified();
378+
return new ConfigurationSource(urlConnection.getInputStream(), url, lastModified);
379+
} else {
380+
final long lastModified = urlConnection.getLastModified();
381+
return new ConfigurationSource(urlConnection.getInputStream(), url, lastModified);
382+
}
383+
} catch (final IOException ex) {
384+
if (ex instanceof FileNotFoundException) {
385+
ConfigurationFactory.LOGGER.info("Unable to locate file {}, ignoring.", url.toString());
386+
} else {
387+
ConfigurationFactory.LOGGER.warn(
388+
"Error accessing {} due to {}, ignoring.", url.toString(), ex.getMessage());
389+
}
390+
if (urlConnection instanceof HttpURLConnection) {
391+
((HttpURLConnection) urlConnection).disconnect();
392+
}
393+
return null;
394+
}
384395
}
385396
}

‎src/changelog/.2.x.x/fix_configuration_source_url_leak.xml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
3+
xmlns="https://logging.apache.org/xml/ns"
4+
xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
5+
type="fixed">
6+
<issue id="4127" link="https://github.com/apache/logging-log4j2/pull/4127"/>
7+
<description format="asciidoc">Fix resource leaks in `ConfigurationSource` when loading configuration via URL fails</description>
8+
</entry>

0 commit comments

Comments
 (0)