Skip to content

Commit 557bf7c

Browse files
ppkarwaszceki
ppkarwasz
authored and
ceki
committed
[SLF4J-548] Fix ServiceLoader usage in servlet environment
If both the servlet container and a web application use SLF4J, `ServiceLoader` calls are susceptible to three problems: 1. The SLF4J copy in the webapp detects the common providers by can not instantiate them (they implement a different copy of `SLF4JProviderService`), 2. The SLF4J copy in the common classloader can bind the providers in the webapp classloader and cause a memory leak, 3. If the server uses a SecurityManager the static initialization of `LoggerFactory` fails if called by unprivileged code. This PR should solve these problems. Signed-off-by: Piotr P. Karwasz <[email protected]>
1 parent 6324105 commit 557bf7c

File tree

4 files changed

+236
-6
lines changed

4 files changed

+236
-6
lines changed

‎slf4j-api/src/main/java/org/slf4j/LoggerFactory.java

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,20 +26,24 @@
2626

2727
import java.io.IOException;
2828
import java.net.URL;
29+
import java.security.AccessControlException;
30+
import java.security.AccessController;
31+
import java.security.PrivilegedAction;
2932
import java.util.ArrayList;
3033
import java.util.Arrays;
3134
import java.util.Enumeration;
35+
import java.util.Iterator;
3236
import java.util.LinkedHashSet;
3337
import java.util.List;
38+
import java.util.ServiceConfigurationError;
3439
import java.util.ServiceLoader;
3540
import java.util.Set;
3641
import java.util.concurrent.LinkedBlockingQueue;
3742

3843
import org.slf4j.event.SubstituteLoggingEvent;
3944
import org.slf4j.helpers.NOP_FallbackServiceProvider;
40-
import org.slf4j.helpers.SubstituteServiceProvider;
4145
import org.slf4j.helpers.SubstituteLogger;
42-
46+
import org.slf4j.helpers.SubstituteServiceProvider;
4347
import org.slf4j.helpers.Util;
4448
import org.slf4j.spi.SLF4JServiceProvider;
4549

