This is an automated email from the ASF dual-hosted git repository.
ggregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-logging.git
The following commit(s) were added to refs/heads/master by this push:
new 3ed2daf [LOGGING-192] Fix factory loading from TCCL (#281)
3ed2daf is described below
commit 3ed2daf47f17506651216ebc95638fcc3cabbbb3
Author: Piotr P. Karwasz <piotr.git...@karwasz.org>
AuthorDate: Thu Aug 15 00:56:20 2024 +0200
[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 +-
.../org/apache/commons/logging/LogFactory.java | 115 ++++++++++++++-------
.../security/SecurityForbiddenTestCase.java | 6 +-
.../tccl/logfactory/AdaptersTcclTestCase.java | 76 ++++++++++++++
.../tccl/logfactory/SiblingTcclTestCase.java | 72 +++++++++++++
6 files changed, 231 insertions(+), 42 deletions(-)
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.
<org.apache.commons.logging.diagnostics.dest>STDOUT</org.apache.commons.logging.diagnostics.dest>
-->
<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>
<servlet-api>${javax.servlet:javax.servlet-api:jar}</servlet-api>
<commons-logging>target/${project.build.finalName}.jar</commons-logging>
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 <action> type attribute can be add,update,fix,remove.
<body>
<release version="1.3.4" date="YYYY-MM-DD" description="This is a feature
and maintenance release. Java 8 or later is required.">
<!-- 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 -->
<!-- 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,
Dependabot">Bump org.slf4j:slf4j-api from 2.0.13 to 2.0.15 #276.</action>
+ <action dev="ggregory" type="update" due-to="Gary Gregory,
Dependabot">Bump org.slf4j:slf4j-api from 2.0.13 to 2.0.15 #276.</action>
</release>
<release version="1.3.3" date="2024-06-30" description="This is a feature
and maintenance release. Java 8 or later is required.">
<!-- FIX -->
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<LogFactory> serviceLoader =
ServiceLoader.load(LogFactory.class);
+ final ServiceLoader<LogFactory> serviceLoader =
ServiceLoader.load(LogFactory.class, baseClassLoader);
final Iterator<LogFactory> 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.
+ * <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
* 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());
+ }
+}
