[
https://issues.apache.org/jira/browse/WW-5674?focusedWorklogId=1033472&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1033472
]
ASF GitHub Bot logged work on WW-5674:
--------------------------------------
Author: ASF GitHub Bot
Created on: 03/Aug/26 13:04
Start Date: 03/Aug/26 13:04
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 1033472)
Time Spent: 0.5h (was: 20m)
> Cut the per-call allocations in SecurityMemberAccess.isClassBelongsToPackages
> -----------------------------------------------------------------------------
>
> Key: WW-5674
> URL: https://issues.apache.org/jira/browse/WW-5674
> Project: Struts 2
> Issue Type: Sub-task
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Sub-task of WW-5667.
> {{SecurityMemberAccess.isClassBelongsToPackages(Class, Set)}} runs on the
> OGNL member-access hot path and allocates heavily on every call. It splits
> the package name with a regex-based {{String.split("\\.")}}, wraps the result
> in a {{List}}, then streams over the prefixes and builds a brand-new joined
> {{String}} for every one of them before probing the set. For a class in
> {{org.apache.struts2.ognl}} that is one array plus four {{String.join}}
> allocations.
> It is invoked up to four times per {{isAccessible()}} call — once for the
> excluded-package check and once for the allowlist check, for both the
> member's declaring class and the target class (see
> {{isExcludedPackageNames}}, {{isClassAllowlisted}}).
> This matches JFR sample 1 on the parent ticket:
> {noformat}
> java.lang.String.split(String)
> SecurityMemberAccess.isClassBelongsToPackages(Class, Set) :390
> SecurityMemberAccess.isExcludedPackageNames(Class) :386
> SecurityMemberAccess.isPackageExcluded(Class) :371
> {noformat}
> The same prefix-matching can be done by walking the package name from each
> {{'.'}} and probing the set with a substring, with no array, stream, or join
> allocations.
> This is the OGNL security gate, so the change must preserve exact allow/deny
> semantics. Equivalence tests are required for: the default package (empty
> package name), single-segment packages, exact matches, sub-package matches,
> and near-misses that share a character prefix but not a package boundary
> (e.g. {{org.apache.struts2x}} must not match {{org.apache.struts2}}).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
