lukaszlenart commented on code in PR #1721:
URL: https://github.com/apache/struts/pull/1721#discussion_r3393294567
##########
core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java:
##########
@@ -73,14 +83,67 @@ public static Set<Class<?>> validateClasses(Set<String>
classNames, ClassLoader
Set<Class<?>> classes = new HashSet<>();
for (String className : classNames) {
try {
- classes.add(validatingClassLoader.loadClass(className));
+ classes.add(loadAndCacheClass(validatingClassLoader,
className));
} catch (ClassNotFoundException e) {
throw new ConfigurationException("Cannot load class for
exclusion/exemption configuration: " + className, e);
}
}
return classes;
}
+ private static Class<?> loadAndCacheClass(ClassLoader
validatingClassLoader, String className) throws ClassNotFoundException {
+ ClassLookupKey lookupKey = new
ClassLookupKey(classLoaderName(validatingClassLoader), className);
+
+ try {
+ return VALIDATED_CLASS_CACHE.get(lookupKey, key -> {
+ try {
+ return validatingClassLoader.loadClass(key.className);
+ } catch (ClassNotFoundException e) {
+ throw new ClassLookupException(e);
+ }
+ });
+ } catch (ClassLookupException e) {
+ throw (ClassNotFoundException) e.getCause();
+ }
+ }
+
+ private static String classLoaderName(ClassLoader classLoader) {
+ return String.valueOf(classLoader);
Review Comment:
**Most important: this string is not a reliable classloader identity.**
`ClassLoader` doesn't override `toString()`, so this is normally
`Class@hexHash` — but a custom loader overriding `toString()` to a constant
(your own `CountingClassLoader` does exactly this), or a plain `hashCode()`
collision, makes two *distinct* loaders produce the same key. The cache then
returns a `Class<?>` loaded by a different loader.
Since those `Class` objects drive `SecurityMemberAccess` exclusion/allowlist
checks (`==` / `isAssignableFrom`), a wrong-loader `Class` could subtly weaken
OGNL access control. Key on the actual `ClassLoader` reference instead — e.g. a
`Cache<ClassLoader, Cache<String, Class<?>>>` with `weakKeys()`, which uses
identity equality and is GC-safe, fixing this and the pinning issue in one go.
The `classLoaderName` helper name also implies a stable identity it doesn't
provide.
##########
core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java:
##########
@@ -33,6 +36,13 @@
import static org.apache.commons.lang3.StringUtils.strip;
public class ConfigParseUtil {
+ // Size the cache to prevent excessive memory usage in environments with
many classloaders and/or large numbers of classes being validated.
+ // While still providing a reasonable caching benefit for common cases
(e.g. multiple Struts instances in the same container, or multiple calls to
validate the same class across different containers).
+ private static final int MAX_CLASS_CACHE_SIZE = 50;
+
+ private static final Cache<ClassLookupKey, Class<?>> VALIDATED_CLASS_CACHE
= Caffeine.newBuilder()
Review Comment:
**Classloader-pinning risk (WW-5537 territory).** This `static` cache holds
strong `Class<?>` *values*, each of which keeps its defining `ClassLoader` (and
every class that loader defined) alive until size-50 LRU eviction. In the
production hot path the loader is always `OgnlUtil.class.getClassLoader()`, but
`validateClasses(Set, ClassLoader)` is `public` — any caller passing a webapp
loader creates a leak vector in whatever loader holds `ConfigParseUtil`.
Recommend `weakValues()` (Class no longer pins its loader) plus `weakKeys()`
on a real `ClassLoader` key — see the comment on `classLoaderName`.
##########
core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java:
##########
@@ -73,14 +83,67 @@ public static Set<Class<?>> validateClasses(Set<String>
classNames, ClassLoader
Set<Class<?>> classes = new HashSet<>();
for (String className : classNames) {
try {
- classes.add(validatingClassLoader.loadClass(className));
+ classes.add(loadAndCacheClass(validatingClassLoader,
className));
} catch (ClassNotFoundException e) {
throw new ConfigurationException("Cannot load class for
exclusion/exemption configuration: " + className, e);
}
}
return classes;
}
+ private static Class<?> loadAndCacheClass(ClassLoader
validatingClassLoader, String className) throws ClassNotFoundException {
+ ClassLookupKey lookupKey = new
ClassLookupKey(classLoaderName(validatingClassLoader), className);
+
+ try {
+ return VALIDATED_CLASS_CACHE.get(lookupKey, key -> {
+ try {
+ return validatingClassLoader.loadClass(key.className);
+ } catch (ClassNotFoundException e) {
+ throw new ClassLookupException(e);
+ }
+ });
+ } catch (ClassLookupException e) {
+ throw (ClassNotFoundException) e.getCause();
Review Comment:
Minor: the unchecked cast `(ClassNotFoundException) e.getCause()` is safe
today because `ClassLookupException`'s only constructor takes a
`ClassNotFoundException`, but a one-line comment documenting that invariant
would prevent a future NPE/CCE if another constructor is ever added.
##########
core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java:
##########
@@ -0,0 +1,161 @@
+/*
+ * 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.struts2.util;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import junit.framework.TestCase;
+import org.apache.struts2.config.ConfigurationException;
+
+import java.lang.reflect.Field;
+import java.util.Collections;
+import java.util.Set;
+
+public class ConfigParseUtilTest extends TestCase {
+
+ @Override
+ protected void setUp() throws Exception {
+ super.setUp();
+ validatedClassCache().invalidateAll();
+ }
+
+ @Override
+ protected void tearDown() throws Exception {
+ validatedClassCache().invalidateAll();
+ super.tearDown();
+ }
+
+ public void testValidateClassesCachesByClassLoaderAndClassName() {
+ CountingClassLoader loader = new
CountingClassLoader(getClass().getClassLoader(), "loader-one");
+ Set<String> classNames = Collections.singleton(String.class.getName());
+
+ ConfigParseUtil.validateClasses(classNames, loader);
+ ConfigParseUtil.validateClasses(classNames, loader);
+
+ assertEquals(1, loader.getStringClassLoads());
+ }
+
+ public void
testValidateClassesCachesAcrossMultipleRepeatedCallsWithSameClassLoader() {
+ CountingClassLoader loader = new
CountingClassLoader(getClass().getClassLoader(), "loader-one");
+ Set<String> classNames = Collections.singleton(String.class.getName());
+
+ for (int i = 0; i < 10; i++) {
+ ConfigParseUtil.validateClasses(classNames, loader);
+ }
+
+ assertEquals(1, loader.getStringClassLoads());
+ }
+
+ public void
testValidateClassesSeparatesEntriesAcrossDifferentClassLoaders() {
+ CountingClassLoader firstLoader = new
CountingClassLoader(getClass().getClassLoader(), "loader-one");
+ CountingClassLoader secondLoader = new
CountingClassLoader(getClass().getClassLoader(), "loader-two");
Review Comment:
Good coverage overall. One gap that matters: every test uses `toString()` as
the loader identity (`"loader-one"`, `"loader-two"`, `"loader-" + i`), which is
exactly what masks the key-collision bug. Please add a test with **two distinct
loaders that return the same `toString()`** and assert each loads independently
— against the current `String.valueOf(classLoader)` keying that test would fail
(cross-loader cache hit), demonstrating the issue and guarding the fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
