WW-5677 Remove the remaining redundant getPackage() lookups on the OGNL member-access path - #1859
Merged
Merged
Conversation
…ess path WW-5674 replaced Class.getPackage() with the cached Class.getPackageName() in toPackageName, but deliberately left two neighbouring call sites alone as out of scope. Both sit on the same per-access path within twenty lines of it. checkDefaultPackageAccess still tested `getPackage() == null || getPackage().getName().isEmpty()`, which resolves through the defining classloader's package map twice per class, for up to two classes per access. It now tests toPackageName(clazz).isEmpty(). The two forms agree for every class shape: getPackage() is null for arrays, primitives and void, and names the unnamed package with the empty string, all of which toPackageName reports as empty. isExcludedPackageNamePatterns evaluated toPackageName inside the lambda, so it re-resolved the package name once per configured pattern. It is now resolved once per call. Both changes are behaviour-preserving. Per the ticket, the equivalence is asserted rather than argued: defaultPackageConditionMatchesLegacyAcrossClassShapes runs the replaced condition, frozen verbatim as an oracle that calls getPackage() directly, against the new one over the existing class-shape matrix -- arrays, primitives, void, a default-package class, a lambda and a JDK proxy. Two behavioural tests cover the gate itself: a named-package class still passes, and an array target, the shape most likely to break the equivalence, stays blocked. All three tests were mutation-checked. Resolving arrays to java.lang fails the equivalence test and the array-target test; inverting the member-class condition fails the named-package test. Neither path runs by default -- checkDefaultPackageAccess only when struts.disallowDefaultPackageAccess is enabled, and the pattern loop only when struts.excludedPackageNamePatterns is configured, both commented out in struts-excluded-classes.xml. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Fixes WW-5677
Sub-task of WW-5667. WW-5674 replaced
Class.getPackage()with the cachedClass.getPackageName()insidetoPackageName, but deliberately left two neighbouring call sites alone as out of scope. Both sit on the same per-access path, within twenty lines of it. This finishes the job. No behaviour change.1.
checkDefaultPackageAccessmade fourgetPackage()calls…and the same for
targetClass.Class.getPackage()resolves through the defining classloader's package map on every call, and the old condition evaluated it twice per class, for up to two classes per access.The two forms agree for every class shape:
getPackage()returnsnullfor arrays, primitives andvoid, and names the unnamed package with the empty string — all of whichtoPackageNamereports as empty.2.
isExcludedPackageNamePatternsrecomputed the package name per patterntoPackageNamewas evaluated inside the lambda, so it ran once per configured pattern. Now once per call.Testing
This is the OGNL security gate, so per the ticket the equivalence is asserted rather than argued:
defaultPackageConditionMatchesLegacyAcrossClassShapesruns the replaced condition — frozen verbatim aslegacyDefaultPackageCondition, which callsgetPackage()directly and never delegates to production code — against the new one over the existingclassShapes()matrix: arrays, primitives,void, a default-package class, a lambda and a JDK proxy.testDefaultPackageAccessPermitsNamedPackageClass— a named-package class still passes the gate withstruts.disallowDefaultPackageAccess=true.testDefaultPackageAccessBlocksArrayTarget— an array target, the shape most likely to break the equivalence, stays blocked. The member declares injava.lang, so only the target branch can block.All three were mutation-checked rather than assumed non-vacuous:
java.langdefaultPackageConditionMatchesLegacyAcrossClassShapes,testDefaultPackageAccessBlocksArrayTarget(plus the two WW-5674 equivalence tests)testDefaultPackageAccessPermitsNamedPackageClass(plus the existingtestDefaultPackageExclusionSetting)mvn test -DskipAssembly -pl core— 3190 tests, 0 failures, 0 errors.Scope deliberately not taken
LOG.warncalls incheckExclusionListalso callgetPackage(), but only on the deny path, so there is no hot-path value — and switching them would change the log text frompackage java.iotojava.io.isPackageExcludedinto both helpers. That would remove the last redundant call, butisExcludedPackageNames/isExcludedPackageNamePatternsareprotectedand subclasses may override them, so changing their signatures is source-breaking. WW-5678 owns that cleanup for 8.0.0.toPackageNameis now a cached field read plus a branch, so calling it twice is negligible.Impact
Neither path runs by default:
checkDefaultPackageAccessonly whenstruts.disallowDefaultPackageAccessis enabled, and the pattern loop only whenstruts.excludedPackageNamePatternsis configured — both pattern constants are commented out instruts-excluded-classes.xml. This is a consistency fix for deployments that do enable them, not where the WW-5667 9% lives; that was WW-5675.🤖 Generated with Claude Code