diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/jmx/LoggerContextAdminSetConfigLocationUriTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/jmx/LoggerContextAdminSetConfigLocationUriTest.java new file mode 100644 index 00000000000..01fb5811d1f --- /dev/null +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/jmx/LoggerContextAdminSetConfigLocationUriTest.java @@ -0,0 +1,134 @@ +/* + * 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. + */ +package org.apache.logging.log4j.core.jmx; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.concurrent.atomic.AtomicReference; +import org.apache.logging.log4j.core.LoggerContext; +import org.apache.logging.log4j.core.config.Configuration; +import org.apache.logging.log4j.core.config.ConfigurationFactory; +import org.apache.logging.log4j.core.config.ConfigurationSource; +import org.apache.logging.log4j.core.config.DefaultConfiguration; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** + * Regression for caller-owned stream cleanup in + * {@link LoggerContextAdmin#setConfigLocationUri(String)}. + * + *

{@code ConfigurationSource(InputStream, File/URL)} leaves stream ownership + * with the caller. Built-in factories close {@code getInputStream()} when they + * consume it, but that is not a substitute for caller-side try-with-resources + * when a factory path never reads the stream. + */ +class LoggerContextAdminSetConfigLocationUriTest { + + @TempDir + Path tempDir; + + @AfterEach + void resetConfigurationFactory() { + ConfigurationFactory.resetConfigurationFactory(); + } + + @Test + void setConfigLocationUri_loadsValidFileAndReconfigures() throws Exception { + final Path config = tempDir.resolve("log4j2-test.xml"); + writeConfig(config, "C"); + + final LoggerContext ctx = new LoggerContext("jmx-admin-stream-test"); + final LoggerContextAdmin admin = new LoggerContextAdmin(ctx, Runnable::run); + + assertDoesNotThrow( + () -> admin.setConfigLocationUri(config.toAbsolutePath().toString())); + assertTrue(ctx.getConfiguration().getAppenders().containsKey("C")); + } + + @Test + void setConfigLocationUri_fileUrlFormAlsoLoads() throws Exception { + final Path config = tempDir.resolve("log4j2-url.xml"); + writeConfig(config, "FromUrl"); + + final LoggerContext ctx = new LoggerContext("jmx-admin-file-url-test"); + final LoggerContextAdmin admin = new LoggerContextAdmin(ctx, Runnable::run); + final String fileUrl = config.toUri().toURL().toString(); + assertDoesNotThrow(() -> admin.setConfigLocationUri(fileUrl)); + assertTrue(ctx.getConfiguration().getAppenders().containsKey("FromUrl")); + } + + @Test + void setConfigLocationUri_rejectsBlankLocation() { + final LoggerContext ctx = new LoggerContext("jmx-admin-blank"); + final LoggerContextAdmin admin = new LoggerContextAdmin(ctx, Runnable::run); + assertThrows(IllegalArgumentException.class, () -> admin.setConfigLocationUri("")); + assertThrows(IllegalArgumentException.class, () -> admin.setConfigLocationUri(null)); + } + + /** + * Portable red-green for caller try-with-resources: install a factory that + * returns without consuming the source stream, capture that stream, and + * assert it is closed after {@code setConfigLocationUri} returns. + */ + @Test + void setConfigLocationUri_closesCallerOwnedStreamWhenFactoryDoesNotConsumeIt() throws Exception { + final Path config = tempDir.resolve("log4j2-unconsumed.xml"); + writeConfig(config, "Unconsumed"); + + final AtomicReference input = new AtomicReference<>(); + ConfigurationFactory.setConfigurationFactory(new ConfigurationFactory() { + @Override + public Configuration getConfiguration(final LoggerContext loggerContext, final ConfigurationSource source) { + // Capture the caller-owned stream without consuming or closing it. + input.set(source.getInputStream()); + return new DefaultConfiguration(); + } + + @Override + protected String[] getSupportedTypes() { + return new String[] {"*"}; + } + }); + + final LoggerContext ctx = new LoggerContext("jmx-admin-stream-close"); + final LoggerContextAdmin admin = new LoggerContextAdmin(ctx, Runnable::run); + admin.setConfigLocationUri(config.toAbsolutePath().toString()); + + final InputStream captured = input.get(); + assertNotNull(captured); + // Closed streams throw on read; an unclosed stream would still read. + assertThrows(IOException.class, captured::read); + } + + private static void writeConfig(final Path config, final String appenderName) throws Exception { + final String xml = "\n" + + "\n" + + " \n" + + " \n" + + "\n"; + Files.write(config, xml.getBytes(StandardCharsets.UTF_8)); + } +} diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/jmx/LoggerContextAdmin.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/jmx/LoggerContextAdmin.java index d8927d6737d..10223843a80 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/jmx/LoggerContextAdmin.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/jmx/LoggerContextAdmin.java @@ -20,7 +20,6 @@ import java.beans.PropertyChangeListener; import java.io.ByteArrayInputStream; import java.io.File; -import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; @@ -31,6 +30,7 @@ import java.net.URL; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.util.Map; import java.util.Objects; import java.util.concurrent.Executor; @@ -128,17 +128,29 @@ public void setConfigLocationUri(final String configLocation) throws URISyntaxEx LOGGER.debug("---------"); LOGGER.debug("Remote request to reconfigure using location " + configLocation); final File configFile = new File(configLocation); - ConfigurationSource configSource = null; + // ConfigurationSource(InputStream, File/URL) documents that the caller owns the stream. + // XmlConfiguration, JsonConfiguration, and PropertiesConfigurationFactory close + // getInputStream() when they consume it, but that is factory-side cleanup. Keep a + // stream-backed source (so resetInputStream/reconfigure still re-reads the file for + // monitorInterval) and always close the caller-owned stream via try-with-resources. if (configFile.exists()) { LOGGER.debug("Opening config file {}", configFile.getAbsolutePath()); - configSource = new ConfigurationSource(new FileInputStream(configFile), configFile); + try (final InputStream in = Files.newInputStream(configFile.toPath())) { + final ConfigurationSource configSource = new ConfigurationSource(in, configFile); + final Configuration config = + ConfigurationFactory.getInstance().getConfiguration(loggerContext, configSource); + loggerContext.start(config); + } } else { final URL configURL = new URL(configLocation); LOGGER.debug("Opening config URL {}", configURL); - configSource = new ConfigurationSource(configURL.openStream(), configURL); + try (final InputStream in = configURL.openStream()) { + final ConfigurationSource configSource = new ConfigurationSource(in, configURL); + final Configuration config = + ConfigurationFactory.getInstance().getConfiguration(loggerContext, configSource); + loggerContext.start(config); + } } - final Configuration config = ConfigurationFactory.getInstance().getConfiguration(loggerContext, configSource); - loggerContext.start(config); LOGGER.debug("Completed remote request to reconfigure."); } diff --git a/src/changelog/.2.x.x/fix_logger_context_admin_config_stream_leak.xml b/src/changelog/.2.x.x/fix_logger_context_admin_config_stream_leak.xml new file mode 100644 index 00000000000..091e6a5087c --- /dev/null +++ b/src/changelog/.2.x.x/fix_logger_context_admin_config_stream_leak.xml @@ -0,0 +1,10 @@ + + + + + Fix file descriptor leak in the JMX integration + +