This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/main by this push:
new 210dc86b8 WW-5630 - Performance Issue SecurityMemberAccess (#1721)
210dc86b8 is described below
commit 210dc86b8840e55ac02c2fe88a56a48b0c898a62
Author: brianandle <[email protected]>
AuthorDate: Fri Jun 12 10:12:26 2026 -0700
WW-5630 - Performance Issue SecurityMemberAccess (#1721)
* WW-5630 - Performance Issue SecurityMemberAccess
* Add size bound cache, 50, for Class lookup
* Add unit test
Code generated by Copilot
* WW-5630 - Add additional UT
* WW-5630 - Add UT for non-existent class
* WW-5630 - Review feedback changes
* Cache ClassLoader directly
* Use weakKeys and weakValues
* Comment on the ClassLookupException
* Additional Unit Tests
Assistance in coding using co-pilot
* WW-5630 - Additional review
* Limit outer, Classloader, to 25. Ensure memory bounding.
* Limit inner, Classes, to 50. Ensure memory bounding.
* Additional UTs
With co-pilot assitance
---
.../org/apache/struts2/util/ConfigParseUtil.java | 44 ++-
.../apache/struts2/util/ConfigParseUtilTest.java | 348 +++++++++++++++++++++
2 files changed, 391 insertions(+), 1 deletion(-)
diff --git a/core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java
b/core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java
index d6cdafabf..116ca637d 100644
--- a/core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java
+++ b/core/src/main/java/org/apache/struts2/util/ConfigParseUtil.java
@@ -18,6 +18,8 @@
*/
package org.apache.struts2.util;
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
import org.apache.struts2.config.ConfigurationException;
import org.apache.struts2.ognl.OgnlUtil;
@@ -33,6 +35,17 @@ import static java.util.stream.Collectors.toSet;
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).
+ // The cache is sized to allow for some level of caching across multiple
classloaders, while still allowing for a reasonable number of classes to be
cached per classloader.
+ private static final int MAX_CLASSLOADER_CACHE_SIZE = 25;
+ // The cache for validated classes is a two-level cache, with the first
level keyed by ClassLoader and the second level keyed by class name.
+ private static final int MAX_CLASS_CACHE_PER_LOADER_SIZE = 50;
+
+ private static final Cache<ClassLoader, Cache<String, Class<?>>>
VALIDATED_CLASS_CACHE = Caffeine.newBuilder()
+ .weakKeys()
+ .maximumSize(MAX_CLASSLOADER_CACHE_SIZE)
+ .build();
private ConfigParseUtil() {
}
@@ -73,7 +86,7 @@ public class ConfigParseUtil {
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);
}
@@ -81,6 +94,35 @@ public class ConfigParseUtil {
return classes;
}
+ private static Class<?> loadAndCacheClass(ClassLoader
validatingClassLoader, String className) throws ClassNotFoundException {
+ Cache<String, Class<?>> classLoaderCache =
VALIDATED_CLASS_CACHE.get(validatingClassLoader,
+ key ->
Caffeine.newBuilder().weakValues().maximumSize(MAX_CLASS_CACHE_PER_LOADER_SIZE).build());
+
+ try {
+ return classLoaderCache.get(className, key -> {
+ try {
+ return validatingClassLoader.loadClass(key);
+ } catch (ClassNotFoundException e) {
+ throw new ClassLookupException(e);
+ }
+ });
+ } catch (ClassLookupException e) {
+ // The ClassLookupException only serves to wrap the checked
ClassNotFoundException thrown by ClassLoader.loadClass.
+ throw (ClassNotFoundException) e.getCause();
+ }
+ }
+
+ /**
+ * This is a wrapper class to allow the checked ClassNotFoundException
thrown by ClassLoader.loadClass to be propagated
+ * We should always be able to unwrap this exception without risk of
ClassCastException since the only code that can throw it is the mapping
function passed to the cache
+ * and it only ever throws this wrapper with a ClassNotFoundException
cause.
+ */
+ private static final class ClassLookupException extends RuntimeException {
+ private ClassLookupException(ClassNotFoundException cause) {
+ super(cause);
+ }
+ }
+
public static Set<String> toPackageNamesSet(String
newDelimitedPackageNames) throws ConfigurationException {
Set<String> packageNames =
commaDelimitedStringToSet(newDelimitedPackageNames)
.stream().map(s -> strip(s, ".")).collect(toSet());
diff --git
a/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java
b/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java
new file mode 100644
index 000000000..c57ae6cbc
--- /dev/null
+++ b/core/src/test/java/org/apache/struts2/util/ConfigParseUtilTest.java
@@ -0,0 +1,348 @@
+/*
+ * 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");
+ Set<String> classNames = Collections.singleton(String.class.getName());
+
+ ConfigParseUtil.validateClasses(classNames, firstLoader);
+ ConfigParseUtil.validateClasses(classNames, secondLoader);
+
+ assertEquals(1, firstLoader.getStringClassLoads());
+ assertEquals(1, secondLoader.getStringClassLoads());
+ }
+
+ public void
testValidateClassesSeparatesEntriesAcrossDifferentClassLoadersWithSameToString()
{
+ CountingClassLoader firstLoader = new
CountingClassLoader(getClass().getClassLoader(), "same-loader-name");
+ CountingClassLoader secondLoader = new
CountingClassLoader(getClass().getClassLoader(), "same-loader-name");
+ Set<String> classNames = Collections.singleton(String.class.getName());
+
+ ConfigParseUtil.validateClasses(classNames, firstLoader);
+ ConfigParseUtil.validateClasses(classNames, secondLoader);
+
+ assertEquals(1, firstLoader.getStringClassLoads());
+ assertEquals(1, secondLoader.getStringClassLoads());
+ }
+
+ public void testValidateClassesEnforcesOuterCacheMaximumSize() {
+ Set<String> classNames = Collections.singleton(String.class.getName());
+
+ for (int i = 0; i < 60; i++) {
+ CountingClassLoader loader = new
CountingClassLoader(getClass().getClassLoader(), "loader-" + i);
+ ConfigParseUtil.validateClasses(classNames, loader);
+ }
+
+ Cache<Object, Object> cache = validatedClassCache();
+ cache.cleanUp();
+
+ assertTrue("Outer cache size should not exceed configured maximum",
cache.estimatedSize() <= outerCacheLimit());
+ }
+
+ public void testValidateClassesThrowsForNonExistingClassNameOnEachCall() {
+ String missingClassName =
"org.apache.struts2.util.NonExistingClassForValidationTest";
+ Set<String> classNames = Collections.singleton(missingClassName);
+ int[] missingClassLoads = new int[1];
+ ClassLoader loader = new ClassLoader(getClass().getClassLoader()) {
+ @Override
+ public Class<?> loadClass(String name) throws
ClassNotFoundException {
+ if (missingClassName.equals(name)) {
+ missingClassLoads[0]++;
+ throw new ClassNotFoundException(name);
+ }
+ return super.loadClass(name);
+ }
+
+ @Override
+ public String toString() {
+ return "missing-class-loader";
+ }
+ };
+
+ for (int i = 0; i < 2; i++) {
+ try {
+ ConfigParseUtil.validateClasses(classNames, loader);
+ fail("Expected ConfigurationException for class: " +
missingClassName);
+ } catch (ConfigurationException e) {
+ assertTrue(e.getMessage().contains(missingClassName));
+ assertNotNull(e.getCause());
+ assertEquals(ClassNotFoundException.class,
e.getCause().getClass());
+ }
+ }
+
+ assertEquals(2, missingClassLoads[0]);
+ }
+
+ public void
testValidateClassesLoadsMultipleDifferentClassesPerLoaderOnce() {
+ CountingClassLoader loader = new
CountingClassLoader(getClass().getClassLoader(), "multi-class-loader");
+ Set<String> classNames = new java.util.HashSet<>();
+ classNames.add(String.class.getName());
+ classNames.add(Integer.class.getName());
+ classNames.add(Boolean.class.getName());
+
+ ConfigParseUtil.validateClasses(classNames, loader);
+ ConfigParseUtil.validateClasses(classNames, loader);
+
+ // Verify each class was loaded only once per loader through the cache
+ assertEquals(1, loader.getLoadCount(String.class.getName()));
+ assertEquals(1, loader.getLoadCount(Integer.class.getName()));
+ assertEquals(1, loader.getLoadCount(Boolean.class.getName()));
+ }
+
+ public void testValidateClassesNestedCacheIsReusedForSameLoader() {
+ CountingClassLoader loader = new
CountingClassLoader(getClass().getClassLoader(), "reuse-cache-loader");
+ Set<String> classNames = Collections.singleton(String.class.getName());
+
+ ConfigParseUtil.validateClasses(classNames, loader);
+ int firstCallLoadCount = loader.getStringClassLoads();
+
+ ConfigParseUtil.validateClasses(classNames, loader);
+ int secondCallLoadCount = loader.getStringClassLoads();
+
+ assertEquals(1, firstCallLoadCount);
+ assertEquals(1, secondCallLoadCount);
+ }
+
+ public void testInnerCacheEnforcesMaximumSizePerClassLoader() {
+ CountingClassLoader loader = new
CountingClassLoader(getClass().getClassLoader(), "many-classes-loader");
+ Set<String> classNames = new java.util.HashSet<>();
+
+ // Use real built-in classes that are guaranteed to exist (70+ classes)
+ String[] realClasses = {
+ String.class.getName(), Integer.class.getName(),
Long.class.getName(),
+ Double.class.getName(), Float.class.getName(),
Boolean.class.getName(),
+ Character.class.getName(), Byte.class.getName(),
Short.class.getName(),
+ Object.class.getName(), Class.class.getName(),
Thread.class.getName(),
+ Exception.class.getName(), RuntimeException.class.getName(),
java.io.File.class.getName(),
+ java.util.ArrayList.class.getName(),
java.util.HashMap.class.getName(),
+ java.util.HashSet.class.getName(),
java.util.LinkedList.class.getName(),
+ java.util.TreeMap.class.getName(),
java.util.regex.Pattern.class.getName(),
+ java.io.InputStream.class.getName(),
java.io.OutputStream.class.getName(),
+ java.io.Reader.class.getName(), java.io.Writer.class.getName(),
+ java.net.URL.class.getName(), java.net.URI.class.getName(),
+ java.nio.file.Path.class.getName(),
java.nio.file.Paths.class.getName(),
+ java.nio.file.Files.class.getName(),
java.nio.file.StandardOpenOption.class.getName(),
+ java.lang.reflect.Method.class.getName(),
java.lang.reflect.Field.class.getName(),
+ java.lang.reflect.Constructor.class.getName(),
java.lang.annotation.Annotation.class.getName(),
+ java.util.concurrent.Future.class.getName(),
java.util.concurrent.ExecutorService.class.getName(),
+ java.time.LocalDate.class.getName(),
java.time.LocalTime.class.getName(),
+ java.time.LocalDateTime.class.getName(),
java.time.ZonedDateTime.class.getName(),
+ java.time.Instant.class.getName(),
java.time.Duration.class.getName(),
+ java.time.Period.class.getName(),
java.util.stream.Stream.class.getName(),
+ java.util.stream.Collectors.class.getName(),
java.util.Optional.class.getName(),
+ java.util.function.Function.class.getName(),
java.util.function.Predicate.class.getName(),
+ java.util.function.Consumer.class.getName(),
java.util.function.Supplier.class.getName(),
+ java.util.function.BiFunction.class.getName(),
java.util.function.BiConsumer.class.getName(),
+ java.util.function.BiPredicate.class.getName(),
java.lang.StringBuilder.class.getName(),
+ java.lang.StringBuffer.class.getName(),
java.util.Collections.class.getName(),
+ java.util.Arrays.class.getName(),
java.util.Objects.class.getName(),
+ java.util.UUID.class.getName(), java.util.Locale.class.getName(),
+ java.util.TimeZone.class.getName(),
java.util.Calendar.class.getName(),
+ java.util.Date.class.getName(),
java.util.GregorianCalendar.class.getName(),
+ java.util.Random.class.getName(),
java.util.Scanner.class.getName(),
+ java.util.Formatter.class.getName(),
java.util.Properties.class.getName(),
+ java.util.concurrent.ConcurrentHashMap.class.getName(),
java.util.concurrent.atomic.AtomicInteger.class.getName(),
+ java.util.concurrent.atomic.AtomicLong.class.getName(),
java.util.concurrent.locks.Lock.class.getName(),
+ java.util.concurrent.locks.ReentrantLock.class.getName(),
java.lang.Comparable.class.getName(),
+ java.lang.Cloneable.class.getName(),
java.io.Serializable.class.getName(),
+ java.nio.ByteBuffer.class.getName(),
java.nio.CharBuffer.class.getName(),
+ java.nio.charset.Charset.class.getName(),
java.security.MessageDigest.class.getName()
+ };
+
+ // Add all real classes to the set (should be 72)
+ for (String className : realClasses) {
+ classNames.add(className);
+ }
+
+ assertTrue("Test setup requires more class names than inner cache
capacity", classNames.size() > innerCacheLimit());
+
+ ConfigParseUtil.validateClasses(classNames, loader);
+
+ Cache<Object, Object> innerCache = innerCacheFor(loader);
+ innerCache.cleanUp();
+ assertTrue("Inner cache size should not exceed configured maximum per
loader", innerCache.estimatedSize() <= innerCacheLimit());
+ }
+
+ public void testInnerCacheIndependentPerClassLoader() {
+ CountingClassLoader loaderA = new
CountingClassLoader(getClass().getClassLoader(), "loader-a");
+ CountingClassLoader loaderB = new
CountingClassLoader(getClass().getClassLoader(), "loader-b");
+
+ Set<String> classNamesA = new java.util.HashSet<>();
+ classNamesA.add(String.class.getName());
+ classNamesA.add(Integer.class.getName());
+
+ Set<String> classNamesB = new java.util.HashSet<>();
+ classNamesB.add(Boolean.class.getName());
+ classNamesB.add(Double.class.getName());
+
+ ConfigParseUtil.validateClasses(classNamesA, loaderA);
+ ConfigParseUtil.validateClasses(classNamesB, loaderB);
+
+ // Validate load counts show independent caching - each loader loaded
its own classes once
+ assertEquals(1, loaderA.getLoadCount(String.class.getName()));
+ assertEquals(1, loaderA.getLoadCount(Integer.class.getName()));
+ assertEquals(0, loaderB.getLoadCount(String.class.getName()));
+ assertEquals(1, loaderB.getLoadCount(Boolean.class.getName()));
+ assertEquals(1, loaderB.getLoadCount(Double.class.getName()));
+ }
+
+ public void testInnerCacheReusesCachedClassLookupForSameLoader() {
+ CountingClassLoader loader = new
CountingClassLoader(getClass().getClassLoader(), "reload-test-loader");
+ Set<String> classNames = Collections.singleton(String.class.getName());
+
+ // Load once
+ ConfigParseUtil.validateClasses(classNames, loader);
+ assertEquals(1, loader.getStringClassLoads());
+
+ // Reload - should use cache
+ ConfigParseUtil.validateClasses(classNames, loader);
+ assertEquals(1, loader.getStringClassLoads());
+ }
+
+ public void testMultipleClassLoadersWithDistinctInnerCaches() {
+ CountingClassLoader loader1 = new
CountingClassLoader(getClass().getClassLoader(), "distinct-1");
+ CountingClassLoader loader2 = new
CountingClassLoader(getClass().getClassLoader(), "distinct-2");
+ CountingClassLoader loader3 = new
CountingClassLoader(getClass().getClassLoader(), "distinct-3");
+
+ Set<String> classNames = Collections.singleton(String.class.getName());
+
+ ConfigParseUtil.validateClasses(classNames, loader1);
+ ConfigParseUtil.validateClasses(classNames, loader2);
+ ConfigParseUtil.validateClasses(classNames, loader3);
+
+ // Each loader should have loaded String once independently
+ assertEquals(1, loader1.getLoadCount(String.class.getName()));
+ assertEquals(1, loader2.getLoadCount(String.class.getName()));
+ assertEquals(1, loader3.getLoadCount(String.class.getName()));
+
+ // Revalidating with loader1 should still use cache
+ ConfigParseUtil.validateClasses(classNames, loader1);
+ assertEquals(1, loader1.getLoadCount(String.class.getName()));
+ }
+
+ @SuppressWarnings("unchecked")
+ private static Cache<Object, Object> validatedClassCache() {
+ try {
+ Field cacheField =
ConfigParseUtil.class.getDeclaredField("VALIDATED_CLASS_CACHE");
+ cacheField.setAccessible(true);
+ return (Cache<Object, Object>) cacheField.get(null);
+ } catch (NoSuchFieldException | IllegalAccessException e) {
+ throw new AssertionError("Cannot access ConfigParseUtil cache
field", e);
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ private static Cache<Object, Object> innerCacheFor(ClassLoader loader) {
+ Cache<Object, Object> outer = validatedClassCache();
+ Object inner = outer.getIfPresent(loader);
+ assertNotNull("Expected an inner cache entry for loader", inner);
+ return (Cache<Object, Object>) inner;
+ }
+
+ private static int outerCacheLimit() {
+ return intConstant("MAX_CLASSLOADER_CACHE_SIZE");
+ }
+
+ private static int innerCacheLimit() {
+ return intConstant("MAX_CLASS_CACHE_PER_LOADER_SIZE");
+ }
+
+ private static int intConstant(String fieldName) {
+ try {
+ Field field = ConfigParseUtil.class.getDeclaredField(fieldName);
+ field.setAccessible(true);
+ return field.getInt(null);
+ } catch (NoSuchFieldException | IllegalAccessException e) {
+ throw new AssertionError("Cannot access ConfigParseUtil constant:
" + fieldName, e);
+ }
+ }
+
+ private static final class CountingClassLoader extends ClassLoader {
+ private final String loaderName;
+ private final java.util.Map<String, Integer> loadCounts = new
java.util.HashMap<>();
+
+ private CountingClassLoader(ClassLoader parent, String loaderName) {
+ super(parent);
+ this.loaderName = loaderName;
+ }
+
+ @Override
+ public Class<?> loadClass(String name) throws ClassNotFoundException {
+ loadCounts.merge(name, 1, Integer::sum);
+ return super.loadClass(name);
+ }
+
+ private int getStringClassLoads() {
+ return loadCounts.getOrDefault(String.class.getName(), 0);
+ }
+
+ private int getLoadCount(String className) {
+ return loadCounts.getOrDefault(className, 0);
+ }
+
+ @Override
+ public String toString() {
+ return loaderName;
+ }
+ }
+}
