This is an automated email from the ASF dual-hosted git repository. pkarwasz pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit 9fcb3c0153b4e4517eb7ee797234d3dfdee04ba1 Author: Piotr P. Karwasz <[email protected]> AuthorDate: Wed Mar 9 22:36:33 2022 +0100 [LOG4J2-3423] Refactoring of URLConnection code Refactors the code to minimize the number of calls to `URLConnection` methods. Conflicts: log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java log4j-core/src/test/java/org/apache/logging/log4j/core/config/ConfigurationSourceTest.java log4j-core/src/test/java/org/apache/logging/log4j/core/net/UrlConnectionFactoryTest.java log4j-core/src/test/java/org/apache/logging/log4j/core/util/NetUtilsTest.java --- .../org/apache/log4j/PropertyConfigurator.java | 5 +- .../java/org/apache/log4j/xml/DOMConfigurator.java | 5 +- .../log4j/core/config/ConfigurationSourceTest.java | 48 +++++++++++- .../log4j/core/net/UrlConnectionFactoryTest.java | 83 +++++++++++++++------ .../logging/log4j/core/util/NetUtilsTest.java | 18 ++++- .../log4j/core/config/ConfigurationFactory.java | 44 +++-------- .../log4j/core/config/ConfigurationSource.java | 10 +-- .../logging/log4j/core/config/HttpWatcher.java | 2 + .../log4j/core/net/UrlConnectionFactory.java | 10 +-- log4j-core/src/test/resources/jarfile.jar | Bin 0 -> 1122 bytes .../src/test/resources/jarfile/config/console.xml | 28 +++++++ 11 files changed, 175 insertions(+), 78 deletions(-) diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/PropertyConfigurator.java b/log4j-1.2-api/src/main/java/org/apache/log4j/PropertyConfigurator.java index 348646a..7f82b47 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/PropertyConfigurator.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/PropertyConfigurator.java @@ -47,6 +47,7 @@ import org.apache.log4j.spi.RendererSupport; import org.apache.log4j.spi.ThrowableRenderer; import org.apache.log4j.spi.ThrowableRendererSupport; import org.apache.logging.log4j.core.config.Configuration; +import org.apache.logging.log4j.core.net.UrlConnectionFactory; import org.apache.logging.log4j.util.StackLocatorUtil; /** @@ -381,9 +382,7 @@ public class PropertyConfigurator implements Configurator { Configuration doConfigure(final URL url, final LoggerRepository loggerRepository, final ClassLoader classLoader) { LogLog.debug("Reading configuration from URL " + url); try { - final URLConnection urlConnection = url.openConnection(); - // A "jar:" URL file remains open after the stream is closed, so do not cache it. - urlConnection.setUseCaches(false); + final URLConnection urlConnection = UrlConnectionFactory.createConnection(url); try (InputStream inputStream = urlConnection.getInputStream()) { return doConfigure(inputStream, loggerRepository, classLoader); } diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/xml/DOMConfigurator.java b/log4j-1.2-api/src/main/java/org/apache/log4j/xml/DOMConfigurator.java index e5adf60..b0cbb58 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/xml/DOMConfigurator.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/xml/DOMConfigurator.java @@ -39,6 +39,7 @@ import org.apache.logging.log4j.core.LoggerContext; import org.apache.logging.log4j.core.config.Configuration; import org.apache.logging.log4j.core.config.ConfigurationSource; import org.apache.logging.log4j.core.config.Configurator; +import org.apache.logging.log4j.core.net.UrlConnectionFactory; import org.apache.logging.log4j.core.util.IOUtils; import org.w3c.dom.Element; @@ -144,9 +145,7 @@ public class DOMConfigurator { public void doConfigure(final URL url, final LoggerRepository repository) { try { - final URLConnection connection = url.openConnection(); - // A "jar:" URL file remains open after the stream is closed, so do not cache it. - connection.setUseCaches(false); + final URLConnection connection = UrlConnectionFactory.createConnection(url); try (InputStream inputStream = connection.getInputStream()) { doConfigure(new ConfigurationSource(inputStream, url)); } 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 c067b71..9182e01 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 @@ -17,11 +17,23 @@ package org.apache.logging.log4j.core.config; -import org.junit.jupiter.api.Test; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.fail; import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.lang.management.ManagementFactory; +import java.lang.management.OperatingSystemMXBean; +import java.net.URL; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; -import static org.junit.jupiter.api.Assertions.assertNotNull; +import org.apache.logging.log4j.core.net.UrlConnectionFactory; +import org.junit.jupiter.api.Test; + +import com.sun.management.UnixOperatingSystemMXBean; public class ConfigurationSourceTest { @@ -30,4 +42,36 @@ public class ConfigurationSourceTest { ConfigurationSource configurationSource = new ConfigurationSource(new ByteArrayInputStream(new byte[] { 'a', 'b' })); assertNotNull(configurationSource.resetInputStream()); } + + /** + * Checks if the usage of 'jar:' URLs does not increase the file descriptor + * count and the jar file can be deleted. + * + * @throws Exception + */ + @Test + public void testNoJarFileLeak() throws Exception { + final Path original = Paths.get("target", "test-classes", "jarfile.jar"); + final Path copy = Paths.get("target", "test-classes", "jarfile-copy.jar"); + Files.copy(original, copy); + final URL jarUrl = new URL("jar:" + copy.toUri().toURL() + "!/config/console.xml"); + final long expected = getOpenFileDescriptorCount(); + UrlConnectionFactory.createConnection(jarUrl).getInputStream().close(); + // This can only fail on UNIX + assertEquals(expected, getOpenFileDescriptorCount()); + // This can only fail on Windows + try { + Files.delete(copy); + } catch (IOException e) { + fail(e); + } + } + + private long getOpenFileDescriptorCount() { + final OperatingSystemMXBean os = ManagementFactory.getOperatingSystemMXBean(); + if (os instanceof UnixOperatingSystemMXBean) { + return ((UnixOperatingSystemMXBean) os).getOpenFileDescriptorCount(); + } + return 0L; + } } diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/net/UrlConnectionFactoryTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/net/UrlConnectionFactoryTest.java index 07cd754..16f948e 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/net/UrlConnectionFactoryTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/net/UrlConnectionFactoryTest.java @@ -16,18 +16,33 @@ */ package org.apache.logging.log4j.core.net; -import javax.servlet.ServletException; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; +import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; +import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; +import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND; +import static javax.servlet.http.HttpServletResponse.SC_NOT_MODIFIED; +import static javax.servlet.http.HttpServletResponse.SC_OK; +import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + import java.io.File; import java.io.IOException; import java.io.InputStream; +import java.lang.management.ManagementFactory; +import java.lang.management.OperatingSystemMXBean; import java.net.HttpURLConnection; import java.net.URI; +import java.net.URL; import java.nio.file.Files; import java.util.Base64; import java.util.Enumeration; -import java.util.Properties; + +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -38,16 +53,11 @@ import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.servlet.DefaultServlet; import org.eclipse.jetty.servlet.ServletContextHandler; import org.eclipse.jetty.servlet.ServletHolder; - import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; +import com.sun.management.UnixOperatingSystemMXBean; /** * Tests the UrlConnectionFactory @@ -60,9 +70,6 @@ public class UrlConnectionFactoryTest { private static Server server; private static Base64.Decoder decoder = Base64.getDecoder(); private static int port; - private static final int NOT_MODIFIED = 304; - private static final int NOT_AUTHORIZED = 401; - private static final int OK = 200; private static final int BUF_SIZE = 1024; @BeforeAll @@ -110,15 +117,16 @@ public class UrlConnectionFactoryTest { assertNotNull(source, "No ConfigurationSource returned"); InputStream is = source.getInputStream(); assertNotNull(is, "No data returned"); + is.close(); long lastModified = source.getLastModified(); int result = verifyNotModified(uri, lastModified); - assertEquals(NOT_MODIFIED, result,"File was modified"); + assertEquals(SC_NOT_MODIFIED, result,"File was modified"); File file = new File("target/test-classes/log4j2-config.xml"); if (!file.setLastModified(System.currentTimeMillis())) { fail("Unable to set last modified time"); } result = verifyNotModified(uri, lastModified); - assertEquals(OK, result,"File was not modified"); + assertEquals(SC_OK, result,"File was not modified"); } private int verifyNotModified(URI uri, long lastModifiedMillis) throws Exception { @@ -130,10 +138,34 @@ public class UrlConnectionFactoryTest { return urlConnection.getResponseCode(); } catch (final IOException ioe) { LOGGER.error("Error accessing configuration at {}: {}", uri, ioe.getMessage()); - return 500; + return SC_INTERNAL_SERVER_ERROR; } } + @Test + public void testNoJarFileLeak() throws Exception { + final URL url = new File("target/test-classes/jarfile.jar").toURI().toURL(); + // Retrieve using 'file:' + URL jarUrl = new URL("jar:" + url.toString() + "!/config/console.xml"); + long expected = getOpenFileDescriptorCount(); + UrlConnectionFactory.createConnection(jarUrl).getInputStream().close(); + assertEquals(expected, getOpenFileDescriptorCount()); + // Retrieve using 'http:' + jarUrl = new URL("jar:http://localhost:"; + port + "/jarfile.jar!/config/console.xml"); + final File tmpDir = new File(System.getProperty("java.io.tmpdir")); + expected = tmpDir.list().length; + UrlConnectionFactory.createConnection(jarUrl).getInputStream().close(); + assertEquals(expected, tmpDir.list().length, "File descriptor leak"); + } + + private long getOpenFileDescriptorCount() { + final OperatingSystemMXBean os = ManagementFactory.getOperatingSystemMXBean(); + if (os instanceof UnixOperatingSystemMXBean) { + return ((UnixOperatingSystemMXBean) os).getOpenFileDescriptorCount(); + } + return 0L; + } + public static class TestServlet extends DefaultServlet { private static final long serialVersionUID = -2885158530511450659L; @@ -143,7 +175,7 @@ public class UrlConnectionFactoryTest { HttpServletResponse response) throws ServletException, IOException { Enumeration<String> headers = request.getHeaders(HttpHeader.AUTHORIZATION.toString()); if (headers == null) { - response.sendError(401, "No Auth header"); + response.sendError(SC_UNAUTHORIZED, "No Auth header"); return; } while (headers.hasMoreElements()) { @@ -151,26 +183,31 @@ public class UrlConnectionFactoryTest { assertTrue(authData.startsWith(BASIC), "Not a Basic auth header"); String credentials = new String(decoder.decode(authData.substring(BASIC.length()))); if (!expectedCreds.equals(credentials)) { - response.sendError(401, "Invalid credentials"); + response.sendError(SC_UNAUTHORIZED, "Invalid credentials"); return; } } - if (request.getServletPath().equals("/log4j2-config.xml")) { - File file = new File("target/test-classes/log4j2-config.xml"); + final String servletPath = request.getServletPath(); + if (servletPath != null) { + final File file = new File("target/test-classes" + servletPath); + if (!file.exists()) { + response.sendError(SC_NOT_FOUND); + return; + } long modifiedSince = request.getDateHeader(HttpHeader.IF_MODIFIED_SINCE.toString()); long lastModified = (file.lastModified() / 1000) * 1000; LOGGER.debug("LastModified: {}, modifiedSince: {}", lastModified, modifiedSince); if (modifiedSince > 0 && lastModified <= modifiedSince) { - response.setStatus(304); + response.setStatus(SC_NOT_MODIFIED); return; } response.setDateHeader(HttpHeader.LAST_MODIFIED.toString(), lastModified); response.setContentLengthLong(file.length()); Files.copy(file.toPath(), response.getOutputStream()); response.getOutputStream().flush(); - response.setStatus(200); + response.setStatus(SC_OK); } else { - response.sendError(400, "Unsupported request"); + response.sendError(SC_BAD_REQUEST, "Unsupported request"); } } } diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/NetUtilsTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/NetUtilsTest.java index c7cddc6..c0f7ffa 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/NetUtilsTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/NetUtilsTest.java @@ -21,6 +21,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.condition.EnabledOnOs; import org.junit.jupiter.api.condition.OS; +import java.io.File; import java.net.URI; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -31,10 +32,25 @@ public class NetUtilsTest { @Test public void testToUriWithoutBackslashes() { final String config = "file:///path/to/something/on/unix"; - final URI uri = NetUtils.toURI(config); + URI uri = NetUtils.toURI(config); assertNotNull(uri, "The URI should not be null."); assertEquals("file:///path/to/something/on/unix", uri.toString(), "The URI is not correct."); + + final String properUriPath = "/path/without/spaces"; + uri = NetUtils.toURI(properUriPath); + + assertNotNull(uri, "The URI should not be null."); + assertEquals(properUriPath, uri.toString(), "The URI is not correct."); + } + + @Test + public void testToUriUnixWithSpaces() { + final String pathWithSpaces = "/ path / with / spaces"; + final URI uri = NetUtils.toURI(pathWithSpaces); + + assertNotNull(uri, "The URI should not be null."); + assertEquals(new File(pathWithSpaces).toURI().toString(), uri.toString(), "The URI is not correct."); } @Test diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java index fac845f..8473e25 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java @@ -16,12 +16,23 @@ */ package org.apache.logging.log4j.core.config; +import java.io.UnsupportedEncodingException; +import java.net.URI; +import java.net.URISyntaxException; +import java.net.URLDecoder; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; + import org.apache.logging.log4j.Level; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.core.LoggerContext; import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFactory; import org.apache.logging.log4j.core.lookup.StrSubstitutor; -import org.apache.logging.log4j.core.net.UrlConnectionFactory; import org.apache.logging.log4j.core.util.AuthorizationProvider; import org.apache.logging.log4j.core.util.BasicAuthorizationProvider; import org.apache.logging.log4j.core.util.FileUtils; @@ -235,35 +246,4 @@ public abstract class ConfigurationFactory extends ConfigurationBuilderFactory { return uri.getScheme() == null ? uri.getPath() : uri.getSchemeSpecificPart(); } - /** - * Loads the configuration from the location represented by the String. - * @param config The configuration location. - * @param loader The default ClassLoader to use. - * @return The InputSource to use to read the configuration. - */ - protected ConfigurationSource getInputFromString(final String config, final ClassLoader loader) { - try { - final URL url = new URL(config); - final URLConnection urlConnection = UrlConnectionFactory.createConnection(url); - final File file = FileUtils.fileFromUri(url.toURI()); - if (file != null) { - return new ConfigurationSource(urlConnection.getInputStream(), FileUtils.fileFromUri(url.toURI())); - } else { - return new ConfigurationSource(urlConnection.getInputStream(), url, urlConnection.getLastModified()); - } - } catch (final Exception ex) { - final ConfigurationSource source = ConfigurationSource.fromResource(config, loader); - if (source == null) { - try { - final File file = new File(config); - return new ConfigurationSource(new FileInputStream(file), file); - } catch (final FileNotFoundException fnfe) { - // Ignore the exception - LOGGER.catching(Level.DEBUG, fnfe); - } - } - return source; - } - } - } 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 bb37919..24f7961 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 @@ -332,17 +332,9 @@ public class ConfigurationSource { return null; } try { - final URL url = configLocation.toURL(); - final URLConnection urlConnection = UrlConnectionFactory.createConnection(url); - final InputStream is = urlConnection.getInputStream(); - final long lastModified = urlConnection.getLastModified(); - return new ConfigurationSource(is, configLocation.toURL(), lastModified); - } catch (final FileNotFoundException ex) { - ConfigurationFactory.LOGGER.warn("Could not locate file {}", configLocation.toString()); + return getConfigurationSource(configLocation.toURL()); } catch (final MalformedURLException ex) { ConfigurationFactory.LOGGER.error("Invalid URL {}", configLocation.toString(), ex); - } catch (final Exception ex) { - ConfigurationFactory.LOGGER.error("Unable to access {}", configLocation.toString(), ex); } return null; } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/HttpWatcher.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/HttpWatcher.java index b781a4c..f6eb7e2 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/HttpWatcher.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/HttpWatcher.java @@ -135,6 +135,8 @@ public class HttpWatcher extends AbstractWatcher { } } catch (final IOException ioe) { LOGGER.error("Error accessing configuration at {}: {}", url, ioe.getMessage()); + } finally { + urlConnection.disconnect(); } } catch (final IOException ioe) { LOGGER.error("Error connecting to configuration at {}: {}", url, ioe.getMessage()); diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/UrlConnectionFactory.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/UrlConnectionFactory.java index 65d4a13..fc72141 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/UrlConnectionFactory.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/UrlConnectionFactory.java @@ -18,6 +18,7 @@ package org.apache.logging.log4j.core.net; import java.io.IOException; import java.net.HttpURLConnection; +import java.net.JarURLConnection; import java.net.ProtocolException; import java.net.URL; import java.net.URLConnection; @@ -77,8 +78,6 @@ public class UrlConnectionFactory { urlConnection.setDoOutput(true); urlConnection.setDoInput(true); urlConnection.setRequestMethod("GET"); - // A "jar:" URL file remains open after the stream is closed, so do not cache it. - urlConnection.setUseCaches(false); if (connectTimeoutMillis > 0) { urlConnection.setConnectTimeout(connectTimeoutMillis); } @@ -107,13 +106,14 @@ public class UrlConnectionFactory { urlConnection = createConnection(url, 0, SslConfigurationFactory.getSslConfiguration()); } else { urlConnection = url.openConnection(); - // A "jar:" URL file remains open after the stream is closed, so do not cache it. - urlConnection.setUseCaches(false); + if (urlConnection instanceof JarURLConnection) { + // A "jar:" URL file remains open after the stream is closed, so do not cache it. + urlConnection.setUseCaches(false); + } } return urlConnection; } - private static boolean isXml(final String type) { return type.equalsIgnoreCase("xml"); } diff --git a/log4j-core/src/test/resources/jarfile.jar b/log4j-core/src/test/resources/jarfile.jar new file mode 100644 index 0000000..bf34b8c Binary files /dev/null and b/log4j-core/src/test/resources/jarfile.jar differ diff --git a/log4j-core/src/test/resources/jarfile/config/console.xml b/log4j-core/src/test/resources/jarfile/config/console.xml new file mode 100644 index 0000000..015ac0b --- /dev/null +++ b/log4j-core/src/test/resources/jarfile/config/console.xml @@ -0,0 +1,28 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + +--> +<Configuration status="WARN"> + <Appenders> + <Console name="Console" /> + </Appenders> + <Loggers> + <Root level="TRACE"> + <AppenderRef ref="Console" /> + </Root> + </Loggers> +</Configuration>
