You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@struts.apache.org by "lukaszlenart (via GitHub)" <gi...@apache.org> on 2026/08/03 11:46:15 UTC
[PR] WW-5674 Make SecurityMemberAccess package matching allocation-free [struts]
lukaszlenart opened a new pull request, #1830:
URL: https://github.com/apache/struts/pull/1830
Fixes [WW-5674](https://issues.apache.org/jira/browse/WW-5674), a sub-task of [WW-5667](https://issues.apache.org/jira/browse/WW-5667).
## Background
WW-5667 reports OGNL security checks consuming 9% of RUNNABLE CPU samples in a 2-minute JFR profile under load. The report contains two stack samples pointing at two independent problems, so it was split:
- **WW-5674 (this PR)** — the per-OGNL-access cost in `isClassBelongsToPackages` (sample 1).
- **[WW-5675](https://issues.apache.org/jira/browse/WW-5675)** — repeated config re-parsing caused by `SecurityMemberAccess` being a `Scope.PROTOTYPE` bean (sample 2). This is the dominant cost and is **not** addressed here.
Note that the fix proposed on WW-5667 (caching the parsed `Set` in a `SecurityMemberAccess` field) addresses neither problem — it does not touch this hot path, and it cannot help sample 2 because the instance holding the field is discarded and rebuilt on each container lookup.
## What changed
`isClassBelongsToPackages` ran on the OGNL member-access path and, for an N-segment package, allocated a `String[]` plus its N substrings from `split`, a `List.of` wrapper, an `IntStream` pipeline, N `subList` views, and N `StringJoiner`-built strings. It is invoked up to four times per `isAccessible()` call.
1. `toPackageName` now uses the cached `Class.getPackageName()` instead of `Class.getPackage().getName()`, which resolves through the defining classloader's package map on every call.
2. The prefix construction is replaced by an in-place index walk, extracted into a package-private `isPackageBelongsToPackages` so the logic is testable against package-name shapes no real `Class` can produce.
3. `isClassAllowlisted` walked the same package name twice, once per allowlist set. A package-private two-set overload now probes both sets at each prefix, halving that work.
## Behaviour is unchanged
This is the OGNL security gate, so the change is a pure optimisation with **zero** semantic change, proven rather than asserted:
- The test suite keeps a frozen, verbatim copy of the replaced implementation as a differential oracle and asserts old and new agree across a matrix of package-name shapes and candidate sets.
- Package-boundary safety is pinned explicitly: `org.apache.struts2x` must not match an `org.apache.struts2` entry.
- The obscure default-package edge is pinned: `struts.excludedPackageNames="."` strips to `""`, which still excludes default-package classes.
- Equivalence was additionally verified exhaustively over all 3280 strings on `{a, b, .}` up to length 7. The only divergences are package names ending in `.`, which `Class.getPackageName()` cannot produce.
- `toPackageName` equivalence was verified over 26 class shapes on the Java 17 target, including hidden classes, JDK proxies, and classes defined by a classloader that never calls `definePackage()`.
**Array and primitive package semantics are deliberately unchanged.** `getPackageName()` would resolve `String[]` to `java.lang` and `java.io.File[]` to `java.io`, where the current code yields `""`. That change is bidirectional — it tightens the exclusion list but *loosens* the allowlist, since arrays of allowlisted-package types would become reachable without an explicit `struts.allowlist.classes` entry. As the allowlist is the primary OGNL defence in 7.x and is on by default, that question is deferred to its own ticket. The `isArray() || isPrimitive()` guard preserves current behaviour exactly.
## Testing
Full `core` module suite green: **3158 tests, 0 failures, 0 errors**. `SecurityMemberAccessTest` passes unmodified — no existing assertion was changed.
--
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
Re: [PR] WW-5674 Cut the per-call allocations in SecurityMemberAccess package matching [struts]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #1830:
URL: https://github.com/apache/struts/pull/1830#issuecomment-5167857066
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=1830) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=1830&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=1830&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=1830&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=1830&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=1830&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=1830)
--
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
Re: [PR] WW-5674 Cut the per-call allocations in SecurityMemberAccess package matching [struts]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #1830:
URL: https://github.com/apache/struts/pull/1830#issuecomment-5166796611
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=1830) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=1830&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=1830&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=1830&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=1830&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=1830&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=1830)
--
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
Re: [PR] WW-5674 Cut the per-call allocations in SecurityMemberAccess package matching [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #1830:
URL: https://github.com/apache/struts/pull/1830#discussion_r3704096000
##########
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:
##########
@@ -387,10 +390,47 @@ protected boolean isExcludedPackageNames(Class<?> clazz) {
}
public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> matchingPackages) {
- List<String> packageParts = List.of(toPackageName(clazz).split("\\."));
- return IntStream.range(0, packageParts.size())
- .mapToObj(i -> String.join(".", packageParts.subList(0, i + 1)))
- .anyMatch(matchingPackages::contains);
+ return isClassBelongsToPackages(clazz, matchingPackages, emptySet());
+ }
+
+ /**
+ * Tests the class's package against two sets in a single walk. Equivalent to calling
+ * {@link #isClassBelongsToPackages(Class, Set)} once per set and OR-ing the results, but
+ * walks the package name only once.
+ *
+ * @param clazz the class whose package is tested
+ * @param first the first set of package names to match against
+ * @param second the second set of package names to match against
+ * @return {@code true} if the class's package or any parent package is in either set
+ */
+ static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> first, Set<String> second) {
+ return isPackageBelongsToPackages(toPackageName(clazz), first, second);
+ }
+
+ /**
+ * Tests whether the given package name, or any of its parent packages, is present in either
+ * set. Walks the name in place rather than building the full prefix list, since this runs on
+ * the OGNL member-access path. Shortest prefix first, so broad entries such as {@code java.io}
+ * short-circuit earliest.
+ *
+ * @param packageName the package name to test, empty for the default package
+ * @param first the first set of package names to match against
+ * @param second the second set of package names to match against
+ * @return {@code true} if the package or any parent package is in either set
+ */
+ static boolean isPackageBelongsToPackages(String packageName, Set<String> first, Set<String> second) {
+ if (first.isEmpty() && second.isEmpty()) {
+ return false;
+ }
+ int idx = packageName.indexOf('.');
+ while (idx != -1) {
+ String prefix = packageName.substring(0, idx);
+ if (first.contains(prefix) || second.contains(prefix)) {
+ return true;
+ }
+ idx = packageName.indexOf('.', idx + 1);
+ }
+ return first.contains(packageName) || second.contains(packageName);
}
Review Comment:
Fair catch, and corrected — "allocation-free" overclaimed. The walk still creates one `substring` per package level; what it removes is everything around that: the `String[]`, the `List.of` wrapper, the `IntStream` pipeline, the N `subList` views and the N `String.join` results.
The PR title and the Jira summary are now "cut the per-call allocations", the same wording is fixed in the spec and plan headings, and the body states the distinction explicitly.
I deliberately did not chase true allocation-freedom. The two routes there are scanning the candidate set with `startsWith` plus a package-boundary check, which is O(set size) and would regress deployments allowlisting many packages, or a prefix trie, which is a lot of machinery and new failure modes for a ~30-entry set on a security-critical path. Neither looked worth it against one short-lived allocation per package level.
--
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
Re: [PR] WW-5674 Cut the per-call allocations in SecurityMemberAccess package matching [struts]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #1830:
URL: https://github.com/apache/struts/pull/1830#issuecomment-5167424308
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=1830) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=1830&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=1830&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=1830&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=1830&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=1830&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=1830)
--
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
Re: [PR] WW-5674 Cut the per-call allocations in SecurityMemberAccess package matching [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart merged PR #1830:
URL: https://github.com/apache/struts/pull/1830
--
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
Re: [PR] WW-5674 Cut the per-call allocations in SecurityMemberAccess package matching [struts]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #1830:
URL: https://github.com/apache/struts/pull/1830#issuecomment-5169911309
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=1830) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=1830&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=1830&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=1830&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=1830&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=1830&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=1830)
--
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
Re: [PR] WW-5674 Make SecurityMemberAccess package matching allocation-free [struts]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1830:
URL: https://github.com/apache/struts/pull/1830#discussion_r3703858249
##########
core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java:
##########
@@ -387,10 +390,47 @@ protected boolean isExcludedPackageNames(Class<?> clazz) {
}
public static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> matchingPackages) {
- List<String> packageParts = List.of(toPackageName(clazz).split("\\."));
- return IntStream.range(0, packageParts.size())
- .mapToObj(i -> String.join(".", packageParts.subList(0, i + 1)))
- .anyMatch(matchingPackages::contains);
+ return isClassBelongsToPackages(clazz, matchingPackages, emptySet());
+ }
+
+ /**
+ * Tests the class's package against two sets in a single walk. Equivalent to calling
+ * {@link #isClassBelongsToPackages(Class, Set)} once per set and OR-ing the results, but
+ * walks the package name only once.
+ *
+ * @param clazz the class whose package is tested
+ * @param first the first set of package names to match against
+ * @param second the second set of package names to match against
+ * @return {@code true} if the class's package or any parent package is in either set
+ */
+ static boolean isClassBelongsToPackages(Class<?> clazz, Set<String> first, Set<String> second) {
+ return isPackageBelongsToPackages(toPackageName(clazz), first, second);
+ }
+
+ /**
+ * Tests whether the given package name, or any of its parent packages, is present in either
+ * set. Walks the name in place rather than building the full prefix list, since this runs on
+ * the OGNL member-access path. Shortest prefix first, so broad entries such as {@code java.io}
+ * short-circuit earliest.
+ *
+ * @param packageName the package name to test, empty for the default package
+ * @param first the first set of package names to match against
+ * @param second the second set of package names to match against
+ * @return {@code true} if the package or any parent package is in either set
+ */
+ static boolean isPackageBelongsToPackages(String packageName, Set<String> first, Set<String> second) {
+ if (first.isEmpty() && second.isEmpty()) {
+ return false;
+ }
+ int idx = packageName.indexOf('.');
+ while (idx != -1) {
+ String prefix = packageName.substring(0, idx);
+ if (first.contains(prefix) || second.contains(prefix)) {
+ return true;
+ }
+ idx = packageName.indexOf('.', idx + 1);
+ }
+ return first.contains(packageName) || second.contains(packageName);
}
Review Comment:
Despite the PR title/wording calling this \"allocation-free\", the new implementation still allocates a new String for each prefix via substring (Java 17 substring creates a new String). If the intent is truly allocation-free matching, this needs a different approach (e.g., a specialized data structure keyed by the original String/char[] with prefix hashing) or the PR/docs/title should be updated to describe it as \"allocation-reduced\" rather than \"allocation-free\".
--
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