Lukasz Lenart created WW-5677:
---------------------------------
Summary: Remove the remaining redundant getPackage() lookups on
the OGNL member-access path
Key: WW-5677
URL: https://issues.apache.org/jira/browse/WW-5677
Project: Struts 2
Issue Type: Sub-task
Reporter: Lukasz Lenart
Sub-task of WW-5667. Residual items found while reviewing WW-5674, which
optimised {{isClassBelongsToPackages}} and {{toPackageName}} but deliberately
left these two call sites alone as out of scope.
Both sit on the same per-access hot path WW-5674 just optimised, within twenty
lines of it, so leaving them is a visible inconsistency.
h2. 1. {{checkDefaultPackageAccess}} makes four {{getPackage()}} calls
{{SecurityMemberAccess.checkDefaultPackageAccess}} still calls
{{clazz.getPackage()}} directly, twice per class and for up to two classes:
{code:java}
if (memberClass.getPackage() == null ||
memberClass.getPackage().getName().isEmpty()) {
{code}
{{Class.getPackage()}} resolves through the defining classloader's package map
on every call. WW-5674 removed exactly this lookup from {{toPackageName}} by
switching to the cached {{Class.getPackageName()}}, so this method is now the
only remaining user of the slow form.
The condition is equivalent to {{toPackageName(memberClass).isEmpty()}} —
including for arrays and primitives, where both forms treat the class as being
in the default package and therefore block. Replacing it would also collapse
the double {{getPackage()}} evaluation per class.
Note that this method only runs when {{struts.disallowDefaultPackageAccess}} is
enabled, so the impact is limited to deployments that turn it on.
h2. 2. {{isExcludedPackageNamePatterns}} recomputes the package name per pattern
{code:java}
protected boolean isExcludedPackageNamePatterns(Class clazz) {
return excludedPackageNamePatterns.stream().anyMatch(pattern ->
pattern.matcher(toPackageName(clazz)).matches());
}
{code}
{{toPackageName(clazz)}} is evaluated inside the lambda, so it runs once per
pattern rather than once per call. Hoisting it out of the stream is a one-line
change.
The call is cheaper after WW-5674, but it is still N redundant calls.
{{struts.excludedPackageNamePatterns}} is empty by default (both pattern
constants are commented out in {{struts-excluded-classes.xml}}), so this only
affects deployments that configure it.
h2. Constraints
This is the OGNL security gate. Both changes must be behaviour-preserving, and
the equivalence of {{getPackage() == null || getPackage().getName().isEmpty()}}
with {{toPackageName(...).isEmpty()}} must be asserted by test rather than
argued — in particular for arrays, primitives, {{void}} and default-package
classes.
{{SecurityMemberAccessPackageMatchingTest}} already provides the class-shape
matrix and the frozen {{legacyToPackageName}} reference oracle to build on.
h2. Related
{{ConfigParseUtil.validatePackageNames}} evaluating {{Pattern.compile("\\s")}}
once per package name rather than once overall is a per-instantiation cost, not
a per-access one, and is tracked on WW-5675 instead.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
