From 89b8b1a468a4d7de773f9a26fcd559306b62719b Mon Sep 17 00:00:00 2001
From: Phillip Webb <phil.webb@broadcom.com>
Date: Wed, 9 Apr 2025 14:36:34 -0700
Subject: [PATCH] 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
context classloader, the loader instance would be 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.

Closes gh-34732
---
 .../io/support/SpringFactoriesLoader.java     | 13 ++++++++----
 .../support/SpringFactoriesLoaderTests.java   | 20 ++++++++++++++++++-
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/spring-core/src/main/java/org/springframework/core/io/support/SpringFactoriesLoader.java b/spring-core/src/main/java/org/springframework/core/io/support/SpringFactoriesLoader.java
index 54ba8d2ab9..6d855b305b 100644
--- a/spring-core/src/main/java/org/springframework/core/io/support/SpringFactoriesLoader.java
+++ b/spring-core/src/main/java/org/springframework/core/io/support/SpringFactoriesLoader.java
@@ -101,7 +101,7 @@ public class SpringFactoriesLoader {
 
 	private static final Log logger = LogFactory.getLog(SpringFactoriesLoader.class);
 
-	static final Map<ClassLoader, Map<String, SpringFactoriesLoader>> cache = new ConcurrentReferenceHashMap<>();
+	static final Map<ClassLoader, Map<String, Factories>> cache = new ConcurrentReferenceHashMap<>();
 
 
 	@Nullable
@@ -321,10 +321,11 @@ public class SpringFactoriesLoader {
 		Assert.hasText(resourceLocation, "'resourceLocation' must not be empty");
 		ClassLoader resourceClassLoader = (classLoader != null ? classLoader :
 				SpringFactoriesLoader.class.getClassLoader());
-		Map<String, SpringFactoriesLoader> loaders = cache.computeIfAbsent(
+		Map<String, Factories> factoriesCache = cache.computeIfAbsent(
 				resourceClassLoader, key -> new ConcurrentReferenceHashMap<>());
-		return loaders.computeIfAbsent(resourceLocation, key ->
-				new SpringFactoriesLoader(classLoader, loadFactoriesResource(resourceClassLoader, resourceLocation)));
+		Factories factories = factoriesCache.computeIfAbsent(resourceLocation, key ->
+				new Factories(loadFactoriesResource(resourceClassLoader, resourceLocation)));
+		return new SpringFactoriesLoader(classLoader, factories.byType());
 	}
 
 	protected static Map<String, List<String>> loadFactoriesResource(ClassLoader classLoader, String resourceLocation) {
@@ -667,4 +668,8 @@ public class SpringFactoriesLoader {
 		}
 	}
 
+
+	private record Factories(Map<String, List<String>> byType) {
+
+	}
 }
diff --git a/spring-core/src/test/java/org/springframework/core/io/support/SpringFactoriesLoaderTests.java b/spring-core/src/test/java/org/springframework/core/io/support/SpringFactoriesLoaderTests.java
index af75d37a02..2d20de260c 100644
--- a/spring-core/src/test/java/org/springframework/core/io/support/SpringFactoriesLoaderTests.java
+++ b/spring-core/src/test/java/org/springframework/core/io/support/SpringFactoriesLoaderTests.java
@@ -25,6 +25,7 @@ import java.util.ArrayList;
 import java.util.List;
 
 import org.apache.commons.logging.Log;
+import org.assertj.core.extractor.Extractors;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Nested;
@@ -166,9 +167,26 @@ class SpringFactoriesLoaderTests {
 	void sameCachedResultIsUsedForDefaultClassLoaderAndNullClassLoader() {
 		SpringFactoriesLoader forNull = SpringFactoriesLoader.forDefaultResourceLocation(null);
 		SpringFactoriesLoader forDefault = SpringFactoriesLoader.forDefaultResourceLocation(ClassUtils.getDefaultClassLoader());
-		assertThat(forNull).isSameAs(forDefault);
+		assertThat(forNull).extracting("factories").isSameAs(Extractors.byName("factories").apply(forDefault));
 	}
 
+	@Test
+	void staleClassLoaderIsUsedWithCachedResult() {
+		ClassLoader defaultClassLoader = ClassUtils.getDefaultClassLoader();
+		ClassLoader cl1 = new ClassLoader() {
+		};
+		SpringFactoriesLoader factories1 = SpringFactoriesLoader.forDefaultResourceLocation(defaultClassLoader);
+		assertThat(factories1).extracting("classLoader").isEqualTo(defaultClassLoader);
+		ClassLoader previousClassLoader = Thread.currentThread().getContextClassLoader();
+		try {
+			Thread.currentThread().setContextClassLoader(cl1);
+			SpringFactoriesLoader factories2 = SpringFactoriesLoader.forDefaultResourceLocation(null);
+			assertThat(factories2).extracting("classLoader").isNull();
+		}
+		finally {
+			Thread.currentThread().setContextClassLoader(previousClassLoader);
+		}
+	}
 
 	@Nested
 	class FailureHandlerTests {