Skip to content

Commit 6f59e55

Browse files
committed
Prevent cache pollution by storing only the factories
Update `SpringFactoriesLoader` so that the cache stores only the factories and not the complete loader. Prior to this commit, if a cache entry was added with the thread conect classloader, the loader instance would a added and the classloader stored. If the thread context classloader subsequently changes, and a call is made to `forDefaultResourceLocation` with `null` for the classloader, the cached entry would be used which contains the older classloader. Fixes gh-34732
1 parent 3afd551 commit 6f59e55

File tree

2 files changed

+30
-7
lines changed

2 files changed

+30
-7
lines changed

Diff for: spring-core/src/main/java/org/springframework/core/io/support/SpringFactoriesLoader.java

+10-5
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2023 the original author or authors.
2+
* Copyright 2002-2025 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -101,7 +101,7 @@ public class SpringFactoriesLoader {
101101

102102
private static final Log logger = LogFactory.getLog(SpringFactoriesLoader.class);
103103

104-
static final Map<ClassLoader, Map<String, SpringFactoriesLoader>> cache = new ConcurrentReferenceHashMap<>();
104+
static final Map<ClassLoader, Map<String, Factories>> cache = new ConcurrentReferenceHashMap<>();
105105

106106

107107
@Nullable
@@ -327,10 +327,11 @@ public static SpringFactoriesLoader forResourceLocation(String resourceLocation,
327327
Assert.hasText(resourceLocation, "'resourceLocation' must not be empty");
328328
ClassLoader resourceClassLoader = (classLoader != null ? classLoader :
329329
SpringFactoriesLoader.class.getClassLoader());
330-
Map<String, SpringFactoriesLoader> loaders = cache.computeIfAbsent(
330+
Map<String, Factories> factoriesCache = cache.computeIfAbsent(
331331
resourceClassLoader, key -> new ConcurrentReferenceHashMap<>());
332-
return loaders.computeIfAbsent(resourceLocation, key ->
333-
new SpringFactoriesLoader(classLoader, loadFactoriesResource(resourceClassLoader, resourceLocation)));
332+
Factories factories = factoriesCache.computeIfAbsent(resourceLocation, key ->
333+
new Factories(loadFactoriesResource(resourceClassLoader, resourceLocation)));
334+
return new SpringFactoriesLoader(classLoader, factories.byType());
334335
}
335336

336337
protected static Map<String, List<String>> loadFactoriesResource(ClassLoader classLoader, String resourceLocation) {
@@ -673,4 +674,8 @@ static FailureHandler handleMessage(BiConsumer<Supplier<String>, Throwable> mess
673674
}
674675
}
675676

677+
678+
private record Factories(Map<String, List<String>> byType) {
679+
680+
}
676681
}

Diff for: spring-core/src/test/java/org/springframework/core/io/support/SpringFactoriesLoaderTests.java

+20-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2024 the original author or authors.
2+
* Copyright 2002-2025 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -25,6 +25,7 @@
2525
import java.util.List;
2626

2727
import org.apache.commons.logging.Log;
28+
import org.assertj.core.extractor.Extractors;
2829
import org.junit.jupiter.api.AfterAll;
2930
import org.junit.jupiter.api.BeforeAll;
3031
import org.junit.jupiter.api.Nested;
@@ -166,9 +167,26 @@ void loadForResourceLocationLoadsFactories() {
166167
void sameCachedResultIsUsedForDefaultClassLoaderAndNullClassLoader() {
167168
SpringFactoriesLoader forNull = SpringFactoriesLoader.forDefaultResourceLocation(null);
168169
SpringFactoriesLoader forDefault = SpringFactoriesLoader.forDefaultResourceLocation(ClassUtils.getDefaultClassLoader());
169-
assertThat(forNull).isSameAs(forDefault);
170+
assertThat(forNull).extracting("factories").isSameAs(Extractors.byName("factories").apply(forDefault));
170171
}
171172

173+
@Test
174+
void staleClassLoaderIsUsedWithCachedResult() {
175+
ClassLoader defaultClassLoader = ClassUtils.getDefaultClassLoader();
176+
ClassLoader cl1 = new ClassLoader() {
177+
};
178+
SpringFactoriesLoader factories1 = SpringFactoriesLoader.forDefaultResourceLocation(defaultClassLoader);
179+
assertThat(factories1).extracting("classLoader").isEqualTo(defaultClassLoader);
180+
ClassLoader previousClassLoader = Thread.currentThread().getContextClassLoader();
181+
try {
182+
Thread.currentThread().setContextClassLoader(cl1);
183+
SpringFactoriesLoader factories2 = SpringFactoriesLoader.forDefaultResourceLocation(null);
184+
assertThat(factories2).extracting("classLoader").isNull();
185+
}
186+
finally {
187+
Thread.currentThread().setContextClassLoader(previousClassLoader);
188+
}
189+
}
172190

173191
@Nested
174192
class FailureHandlerTests {

0 commit comments

Comments
 (0)