From 3ed2daf47f17506651216ebc95638fcc3cabbbb3 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Thu, 15 Aug 2024 00:56:20 +0200 Subject: [PATCH] [LOGGING-192] Fix factory loading from TCCL (#281) This fixes the loading of standard `LogFactory` implementations from the TCCL. The current code has a fallback clause in `newFactory`, so it can happen that the code tests for the presence of Log4j API and SLF4J in the TCCL, but it actually loads the factory from the current classloader (which lacks either of those libraries). --- pom.xml | 1 + src/changes/changes.xml | 3 +- .../apache/commons/logging/LogFactory.java | 117 ++++++++++++------ .../security/SecurityForbiddenTestCase.java | 6 +- .../tccl/logfactory/AdaptersTcclTestCase.java | 76 ++++++++++++ .../tccl/logfactory/SiblingTcclTestCase.java | 72 +++++++++++ 6 files changed, 232 insertions(+), 43 deletions(-) create mode 100644 src/test/java/org/apache/commons/logging/tccl/logfactory/AdaptersTcclTestCase.java create mode 100644 src/test/java/org/apache/commons/logging/tccl/logfactory/SiblingTcclTestCase.java diff --git a/pom.xml b/pom.xml index 1506572..a1d4457 100644 --- a/pom.xml +++ b/pom.xml @@ -283,6 +283,7 @@ under the License. STDOUT --> ${org.apache.logging.log4j:log4j-1.2-api:jar} + ${org.apache.logging.log4j:log4j-api:jar} ${logkit:logkit:jar} ${javax.servlet:javax.servlet-api:jar} target/${project.build.finalName}.jar diff --git a/src/changes/changes.xml b/src/changes/changes.xml index c240e9c..83ef9ac 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -45,10 +45,11 @@ The type attribute can be add,update,fix,remove. + Fix factory loading from context class loader. Bump org.apache.commons:commons-parent from 71 to 72. - Bump org.slf4j:slf4j-api from 2.0.13 to 2.0.15 #276. + Bump org.slf4j:slf4j-api from 2.0.13 to 2.0.15 #276. diff --git a/src/main/java/org/apache/commons/logging/LogFactory.java b/src/main/java/org/apache/commons/logging/LogFactory.java index bcf0983..238ca9e 100644 --- a/src/main/java/org/apache/commons/logging/LogFactory.java +++ b/src/main/java/org/apache/commons/logging/LogFactory.java @@ -331,8 +331,8 @@ public abstract class LogFactory { + " does not extend '" + LogFactory.class.getName() + "' as loaded by this class loader."); logHierarchy("[BAD CL TREE] ", classLoader); } - - return logFactoryClass.getConstructor().newInstance(); + // Force a ClassCastException + return LogFactory.class.cast(logFactoryClass.getConstructor().newInstance()); } catch (final ClassNotFoundException ex) { if (classLoader == thisClassLoaderRef.get()) { @@ -425,7 +425,8 @@ public abstract class LogFactory { "Unable to load factory class via class loader " + objectId(classLoader) + " - trying the class loader associated with this LogFactory."); } logFactoryClass = Class.forName(factoryClassName); - return logFactoryClass.newInstance(); + // Force a ClassCastException + return LogFactory.class.cast(logFactoryClass.getConstructor().newInstance()); } catch (final Exception e) { // Check to see if we've got a bad configuration if (isDiagnosticsEnabled()) { @@ -796,22 +797,31 @@ public abstract class LogFactory { // Determine whether we will be using the thread context class loader to // load logging classes or not by checking the loaded properties file (if any). - ClassLoader baseClassLoader = contextClassLoader; + boolean useTccl = contextClassLoader != null; if (props != null) { final String useTCCLStr = props.getProperty(TCCL_KEY); - // The Boolean.valueOf(useTCCLStr).booleanValue() formulation - // is required for Java 1.2 compatibility. - if (useTCCLStr != null && !Boolean.parseBoolean(useTCCLStr)) { - // Don't use current context class loader when locating any - // LogFactory or Log classes, just use the class that loaded - // this abstract class. When this class is deployed in a shared - // classpath of a container, it means webapps cannot deploy their - // own logging implementations. It also means that it is up to the - // implementation whether to load library-specific config files - // from the TCCL or not. - baseClassLoader = thisClassLoaderRef.get(); + useTccl &= useTCCLStr == null || Boolean.parseBoolean(useTCCLStr); + } + // If TCCL is still enabled at this point, we check if it resolves this class + if (useTccl) { + try { + if (!LogFactory.class.equals( + Class.forName(LogFactory.class.getName(), false, contextClassLoader))) { + logDiagnostic("The class " + LogFactory.class.getName() + " loaded by the context class loader " + objectId(contextClassLoader) + + " and this class differ. Disabling the usage of the context class loader." + + "Background can be found in https://commons.apache.org/logging/tech.html. "); + logHierarchy("[BAD CL TREE] ", contextClassLoader); + useTccl = false; + } + } catch (ClassNotFoundException ignored) { + logDiagnostic("The class " + LogFactory.class.getName() + " is not present in the the context class loader " + objectId(contextClassLoader) + + ". Disabling the usage of the context class loader." + + "Background can be found in https://commons.apache.org/logging/tech.html. "); + logHierarchy("[BAD CL TREE] ", contextClassLoader); + useTccl = false; } } + final ClassLoader baseClassLoader = useTccl ? contextClassLoader : thisClassLoaderRef.get(); // Determine which concrete LogFactory subclass to use. // First, try a global system property @@ -864,7 +874,7 @@ public abstract class LogFactory { logDiagnostic("[LOOKUP] Using ServiceLoader to define the LogFactory subclass to use..."); } try { - final ServiceLoader serviceLoader = ServiceLoader.load(LogFactory.class); + final ServiceLoader serviceLoader = ServiceLoader.load(LogFactory.class, baseClassLoader); final Iterator iterator = serviceLoader.iterator(); int i = MAX_BROKEN_SERVICES; @@ -923,31 +933,19 @@ public abstract class LogFactory { } } - // Fourth, try one of the 3 provided factories - - try { - // We prefer Log4j API, since it does not stringify objects. - if (factory == null && isClassAvailable(LOG4J_API_LOGGER, baseClassLoader)) { - // If the Log4j API is redirected to SLF4J, we use SLF4J directly. - if (isClassAvailable(LOG4J_TO_SLF4J_BRIDGE, baseClassLoader)) { - logDiagnostic( - "[LOOKUP] Log4j API to SLF4J redirection detected. Loading the SLF4J LogFactory implementation '" + FACTORY_SLF4J + "'."); - factory = newFactory(FACTORY_SLF4J, baseClassLoader, contextClassLoader); - } else { - logDiagnostic("[LOOKUP] Log4j API detected. Loading the Log4j API LogFactory implementation '" + FACTORY_LOG4J_API + "'."); - factory = newFactory(FACTORY_LOG4J_API, baseClassLoader, contextClassLoader); - } - } - - if (factory == null && isClassAvailable(SLF4J_API_LOGGER, baseClassLoader)) { - logDiagnostic("[LOOKUP] SLF4J detected. Loading the SLF4J LogFactory implementation '" + FACTORY_SLF4J + "'."); - factory = newFactory(FACTORY_SLF4J, baseClassLoader, contextClassLoader); - } - } catch (final Exception e) { - logDiagnostic("[LOOKUP] An exception occurred while creating LogFactory: " + e.getMessage()); - } - + // Fourth, try one of the three provided factories first from the specified classloader + // and then from the current one. if (factory == null) { + factory = newStandardFactory(baseClassLoader); + } + if (factory == null) { + factory = newStandardFactory(thisClassLoaderRef.get()); + } + if (factory != null) { + if (isDiagnosticsEnabled()) { + logDiagnostic("Created object " + objectId(factory) + " to manage class loader " + objectId(contextClassLoader)); + } + } else { if (isDiagnosticsEnabled()) { logDiagnostic( "[LOOKUP] Loading the default LogFactory implementation '" + FACTORY_DEFAULT + @@ -1408,6 +1406,45 @@ public abstract class LogFactory { return newFactory(factoryClass, classLoader, null); } + /** + * Tries to load one of the standard three implementations from the given classloader. + *

+ * We assume that {@code classLoader} can load this class. + *

+ * @param classLoader The classloader to use. + * @return An implementation of this class. + */ + private static LogFactory newStandardFactory(final ClassLoader classLoader) { + if (isClassAvailable(LOG4J_TO_SLF4J_BRIDGE, classLoader)) { + try { + return (LogFactory) Class.forName(FACTORY_SLF4J, true, classLoader).getConstructor().newInstance(); + } catch (final LinkageError | ReflectiveOperationException ignored) { + } finally { + logDiagnostic( + "[LOOKUP] Log4j API to SLF4J redirection detected. Loading the SLF4J LogFactory implementation '" + FACTORY_SLF4J + "'."); + } + } + try { + return (LogFactory) Class.forName(FACTORY_LOG4J_API, true, classLoader).getConstructor().newInstance(); + } catch (final LinkageError | ReflectiveOperationException ignored) { + } finally { + logDiagnostic("[LOOKUP] Loading the Log4j API LogFactory implementation '" + FACTORY_LOG4J_API + "'."); + } + try { + return (LogFactory) Class.forName(FACTORY_SLF4J, true, classLoader).getConstructor().newInstance(); + } catch (final LinkageError | ReflectiveOperationException ignored) { + } finally { + logDiagnostic("[LOOKUP] Loading the SLF4J LogFactory implementation '" + FACTORY_SLF4J + "'."); + } + try { + return (LogFactory) Class.forName(FACTORY_DEFAULT, true, classLoader).getConstructor().newInstance(); + } catch (final LinkageError | ReflectiveOperationException ignored) { + } finally { + logDiagnostic("[LOOKUP] Loading the legacy LogFactory implementation '" + FACTORY_DEFAULT + "'."); + } + return null; + } + /** * Gets a new instance of the specified {@code LogFactory} implementation class, loaded by the specified class loader. If that fails, try the class loader * used to load this (abstract) LogFactory. diff --git a/src/test/java/org/apache/commons/logging/security/SecurityForbiddenTestCase.java b/src/test/java/org/apache/commons/logging/security/SecurityForbiddenTestCase.java index 5e87db4..aa4005c 100644 --- a/src/test/java/org/apache/commons/logging/security/SecurityForbiddenTestCase.java +++ b/src/test/java/org/apache/commons/logging/security/SecurityForbiddenTestCase.java @@ -22,6 +22,7 @@ import static org.junit.Assert.assertNotEquals; import java.io.PrintWriter; import java.io.StringWriter; import java.lang.reflect.Field; +import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.Hashtable; @@ -93,10 +94,11 @@ public class SecurityForbiddenTestCase extends TestCase { final Class clazz = classLoader.loadClass(name); return clazz.getConstructor().newInstance(); } catch (final Exception e) { + final Throwable wrapped = e instanceof InvocationTargetException ? ((InvocationTargetException) e).getTargetException() : e; final StringWriter sw = new StringWriter(); final PrintWriter pw = new PrintWriter(sw); - e.printStackTrace(pw); - fail("Unexpected exception:" + e.getMessage() + ":" + sw.toString()); + wrapped.printStackTrace(pw); + fail("Unexpected exception:" + wrapped.getMessage() + ":" + sw); } return null; } diff --git a/src/test/java/org/apache/commons/logging/tccl/logfactory/AdaptersTcclTestCase.java b/src/test/java/org/apache/commons/logging/tccl/logfactory/AdaptersTcclTestCase.java new file mode 100644 index 0000000..2d97056 --- /dev/null +++ b/src/test/java/org/apache/commons/logging/tccl/logfactory/AdaptersTcclTestCase.java @@ -0,0 +1,76 @@ +/* + * 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.commons.logging.tccl.logfactory; + +import junit.framework.Test; +import junit.framework.TestCase; +import org.apache.commons.logging.LogFactory; +import org.apache.commons.logging.PathableClassLoader; +import org.apache.commons.logging.PathableTestSuite; +import org.apache.commons.logging.impl.Log4jApiLogFactory; + +/** + * Verifies that if the TCCL contains only `commons-logging-adapters`, it can load a web-application specific + * logging backend. + */ +public class AdaptersTcclTestCase extends TestCase { + + /** + * Return the tests included in this test suite. + */ + public static Test suite() throws Exception { + // The classloader running the test has access to `commons-logging` + final PathableClassLoader classLoader = new PathableClassLoader(null); + classLoader.useExplicitLoader("junit.", Test.class.getClassLoader()); + classLoader.addLogicalLib("commons-logging"); + classLoader.addLogicalLib("testclasses"); + + // The TCCL has access to both `commons-logging-adapters` and `log4j-api` + final PathableClassLoader tcclLoader = new PathableClassLoader(classLoader); + tcclLoader.addLogicalLib("commons-logging-adapters"); + tcclLoader.addLogicalLib("log4j-api"); + tcclLoader.setParentFirst(false); + + final Class testClass = classLoader.loadClass(AdaptersTcclTestCase.class.getName()); + return new PathableTestSuite(testClass, tcclLoader); + } + + /** + * Sets up instance variables required by this test case. + */ + @Override + public void setUp() throws Exception { + LogFactory.releaseAll(); + } + + /** + * Tear down instance variables required by this test case. + */ + @Override + public void tearDown() { + LogFactory.releaseAll(); + } + + public void testFactoryLoading() { + final LogFactory factory = LogFactory.getFactory(); + // The implementation comes from the TCCL + assertEquals(Thread.currentThread().getContextClassLoader(), factory.getClass().getClassLoader()); + // It uses the `log4j-api` only available in the TCCL + assertEquals(Log4jApiLogFactory.class.getName(), factory.getClass().getName()); + } +} diff --git a/src/test/java/org/apache/commons/logging/tccl/logfactory/SiblingTcclTestCase.java b/src/test/java/org/apache/commons/logging/tccl/logfactory/SiblingTcclTestCase.java new file mode 100644 index 0000000..54e3279 --- /dev/null +++ b/src/test/java/org/apache/commons/logging/tccl/logfactory/SiblingTcclTestCase.java @@ -0,0 +1,72 @@ +/* + * 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.commons.logging.tccl.logfactory; + +import junit.framework.Test; +import junit.framework.TestCase; +import org.apache.commons.logging.LogFactory; +import org.apache.commons.logging.PathableClassLoader; +import org.apache.commons.logging.PathableTestSuite; + +/** + * Verifies that if the TCCL is a sibling of the classloader with `commons-logging` + */ +public class SiblingTcclTestCase extends TestCase { + + /** + * Return the tests included in this test suite. + */ + public static Test suite() throws Exception { + // The classloader running the test has access to `commons-logging` + final PathableClassLoader classLoader = new PathableClassLoader(null); + classLoader.useExplicitLoader("junit.", Test.class.getClassLoader()); + classLoader.addLogicalLib("commons-logging"); + classLoader.addLogicalLib("testclasses"); + + // The TCCL has only access to `log4j-api` and `slf4j-api` + // See https://issues.apache.org/jira/browse/LOGGING-192 + final PathableClassLoader tcclLoader = new PathableClassLoader(null); + tcclLoader.addLogicalLib("log4j-api"); + + final Class testClass = classLoader.loadClass(SiblingTcclTestCase.class.getName()); + return new PathableTestSuite(testClass, tcclLoader); + } + + /** + * Sets up instance variables required by this test case. + */ + @Override + public void setUp() throws Exception { + LogFactory.releaseAll(); + } + + /** + * Tear down instance variables required by this test case. + */ + @Override + public void tearDown() { + LogFactory.releaseAll(); + } + + public void testFactoryLoading() { + // Loading the factory does not fail as in LOGGING-192 + final LogFactory factory = LogFactory.getFactory(); + // The selected implementation comes from this classloader + assertEquals(getClass().getClassLoader(), factory.getClass().getClassLoader()); + } +}