Re: [PR] WW-5630 - Performance Issue SecurityMemberAccess [struts]

["lukaszlenart (via GitHub)" <gi...@apache.org>]

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: notifications-unsubscribe@struts.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org