Siphalor/tweed5-commons-logging
1
0
Fork 0
Code Activity

[LOGGING-192] Fix factory loading from TCCL (#281)

Browse Source 
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).
This commit is contained in:
Piotr P. Karwasz
2024-08-15 00:56:20 +02:00
committed by GitHub
parent 46f2d36868
commit 3ed2daf47f
6 changed files with 232 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
pom.xml
View File

@@ -283,6 +283,7 @@ under the License.
<org.apache.commons.logging.diagnostics.dest>STDOUT</org.apache.commons.logging.diagnostics.dest> <org.apache.commons.logging.diagnostics.dest>STDOUT</org.apache.commons.logging.diagnostics.dest>
--> -->
<log4j12>${org.apache.logging.log4j:log4j-1.2-api:jar}</log4j12> <log4j12>${org.apache.logging.log4j:log4j-1.2-api:jar}</log4j12>
<log4j-api>${org.apache.logging.log4j:log4j-api:jar}</log4j-api>
<logkit>${logkit:logkit:jar}</logkit> <logkit>${logkit:logkit:jar}</logkit>
<servlet-api>${javax.servlet:javax.servlet-api:jar}</servlet-api> <servlet-api>${javax.servlet:javax.servlet-api:jar}</servlet-api>
<commons-logging>target/${project.build.finalName}.jar</commons-logging> <commons-logging>target/${project.build.finalName}.jar</commons-logging>

1
src/changes/changes.xml
View File

@@ -45,6 +45,7 @@ The <action> type attribute can be add,update,fix,remove.
<body> <body>
<release version="1.3.4" date="YYYY-MM-DD" description="This is a feature and maintenance release. Java 8 or later is required."> <release version="1.3.4" date="YYYY-MM-DD" description="This is a feature and maintenance release. Java 8 or later is required.">
<!-- FIX --> <!-- FIX -->
<action dev="pkarwasz" issue="LOGGING-192" type="fix" due-to="Björn Kautler, Piotr Karwasz">Fix factory loading from context class loader.</action>
<!-- ADD --> <!-- ADD -->
<!-- UPDATE --> <!-- UPDATE -->
<action dev="ggregory" type="update" due-to="Gary Gregory">Bump org.apache.commons:commons-parent from 71 to 72.</action> <action dev="ggregory" type="update" due-to="Gary Gregory">Bump org.apache.commons:commons-parent from 71 to 72.</action>

117
src/main/java/org/apache/commons/logging/LogFactory.java
View File

