[
https://issues.apache.org/jira/browse/WW-5675?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5675:
------------------------------
Fix Version/s: 7.4.0
> Stop re-parsing OGNL security config on every SecurityMemberAccess
> instantiation
> --------------------------------------------------------------------------------
>
> Key: WW-5675
> URL: https://issues.apache.org/jira/browse/WW-5675
> Project: Struts 2
> Issue Type: Sub-task
> Reporter: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
>
> Sub-task of WW-5667.
> {{SecurityMemberAccess}} is a {{Scope.PROTOTYPE}} bean
> ({{StrutsBeanSelectionProvider}}, since WW-5343). Every
> {{container.getInstance(SecurityMemberAccess.class)}} therefore constructs a
> fresh instance and re-runs all of its {{@Inject}} setters, each of which
> re-parses a raw comma-delimited configuration string from scratch.
> With the stock {{struts-excluded-classes.xml}}, a single instantiation
> re-parses roughly:
> * 16 excluded class names from {{struts.excludedClasses}} and another 16 from
> {{struts.devMode.excludedClasses}} — comma split plus classloader validation
> * 29 excluded package names from {{struts.excludedPackageNames}} and another
> 29 from {{struts.devMode.excludedPackageNames}} — comma split, {{strip}}, and
> {{validatePackageNames}}
> * the allowlist class and package sets
> * any configured excluded-package-name patterns, which are re-compiled via
> {{Pattern.compile}}
> New instances are created on the request path from at least:
> * {{OgnlValueStackFactory.createValueStack(...)}} — once per value stack;
> {{ParametersInterceptor.toNewStack}} creates an additional stack per request
> * {{OgnlUtil.createDefaultContext(Object, ClassResolver)}} — reached from
> {{setProperties}}, {{copy}}, {{getBeanMap}} and friends
> so the full configuration is rebuilt several times per request.
> This matches JFR sample 2 on the parent ticket:
> {noformat}
> HashMap.put / HashSet.add
> ConfigParseUtil.toNewPackageNamesSet(Collection, String) :138
> SecurityMemberAccess.useExcludedPackageNames(String) :446
> {noformat}
> Note that the fix proposed on the parent ticket — caching the parsed set in a
> {{SecurityMemberAccess}} field — does not address this, because the instance
> holding the field is itself discarded and rebuilt each time.
> h2. Also in scope: validatePackageNames recompiles a constant pattern per
> entry
> {{ConfigParseUtil.validatePackageNames}} evaluates {{Pattern.compile("\\s")}}
> once per package name rather than once overall:
> {code:java}
> public static void validatePackageNames(Collection packageNames) {
> if (packageNames.stream().anyMatch(s ->
> Pattern.compile("\\s").matcher(s).find())) {
> throw new ConfigurationException("Excluded package names could not be
> parsed due to erroneous whitespace characters: " + packageNames);
> }
> }
> {code}
> With the default configuration that is roughly 58 recompiles of a trivial
> pattern per {{SecurityMemberAccess}} instantiation, on top of the parsing
> work above. Hoisting the pattern to a static constant — or replacing the
> check with a plain character scan — is a small, self-contained fix, and it
> shares this ticket's root cause of doing per-instantiation work that should
> be done once.
> h2. Design note: this ticket unblocks collapsing the allowlist two-set walk
> WW-5674 merged the two allowlist walks in {{isClassAllowlisted}} by adding a
> two-set helper:
> {code:java}
> static boolean isPackageBelongsToPackages(String packageName, Set first, Set
> second)
> {code}
> The cleaner shape is a single set holding the union of
> {{ALLOWLIST_REQUIRED_PACKAGES}} and {{allowlistPackageNames}}, precomputed
> once. That would delete the two-set parameter and the three-argument overload
> entirely, leaving one set and one walk.
> It was rejected in WW-5674 for two reasons, and _this ticket removes the
> first and largely removes the second_:
> h1. Under the current prototype scope, a precomputed union field is rebuilt
> on every instantiation — several times per request — adding to exactly the
> cost this ticket is about. Once the parsed configuration is shared rather
> than per-instance, the union becomes free.
> h1. The union would need initialising in two places: at the field
> declaration, for the case where the {{@Inject}} setter never fires because no
> allowlist is configured, and again in the setter. Duplicated initialisation
> here is a _fail-open_ hazard — get it wrong and
> {{ALLOWLIST_REQUIRED_PACKAGES}} silently drops out of the allowlist with
> nothing failing loudly. Whatever this ticket does to give the configuration a
> single well-defined construction point makes that much safer to get right.
> Worth doing as part of this ticket rather than leaving it to WW-5678, since
> the enabling change lands here. Note that it would also resolve WW-5678's
> first item for free, by removing the package-private overload that shares a
> name with a public method.
> h2. Constraints
> The OGNL allow/deny semantics must not change. The dev-mode configuration
> switchover in {{useDevModeConfiguration()}} must keep working. Per-instance
> mutable state set at request time by
> {{ParametersInterceptor.applyMemberAccessProperties}}
> ({{useAcceptProperties}} / {{useExcludeProperties}}) must stay per-instance —
> this is why the bean was made prototype-scoped in the first place, so any
> move back towards sharing has to account for it.
> h2. Related
> * WW-5674 — the per-access half of the parent report
> ({{isClassBelongsToPackages}} allocations). Handled separately; it does not
> move this cost.
> * WW-5677 — remaining redundant {{getPackage()}} lookups on the per-access
> path. Same file, different root cause.
> * WW-5678 — helper naming and visibility cleanup for 8.0.0. Its first item
> may be resolved for free here, see the design note above.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
