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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