@@ -331,8 +331,8 @@ public abstract class LogFactory {
+ " does not extend '" + LogFactory.class.getName() + "' as loaded by this class loader."); + " does not extend '" + LogFactory.class.getName() + "' as loaded by this class loader.");
logHierarchy("[BAD CL TREE] ", classLoader); logHierarchy("[BAD CL TREE] ", classLoader);
} }
// Force a ClassCastException
return logFactoryClass.getConstructor().newInstance(); return LogFactory.class.cast(logFactoryClass.getConstructor().newInstance());
} catch (final ClassNotFoundException ex) { } catch (final ClassNotFoundException ex) {
if (classLoader == thisClassLoaderRef.get()) { 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."); "Unable to load factory class via class loader " + objectId(classLoader) + " - trying the class loader associated with this LogFactory.");
} }
logFactoryClass = Class.forName(factoryClassName); logFactoryClass = Class.forName(factoryClassName);
return logFactoryClass.newInstance(); // Force a ClassCastException
return LogFactory.class.cast(logFactoryClass.getConstructor().newInstance());
} catch (final Exception e) { } catch (final Exception e) {
// Check to see if we've got a bad configuration // Check to see if we've got a bad configuration
if (isDiagnosticsEnabled()) { if (isDiagnosticsEnabled()) {
@@ -796,22 +797,31 @@ public abstract class LogFactory {
// Determine whether we will be using the thread context class loader to // 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). // load logging classes or not by checking the loaded properties file (if any).
ClassLoader baseClassLoader = contextClassLoader; boolean useTccl = contextClassLoader != null;
if (props != null) { if (props != null) {
final String useTCCLStr = props.getProperty(TCCL_KEY); final String useTCCLStr = props.getProperty(TCCL_KEY);
// The Boolean.valueOf(useTCCLStr).booleanValue() formulation useTccl &= useTCCLStr == null || Boolean.parseBoolean(useTCCLStr);
// is required for Java 1.2 compatibility. }
if (useTCCLStr != null && !Boolean.parseBoolean(useTCCLStr)) { // If TCCL is still enabled at this point, we check if it resolves this class
// Don't use current context class loader when locating any if (useTccl) {
// LogFactory or Log classes, just use the class that loaded try {
// this abstract class. When this class is deployed in a shared if (!LogFactory.class.equals(
// classpath of a container, it means webapps cannot deploy their Class.forName(LogFactory.class.getName(), false, contextClassLoader))) {
// own logging implementations. It also means that it is up to the logDiagnostic("The class " + LogFactory.class.getName() + " loaded by the context class loader " + objectId(contextClassLoader)
// implementation whether to load library-specific config files + " and this class differ. Disabling the usage of the context class loader."
// from the TCCL or not. + "Background can be found in https://commons.apache.org/logging/tech.html. ");
baseClassLoader = thisClassLoaderRef.get(); 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. // Determine which concrete LogFactory subclass to use.
// First, try a global system property // 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..."); logDiagnostic("[LOOKUP] Using ServiceLoader to define the LogFactory subclass to use...");
} }
try { try {
final ServiceLoader<LogFactory> serviceLoader = ServiceLoader.load(LogFactory.class); final ServiceLoader<LogFactory> serviceLoader = ServiceLoader.load(LogFactory.class, baseClassLoader);
final Iterator<LogFactory> iterator = serviceLoader.iterator(); final Iterator<LogFactory> iterator = serviceLoader.iterator();
int i = MAX_BROKEN_SERVICES; int i = MAX_BROKEN_SERVICES;
@@ -923,31 +933,19 @@ public abstract class LogFactory {
} }
} }
// Fourth, try one of the 3 provided factories // Fourth, try one of the three provided factories first from the specified classloader
// and then from the current one.
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());
}
if (factory == null) { 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()) { if (isDiagnosticsEnabled()) {
logDiagnostic( logDiagnostic(
"[LOOKUP] Loading the default LogFactory implementation '" + FACTORY_DEFAULT + "[LOOKUP] Loading the default LogFactory implementation '" + FACTORY_DEFAULT +
@@ -1408,6 +1406,45 @@ public abstract class LogFactory {
return newFactory(factoryClass, classLoader, null); return newFactory(factoryClass, classLoader, null);
} }
/**
* Tries to load one of the standard three implementations from the given classloader.
* <p>
* We assume that {@code classLoader} can load this class.
* </p>
* @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 * 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. * used to load this (abstract) LogFactory.

6
src/test/java/org/apache/commons/logging/security/SecurityForbiddenTestCase.java
View File

@@ -22,6 +22,7 @@ import static org.junit.Assert.assertNotEquals;
import java.io.PrintWriter; import java.io.PrintWriter;
import java.io.StringWriter; import java.io.StringWriter;
import java.lang.reflect.Field; import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method; import java.lang.reflect.Method;
import java.util.Hashtable; import java.util.Hashtable;
@@ -93,10 +94,11 @@ public class SecurityForbiddenTestCase extends TestCase {
final Class<?> clazz = classLoader.loadClass(name); final Class<?> clazz = classLoader.loadClass(name);
return clazz.getConstructor().newInstance(); return clazz.getConstructor().newInstance();
} catch (final Exception e) { } catch (final Exception e) {
final Throwable wrapped = e instanceof InvocationTargetException ? ((InvocationTargetException) e).getTargetException() : e;
final StringWriter sw = new StringWriter(); final StringWriter sw = new StringWriter();
final PrintWriter pw = new PrintWriter(sw); final PrintWriter pw = new PrintWriter(sw);
e.printStackTrace(pw); wrapped.printStackTrace(pw);
fail("Unexpected exception:" + e.getMessage() + ":" + sw.toString()); fail("Unexpected exception:" + wrapped.getMessage() + ":" + sw);
} }
return null; return null;
} }

76
src/test/java/org/apache/commons/logging/tccl/logfactory/AdaptersTcclTestCase.java Normal file
View File

@@ -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());
}
}

72
src/test/java/org/apache/commons/logging/tccl/logfactory/SiblingTcclTestCase.java Normal file
View File

@@ -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());
}
}