@@ -98,11 +102,22 @@ public final class LoggerFactory {
98102

99103
static volatile SLF4JServiceProvider PROVIDER;
100104

101-
private static List<SLF4JServiceProvider> findServiceProviders() {
102-
ServiceLoader<SLF4JServiceProvider> serviceLoader = ServiceLoader.load(SLF4JServiceProvider.class);
105+
// Package access for tests
106+
static List<SLF4JServiceProvider> findServiceProviders() {
107+
final ClassLoader cl = LoggerFactory.class.getClassLoader();
108+
final PrivilegedAction<ServiceLoader<SLF4JServiceProvider>> action = () -> ServiceLoader.load(SLF4JServiceProvider.class, cl);
109+
final ServiceLoader<SLF4JServiceProvider> serviceLoader = System.getSecurityManager() != null
110+
? AccessController.doPrivileged(action)
111+
: action.run();
103112
List<SLF4JServiceProvider> providerList = new ArrayList<>();
104-
for (SLF4JServiceProvider provider : serviceLoader) {
105-
providerList.add(provider);
113+
Iterator<SLF4JServiceProvider> iterator = serviceLoader.iterator();
114+
while (iterator.hasNext()) {
115+
try {
116+
providerList.add(iterator.next());
117+
} catch (ServiceConfigurationError | AccessControlException e) {
118+
// Short warning
119+
Util.report("A SLF4J service provider failed to instantiate:\n" + e.getMessage());
120+
}
106121
}
107122
return providerList;
108123
}

‎slf4j-api/src/test/java/org/slf4j/LoggerFactoryTest.java

Lines changed: 210 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,210 @@
1+
/*
2+
* Permission is hereby granted, free of charge, to any person obtaining
3+
* a copy of this software and associated documentation files (the
4+
* "Software"), to deal in the Software without restriction, including
5+
* without limitation the rights to use, copy, modify, merge, publish,
6+
* distribute, sublicense, and/or sell copies of the Software, and to
7+
* permit persons to whom the Software is furnished to do so, subject to
8+
* the following conditions:
9+
*
10+
* The above copyright notice and this permission notice shall be
11+
* included in all copies or substantial portions of the Software.
12+
*
13+
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
14+
* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
15+
* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
16+
* NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
17+
* LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
18+
* OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
19+
* WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
20+
*
21+
*/
22+
package org.slf4j;
23+
24+
import static org.hamcrest.MatcherAssert.assertThat;
25+
import static org.hamcrest.core.Every.everyItem;
26+
import static org.hamcrest.core.IsInstanceOf.instanceOf;
27+
28+
import java.io.IOException;
29+
import java.lang.reflect.InvocationTargetException;
30+
import java.lang.reflect.Method;
31+
import java.net.URL;
32+
import java.net.URLClassLoader;
33+
import java.util.ArrayList;
34+
import java.util.Collections;
35+
import java.util.Enumeration;
36+
import java.util.HashMap;
37+
import java.util.List;
38+
import java.util.Map;
39+
40+
import org.hamcrest.BaseMatcher;
41+
import org.hamcrest.Description;
42+
import org.hamcrest.Matcher;
43+
import org.junit.Before;
44+
import org.junit.Test;
45+
import org.slf4j.helpers.NOP_FallbackServiceProvider;
46+
import org.slf4j.spi.SLF4JServiceProvider;
47+
48+
public class LoggerFactoryTest {
49+
50+
private static String SLF4J_PROVIDER = SLF4JServiceProvider.class.getName();
51+
private static String NOP_PROVIDER = NOP_FallbackServiceProvider.class.getName();
52+
53+
private WebappClassloader child;
54+
55+
@Before
56+
public void setUp() {
57+
final URL apiUrl = getLocation(LoggerFactory.class);
58+
final URL testUrl = getLocation(LoggerFactoryTest.class);
59+
child = new WebappClassloader(getClass().getClassLoader(), new URL[] { apiUrl, testUrl });
60+
}
61+
62+
/**
63+
* Tests if SLF4J in a web application is not disturbed by the SLF4J providers of the servlet container.
64+
*/
65+
@Test
66+
public void testNotASubtype() throws Exception {
67+
final ClassLoader tccl = Thread.currentThread().getContextClassLoader();
68+
// both the server and the webapp have `slf4j-api`, but with different providers:
69+
// NOP_FallbackServiceProvider only in the server classloader
70+
child.delegateToParent(NOP_PROVIDER);
71+
// SubstituteServiceProvider in the webapp
72+
child.addResource("META-INF/services/org.slf4j.spi.SLF4JServiceProvider", LoggerFactoryTest.class.getResource("substituteServiceProvider.txt"));
73+
try {
74+
Thread.currentThread().setContextClassLoader(child);
75+
final List<?> providers = findServiceProviders(child);
76+
final Class<?> slf4jProvider = child.loadClass(SLF4J_PROVIDER);
77+
assertThat(providers, everyItem(instanceOf(slf4jProvider)));
78+
} finally {
79+
Thread.currentThread().setContextClassLoader(tccl);
80+
}
81+
}
82+
83+
/**
84+
* Tests if SLF4J in a servlet container's common classloader does not use the providers in the webapplications.
85+
*/
86+
@Test
87+
public void testMemoryLeak() throws Exception {
88+
final ClassLoader tccl = Thread.currentThread().getContextClassLoader();
89+
// both the server and the webapp have `slf4j-api`, but with different providers:
90+
// NOP_FallbackServiceProvider only in the server classloader
91+
child.delegateToParent(NOP_PROVIDER);
92+
// SubstituteServiceProvider in the webapp
93+
child.addResource("META-INF/services/org.slf4j.spi.SLF4JServiceProvider", LoggerFactoryTest.class.getResource("substituteServiceProvider.txt"));
94+
try {
95+
Thread.currentThread().setContextClassLoader(child);
96+
final List<?> providers = LoggerFactory.findServiceProviders();
97+
assertThat(providers, everyItem(isAccessibleBy(LoggerFactory.class.getClassLoader())));
98+
} finally {
99+
Thread.currentThread().setContextClassLoader(tccl);
100+
}
101+
}
102+
103+
static List<SLF4JServiceProvider> callFromUnprivilegedDomain() {
104+
return LoggerFactory.findServiceProviders();
105+
}
106+
107+
private static List<?> findServiceProviders(final ClassLoader cl) throws Exception {
108+
try {
109+
final Class<?> loggerFactory = Class.forName("org.slf4j.LoggerFactory", true, cl);
110+
final Method findServiceProviders = loggerFactory.getDeclaredMethod("findServiceProviders");
111+
findServiceProviders.setAccessible(true);
112+
return (List<?>) findServiceProviders.invoke(null);
113+
} catch (InvocationTargetException e) {
114+
Throwable t = e.getCause();
115+
if (t instanceof Error) {
116+
throw (Error) t;
117+
}
118+
throw (Exception) t;
119+
}
120+
}
121+
122+
private static URL getLocation(Class<?> clazz) {
123+
return clazz.getProtectionDomain().getCodeSource().getLocation();
124+
}
125+
126+
private static <T> Matcher<T> isAccessibleBy(final ClassLoader cl) {
127+
return new BaseMatcher<T>() {
128+
129+
@Override
130+
public boolean matches(Object item) {
131+
try {
132+
Class<?> clazz = item.getClass();
133+
return Class.forName(clazz.getName(), true, cl).equals(clazz);
134+
} catch (ClassNotFoundException e) {
135+
return false;
136+
}
137+
}
138+
139+
@Override
140+
public void describeTo(Description description) {
141+
description.appendText("in accessible by").appendValue(cl);
142+
}
143+
};
144+
}
145+
146+
/**
147+
* Classloader with the same classpath as the parent, but does not delegate to the parent if the class name starts with {@code childFirstPrefix}.
148+
*
149+
*/
150+
private static class WebappClassloader extends URLClassLoader {
151+
152+
private final List<String> delegateToParent = new ArrayList<>();
153+
private final Map<String, URL> resourceOverride = new HashMap<>();
154+
155+
public WebappClassloader(ClassLoader parent, URL[] urls) {
156+
super(urls, parent);
157+
}
158+
159+
public void addResource(String name, URL resource) {
160+
resourceOverride.put(name, resource);
161+
}
162+
163+
public void delegateToParent(String prefix) {
164+
delegateToParent.add(prefix);
165+
}
166+
167+
@Override
168+
public Enumeration<URL> findResources(String name) throws IOException {
169+
final URL resource = resourceOverride.get(name);
170+
if (resource != null) {
171+
return Collections.enumeration(Collections.singleton(resource));
172+
}
173+
return super.findResources(name);
174+
}
175+
176+
@Override
177+
protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException {
178+
synchronized (getClassLoadingLock(name)) {
179+
// 1. Class already loaded
180+
Class<?> c = findLoadedClass(name);
181+
// 2. classes that we delegate to the parent classloader
182+
if (c == null) {
183+
if (name.startsWith("java")) {
184+
c = getParent().loadClass(name);
185+
} else {
186+
for (final String prefix : delegateToParent) {
187+
if (name.startsWith(prefix)) {
188+
c = getParent().loadClass(name);
189+
break;
190+
}
191+
}
192+
}
193+
}
194+
// 3. find in the classloader
195+
if (c == null) {
196+
c = findClass(name);
197+
}
198+
// 4. delegate to parent
199+
if (c == null) {
200+
c = getParent().loadClass(name);
201+
}
202+
if (resolve) {
203+
resolveClass(c);
204+
}
205+
return c;
206+
}
207+
}
208+
209+
}
210+
}

‎slf4j-api/src/test/resources/META-INF/services/org.slf4j.spi.SLF4JServiceProvider

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
###
2+
# Used in `LoggerFactoryTest`:
3+
org.slf4j.helpers.NOP_FallbackServiceProvider

‎slf4j-api/src/test/resources/org/slf4j/substituteServiceProvider.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
# Used in `LoggerFactoryTest`
2+
org.slf4j.helpers.SubstituteServiceProvider

0 commit comments

Comments
 (0)