[
https://issues.apache.org/jira/browse/WW-5630?focusedWorklogId=1024599&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1024599
]
ASF GitHub Bot logged work on WW-5630:
--------------------------------------
Author: ASF GitHub Bot
Created on: 11/Jun/26 04:37
Start Date: 11/Jun/26 04:37
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 1024599)
Time Spent: 20m (was: 10m)
> Performance Issue SecurityMemberAccess
> --------------------------------------
>
> Key: WW-5630
> URL: https://issues.apache.org/jira/browse/WW-5630
> Project: Struts 2
> Issue Type: Bug
> Components: Value Stack
> Environment: Payara 7
> Reporter: Jose Miguel
> Priority: Major
> Fix For: 6.11.0, 7.2.0
>
> Attachments: image-2026-05-20-16-37-00-000.png
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> The Security hardening introduced some performance issues in a system
> multibranch (payara) where one single action is used to process thousand of
> files per minute. The action is using exctly same calls, parameters, and
> classes every call.
> Every call is loading classes, so, blocks the ClassLoader as can be seen in
> JDK Mission Control.
> There's no way to avoid this issue, played with several confi parameters,
> with no luck.
> Checking code, is not cached, it's suggested to cache The validation of
> classes, to avoid validate again and again the same class. the classes
> mostly validated again and again are: java.lang.Process
> org.apache.struts2.ActionContext
> java.lang.Runtime
> java.lang.Thread
> java.lang.ThreadLocal
>
> public static Set<Class<?>> validateClasses(Set<String> classNames,
> ClassLoader validatingClassLoader) throws ConfigurationException {
> Set<Class<?>> classes = new HashSet<>();
> for (String className : classNames) {
> try {
> classes.add(validatingClassLoader.loadClass(className));
> } catch (ClassNotFoundException e) {
> throw new ConfigurationException("Cannot load class for exclusion/exemption
> configuration: " + className, e);
> }
> }
> return classes;
> }
>
> !image-2026-05-20-16-37-00-000.png!
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
