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