This is an automated email from the ASF dual-hosted git repository. markt-asf pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit cc4f280ba0c89bf2436e25e772996a8fe9407d45 Author: Mark Thomas <[email protected]> AuthorDate: Tue Nov 3 18:37:23 2026 +0000 Review "Close the descriptor stream in the digester modeler source" Apply a broader fix that catches more error cases Co-authored by: Claude / Sonnet 5.5 --- .../tomcat/util/modeler/LocalStrings.properties | 1 + java/org/apache/tomcat/util/modeler/Registry.java | 32 ++++++-- .../modules/MbeansDescriptorsDigesterSource.java | 4 +- .../TestMbeansDescriptorsDigesterSource.java | 87 ++++++++++++++++++++++ 4 files changed, 117 insertions(+), 7 deletions(-) diff --git a/java/org/apache/tomcat/util/modeler/LocalStrings.properties b/java/org/apache/tomcat/util/modeler/LocalStrings.properties index 3a8916fca5..9d679f3063 100644 --- a/java/org/apache/tomcat/util/modeler/LocalStrings.properties +++ b/java/org/apache/tomcat/util/modeler/LocalStrings.properties @@ -40,6 +40,7 @@ modules.readDescriptorsError=Error reading descriptors noJmxMBeanServer.mBeanNotFound=The MBean [{0}] was not found. JMX support is disabled. registry.cannotAccessRegistry=Guard object [{0}] does not allow access to the registry +registry.closeSourceError=Failed to close the source of the descriptors registry.createdServer=Created MBeanServer registry.existingServer=Using existing MBeanServer registry.initError=Error initializing [{0}] diff --git a/java/org/apache/tomcat/util/modeler/Registry.java b/java/org/apache/tomcat/util/modeler/Registry.java index 04bf1f1447..8686645f44 100644 --- a/java/org/apache/tomcat/util/modeler/Registry.java +++ b/java/org/apache/tomcat/util/modeler/Registry.java @@ -18,6 +18,7 @@ package org.apache.tomcat.util.modeler; import java.io.File; import java.io.FileInputStream; +import java.io.IOException; import java.io.InputStream; import java.lang.management.ManagementFactory; import java.net.URL; @@ -574,6 +575,12 @@ public class Registry implements RegistryMBean, MBeanRegistration { /** * Load descriptors. * + * <p> + * If the source is a {@link URL} or a {@link File}, the stream opened to read it is closed before this method + * returns. If the source is an {@link InputStream}, that stream is closed before this method returns as well. In + * both cases the stream is closed whether or not loading succeeds, so the caller must not use a stream passed to + * this method after the call. + * * @param sourceType The source type * @param source The bean * @param param A type to load @@ -624,12 +631,27 @@ public class Registry implements RegistryMBean, MBeanRegistration { case null, default -> throw new IllegalArgumentException(sm.getString("registry.invalidSource")); } - if (sourceType == null) { - sourceType = "MbeansDescriptorsDigesterSource"; + try { + if (sourceType == null) { + sourceType = "MbeansDescriptorsDigesterSource"; + } + ModelerSource ds = getModelerSource(sourceType); + + return ds.loadDescriptors(this, type, inputsource); + } finally { + // The source does not keep the stream. Close it here, whichever way loading ended, since this method opened + // it or was given it. + if (inputsource instanceof InputStream stream) { + try { + stream.close(); + } catch (IOException e) { + // Do not hide the outcome of the load + if (log.isDebugEnabled()) { + log.debug(sm.getString("registry.closeSourceError"), e); + } + } + } } - ModelerSource ds = getModelerSource(sourceType); - - return ds.loadDescriptors(this, type, inputsource); } diff --git a/java/org/apache/tomcat/util/modeler/modules/MbeansDescriptorsDigesterSource.java b/java/org/apache/tomcat/util/modeler/modules/MbeansDescriptorsDigesterSource.java index d70dcb0474..f052ccdc76 100644 --- a/java/org/apache/tomcat/util/modeler/modules/MbeansDescriptorsDigesterSource.java +++ b/java/org/apache/tomcat/util/modeler/modules/MbeansDescriptorsDigesterSource.java @@ -152,8 +152,8 @@ public class MbeansDescriptorsDigesterSource extends ModelerSource { digester = createDigester(); } - // Process the input file to configure our registry - try (stream) { + // Process the input file to configure our registry. The stream is closed by the caller (Registry.load()). + try { // Push our registry object onto the stack digester.push(loadedMbeans); digester.parse(stream); diff --git a/test/org/apache/tomcat/util/modeler/modules/TestMbeansDescriptorsDigesterSource.java b/test/org/apache/tomcat/util/modeler/modules/TestMbeansDescriptorsDigesterSource.java index ca7a0e56d5..c0db6e7074 100644 --- a/test/org/apache/tomcat/util/modeler/modules/TestMbeansDescriptorsDigesterSource.java +++ b/test/org/apache/tomcat/util/modeler/modules/TestMbeansDescriptorsDigesterSource.java @@ -17,10 +17,16 @@ package org.apache.tomcat.util.modeler.modules; import java.io.ByteArrayInputStream; +import java.io.File; import java.io.FilterInputStream; import java.io.IOException; import java.io.InputStream; +import java.net.URI; +import java.net.URL; +import java.net.URLConnection; +import java.net.URLStreamHandler; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.util.List; import java.util.concurrent.atomic.AtomicBoolean; @@ -81,6 +87,87 @@ public class TestMbeansDescriptorsDigesterSource { Assert.assertTrue(closed.get()); } + /* + * Registry.load() closes the stream whatever the outcome, including when the modeler source can't be created or + * rejects the source. + */ + @Test + public void testLoadClosesStreamWhenSourceTypeIsUnknown() throws Exception { + AtomicBoolean closed = new AtomicBoolean(); + Registry registry = Registry.getRegistry(null); + + Assert.assertThrows(ClassNotFoundException.class, + () -> registry.load("NoSuchModelerSource", createTrackingStream(XML, closed), null)); + + Assert.assertTrue(closed.get()); + } + + /* + * The stream that Registry.load() opens for a URL is closed too. + */ + @Test + public void testLoadClosesStreamOpenedForUrl() throws Exception { + AtomicBoolean closed = new AtomicBoolean(); + URL url = URL.of(URI.create("memory:/mbeans-descriptors.xml"), new URLStreamHandler() { + @Override + protected URLConnection openConnection(URL resource) { + return new URLConnection(resource) { + @Override + public void connect() { + // NO-OP + } + + @Override + public InputStream getInputStream() { + return createTrackingStream(XML, closed); + } + }; + } + }); + Registry registry = Registry.getRegistry(null); + + registry.load(null, url, null); + + Assert.assertTrue(closed.get()); + Assert.assertNotNull(registry.findManagedBean("TestDigesterSourceBean")); + } + + @Test + public void testLoadClosesStreamOpenedForFile() throws Exception { + File file = File.createTempFile("tomcat-test-", ".xml"); + try { + Files.writeString(file.toPath(), XML); + Registry registry = Registry.getRegistry(null); + + registry.load(null, file, null); + + Assert.assertNotNull(registry.findManagedBean("TestDigesterSourceBean")); + } finally { + // Can't be deleted on some platforms if the stream is still open + Assert.assertTrue(file.delete()); + } + } + + /* + * A failure to close the stream must not hide the outcome of the load. + */ + @Test + public void testFailureToCloseStreamDoesNotHideLoadFailure() throws Exception { + AtomicBoolean closed = new AtomicBoolean(); + InputStream stream = new FilterInputStream(new ByteArrayInputStream(XML.getBytes(StandardCharsets.UTF_8))) { + @Override + public void close() throws IOException { + closed.set(true); + throw new IOException("Close failed"); + } + }; + Registry registry = Registry.getRegistry(null); + + Assert.assertThrows(ClassNotFoundException.class, () -> registry.load("NoSuchModelerSource", stream, null)); + + Assert.assertTrue(closed.get()); + } + @Test public void testExecuteRequiresInputStreamSource() throws Exception { Registry registry = Registry.getRegistry(null); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
