This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 3fed180fbc3665665015c3923ee3711e4ef122ac Author: opencode <[email protected]> AuthorDate: Tue Oct 20 10:05:21 2026 +0200 Ignore unusable JULI Log service providers instead of failing The private constructor of LogFactory runs during the initialization of the singleton, so any unchecked exception escaping it leaves the class permanently unusable: every later call of getLog() or getFactory() fails with NoClassDefFoundError, which takes down the whole container because nearly every class holds a static Log. Two cases reached such an exception when a provider of the Log service was discovered through the ServiceLoader: a provider class without a public constructor taking the name, for which the code threw a raw Error, and a provider naming a class that cannot be loaded, for which the ServiceLoader itself throws a ServiceConfigurationError. Both cases now simply mark the provider as unusable. Discovery continues with the next provider and falls back to DirectJDKLog when no usable one is found, as documented by getFactory() all along. Providers are now inspected through the ServiceLoader stream API instead of instantiated objects, so the constructors of providers that are examined but not chosen are no longer run as a side effect. Added regression tests that run the discovery in an isolated class loader with a temporary services file, covering providers without a name constructor, missing provider classes, malformed services file lines, a usable provider behind unusable ones and the two unchanged cases of a single usable provider and no provider at all. --- java/org/apache/juli/logging/LogFactory.java | 26 +++- test/org/apache/juli/logging/TestLogFactory.java | 165 +++++++++++++++++++++ .../juli/logging/TesterLogNoStringConstructor.java | 117 +++++++++++++++ .../juli/logging/TesterLogStringConstructor.java | 125 ++++++++++++++++ 4 files changed, 425 insertions(+), 8 deletions(-) diff --git a/java/org/apache/juli/logging/LogFactory.java b/java/org/apache/juli/logging/LogFactory.java index 4df17a1f0a..3b6bc5e2b0 100644 --- a/java/org/apache/juli/logging/LogFactory.java +++ b/java/org/apache/juli/logging/LogFactory.java @@ -18,6 +18,8 @@ package org.apache.juli.logging; import java.lang.reflect.Constructor; import java.nio.file.FileSystems; +import java.util.Iterator; +import java.util.ServiceConfigurationError; import java.util.ServiceLoader; import java.util.logging.LogManager; @@ -70,16 +72,23 @@ public class LogFactory { FileSystems.getDefault(); // Look via a ServiceLoader for a Log implementation that has a - // constructor taking the String name. + // constructor taking the String name. Providers that cannot be + // loaded or that do not expose the required constructor are + // ignored, so a malformed provider does not prevent the discovery + // of a usable one, nor the fall back to DirectJDKLog. Note that, + // as the ServiceLoader requires, a provider also needs a public + // no-argument constructor to be considered at all. ServiceLoader<Log> logLoader = ServiceLoader.load(Log.class); Constructor<? extends Log> m = null; - for (Log log : logLoader) { - Class<? extends Log> c = log.getClass(); + Iterator<ServiceLoader.Provider<Log>> providers = logLoader.stream().iterator(); + while (m == null) { try { - m = c.getConstructor(String.class); - break; - } catch (NoSuchMethodException | SecurityException e) { - throw new Error(e); + if (!providers.hasNext()) { + break; + } + m = providers.next().type().getConstructor(String.class); + } catch (ReflectiveOperationException | SecurityException | ServiceConfigurationError e) { + // Not a usable provider. Ignore it and look at the next one. } } discoveredLogConstructor = m; @@ -145,7 +154,8 @@ public class LogFactory { * <p> * Return the singleton <code>LogFactory</code> instance. The {@link Log} implementation is discovered once, using * a {@link java.util.ServiceLoader} lookup for providers of the <code>Log</code> interface, falling back to - * {@link DirectJDKLog} if no suitable provider is found. + * {@link DirectJDKLog} if no suitable provider is found. Providers that cannot be loaded or that do not expose a + * public constructor taking the log name as its only argument are ignored. * </p> * * @return The singleton LogFactory instance diff --git a/test/org/apache/juli/logging/TestLogFactory.java b/test/org/apache/juli/logging/TestLogFactory.java new file mode 100644 index 0000000000..69d595ff79 --- /dev/null +++ b/test/org/apache/juli/logging/TestLogFactory.java @@ -0,0 +1,165 @@ +/* + * 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.juli.logging; + +import java.io.File; +import java.net.URL; +import java.net.URLClassLoader; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.junit.Assert; +import org.junit.Test; + +public class TestLogFactory { + + private static final String NO_STRING_CONSTRUCTOR = TesterLogNoStringConstructor.class.getName(); + private static final String STRING_CONSTRUCTOR = TesterLogStringConstructor.class.getName(); + private static final String MISSING = "org.apache.juli.logging.TesterMissingProvider"; + private static final String DIRECT_JDK_LOG = DirectJDKLog.class.getName(); + + /* + * A provider without the constructor taking the log name cannot be used. It must be + * ignored, with the discovery falling back to DirectJDKLog. Before the fix a raw Error + * was thrown from the singleton initialization, so all later calls failed with + * NoClassDefFoundError. + */ + @Test + public void testProviderWithoutStringConstructorIsIgnored() throws Exception { + Object log = getLogFromIsolatedFactory(NO_STRING_CONSTRUCTOR); + Assert.assertEquals(DIRECT_JDK_LOG, log.getClass().getName()); + } + + + /* + * A provider that names a class that does not exist makes the ServiceLoader throw a + * ServiceConfigurationError while iterating. That must not prevent discovery either. + */ + @Test + public void testMissingProviderClassIsIgnored() throws Exception { + Object log = getLogFromIsolatedFactory(MISSING); + Assert.assertEquals(DIRECT_JDK_LOG, log.getClass().getName()); + } + + + /* + * A syntactically invalid line in a services file also makes the ServiceLoader throw + * a ServiceConfigurationError. The factory must fall back instead of failing. Note + * that the JDK abandons the whole file in this case, so a usable provider in the same + * file is not found and DirectJDKLog is used. + */ + @Test + public void testMalformedProviderLineIsIgnored() throws Exception { + Object log = getLogFromIsolatedFactory("not a class name!!"); + Assert.assertEquals(DIRECT_JDK_LOG, log.getClass().getName()); + } + + + /* + * Discovery must continue past unusable providers and use the first usable one. + */ + @Test + public void testUsableProviderAfterUnusableOnesIsDiscovered() throws Exception { + Object log = getLogFromIsolatedFactory(MISSING, NO_STRING_CONSTRUCTOR, STRING_CONSTRUCTOR); + Assert.assertEquals(STRING_CONSTRUCTOR, log.getClass().getName()); + } + + + /* + * The regular case: a usable provider is discovered and used. Calling getLog() twice + * also pins that the singleton initialization does not fail on first use. + */ + @Test + public void testUsableProviderIsDiscovered() throws Exception { + Object log = getLogFromIsolatedFactory(STRING_CONSTRUCTOR); + Assert.assertEquals(STRING_CONSTRUCTOR, log.getClass().getName()); + } + + + /* + * Without any provider on the class path the fallback to DirectJDKLog must work. + */ + @Test + public void testNoProviderFallsBackToDirectJDKLog() throws Exception { + Object log = getLogFromIsolatedFactory(); + Assert.assertEquals(DIRECT_JDK_LOG, log.getClass().getName()); + } + + + /* + * Loads a fresh copy of the LogFactory in an isolated class loader in which the given + * names are registered as the only providers of the Log service, calls getLog() twice + * and returns the class name of the resulting Log instance. The returned object is of + * a Log type defined by the isolated class loader, so it can only be inspected as an + * Object. + */ + private Object getLogFromIsolatedFactory(String... providerNames) throws Exception { + Path tempDir = Files.createTempDirectory("juli-logfactory-test"); + try { + Path servicesDir = Files.createDirectories(tempDir.resolve("META-INF/services")); + StringBuilder servicesFile = new StringBuilder(); + for (String providerName : providerNames) { + servicesFile.append(providerName).append(System.lineSeparator()); + } + Files.write(servicesDir.resolve(Log.class.getName()), + servicesFile.toString().getBytes(StandardCharsets.UTF_8)); + + // The platform class loader as parent keeps the Log interface and the + // LogFactory of this class loader out of the way, so the isolated copy + // of the factory really runs its own discovery + URL[] urls = { codeSourceOf(LogFactory.class), codeSourceOf(TestLogFactory.class), + tempDir.toUri().toURL() }; + try (URLClassLoader isolatedLoader = new URLClassLoader(urls, ClassLoader.getPlatformClassLoader())) { + Thread currentThread = Thread.currentThread(); + ClassLoader originalLoader = currentThread.getContextClassLoader(); + // The ServiceLoader used by LogFactory looks at the context class loader + currentThread.setContextClassLoader(isolatedLoader); + try { + Class<?> isolatedFactory = isolatedLoader.loadClass(LogFactory.class.getName()); + Object result = null; + // Call twice, the second call must not fail with NoClassDefFoundError + for (int i = 0; i < 2; i++) { + result = isolatedFactory.getMethod("getLog", String.class).invoke(null, "tester"); + Assert.assertNotNull(result); + } + return result; + } finally { + currentThread.setContextClassLoader(originalLoader); + } + } + } finally { + deleteRecursively(tempDir.toFile()); + } + } + + + private static URL codeSourceOf(Class<?> clazz) throws Exception { + return clazz.getProtectionDomain().getCodeSource().getLocation(); + } + + + private static void deleteRecursively(File file) { + File[] children = file.listFiles(); + if (children != null) { + for (File child : children) { + deleteRecursively(child); + } + } + Assert.assertTrue("Failed to delete [" + file + "]", file.delete()); + } +} diff --git a/test/org/apache/juli/logging/TesterLogNoStringConstructor.java b/test/org/apache/juli/logging/TesterLogNoStringConstructor.java new file mode 100644 index 0000000000..454703102b --- /dev/null +++ b/test/org/apache/juli/logging/TesterLogNoStringConstructor.java @@ -0,0 +1,117 @@ +/* + * 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.juli.logging; + +/* + * Service provider for the tests of LogFactory that offers only the no-argument + * constructor required by the ServiceLoader itself, but not the constructor + * taking the log name, so LogFactory must ignore it. + */ +public class TesterLogNoStringConstructor implements Log { + + private static final long serialVersionUID = 1L; + + @Override + public boolean isDebugEnabled() { + return false; + } + + @Override + public boolean isErrorEnabled() { + return false; + } + + @Override + public boolean isFatalEnabled() { + return false; + } + + @Override + public boolean isInfoEnabled() { + return false; + } + + @Override + public boolean isTraceEnabled() { + return false; + } + + @Override + public boolean isWarnEnabled() { + return false; + } + + @Override + public void trace(Object message) { + // NO-OP + } + + @Override + public void trace(Object message, Throwable t) { + // NO-OP + } + + @Override + public void debug(Object message) { + // NO-OP + } + + @Override + public void debug(Object message, Throwable t) { + // NO-OP + } + + @Override + public void info(Object message) { + // NO-OP + } + + @Override + public void info(Object message, Throwable t) { + // NO-OP + } + + @Override + public void warn(Object message) { + // NO-OP + } + + @Override + public void warn(Object message, Throwable t) { + // NO-OP + } + + @Override + public void error(Object message) { + // NO-OP + } + + @Override + public void error(Object message, Throwable t) { + // NO-OP + } + + @Override + public void fatal(Object message) { + // NO-OP + } + + @Override + public void fatal(Object message, Throwable t) { + // NO-OP + } +} diff --git a/test/org/apache/juli/logging/TesterLogStringConstructor.java b/test/org/apache/juli/logging/TesterLogStringConstructor.java new file mode 100644 index 0000000000..50b0628721 --- /dev/null +++ b/test/org/apache/juli/logging/TesterLogStringConstructor.java @@ -0,0 +1,125 @@ +/* + * 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.juli.logging; + +/* + * Service provider for the tests of LogFactory that offers both the + * no-argument constructor required by the ServiceLoader itself and the + * constructor taking the log name, so LogFactory must discover and use it. + */ +public class TesterLogStringConstructor implements Log { + + private static final long serialVersionUID = 1L; + + public TesterLogStringConstructor() { + // Required no-argument constructor for the ServiceLoader + } + + public TesterLogStringConstructor(String name) { + // Used by LogFactory to create the instance for the given name + } + + @Override + public boolean isDebugEnabled() { + return false; + } + + @Override + public boolean isErrorEnabled() { + return false; + } + + @Override + public boolean isFatalEnabled() { + return false; + } + + @Override + public boolean isInfoEnabled() { + return false; + } + + @Override + public boolean isTraceEnabled() { + return false; + } + + @Override + public boolean isWarnEnabled() { + return false; + } + + @Override + public void trace(Object message) { + // NO-OP + } + + @Override + public void trace(Object message, Throwable t) { + // NO-OP + } + + @Override + public void debug(Object message) { + // NO-OP + } + + @Override + public void debug(Object message, Throwable t) { + // NO-OP + } + + @Override + public void info(Object message) { + // NO-OP + } + + @Override + public void info(Object message, Throwable t) { + // NO-OP + } + + @Override + public void warn(Object message) { + // NO-OP + } + + @Override + public void warn(Object message, Throwable t) { + // NO-OP + } + + @Override + public void error(Object message) { + // NO-OP + } + + @Override + public void error(Object message, Throwable t) { + // NO-OP + } + + @Override + public void fatal(Object message) { + // NO-OP + } + + @Override + public void fatal(Object message, Throwable t) { + // NO-OP + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
