paulk-asert commented on code in PR #2842:
URL: https://github.com/apache/groovy/pull/2842#discussion_r3889223476
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
Review Comment:
Same fix: setFieldWriteHandle now uses the call-site lookup too
(5624741568). The isPublic pre-check stays only as the cheap filter for the
direct-handle fast path; anything else goes through the sender-aware adapter
path as before.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -443,13 +443,37 @@ public void chooseMeta(MetaClassImpl mci) {
MethodHandles.Lookup lookup = ((Java8)
VMPluginFactory.getPlugin()).newLookup(sender);
handle = ((CachedField) mp).asAccessMethod(lookup);
} catch (IllegalAccessException e) {
- throw new GroovyBugError(e);
+ // GROOVY-12314: refusal is an access-control outcome, not
an internal
+ // error; invoke the MetaProperty generically like any
other property
+ handle = META_PROPERTY_GETTER.bindTo(mp);
}
} else {
+ // GROOVY-12314: the effective lookup skips fields whose
access reflection
+ // cannot force, but this sender's own lookup may still reach
an inherited
+ // protected (or same-package) field, e.g. FilterReader#in
from a subclass
+ MetaProperty rawMp = mci.getMetaProperty(name);
+ if (rawMp instanceof CachedField cf && !cf.isStatic()
+ && !cf.isAccessEstablishable() &&
senderPassesJavaAccessRules(cf)) {
+ try {
+ @SuppressWarnings("removal")
+ MethodHandles.Lookup lookup = ((Java8)
VMPluginFactory.getPlugin()).newLookup(sender);
+ handle = cf.asAccessMethod(lookup);
Review Comment:
Agreed, and thanks for the module example — it made the problem concrete.
newLookup(sender) is privateLookupIn(sender, Groovy's lookup), so access was
being judged from Groovy's module: across modules the JDK drops MODULE access
from that lookup and requires the target package to be open to Groovy, which is
exactly your G/GM/JM case. The call-site lookup's lookup class is the sender,
so it carries precisely the caller's rights.
Fixed in 5624741568: the property-get path (both the effective-field case
and the retry for a field the metaclass skipped) now unreflects against
callSite.getLookup(), and senderPassesJavaAccessRules is gone — the lookup
decides, the reflective fallback still applies where it can, and the generic
MetaProperty path remains for the rest.
On isAccessEstablishable in CachedField: I've kept it, but only for the
classic Field.get path in MetaClassImpl, which has no lookup to ask. Without it
a non-public field of a strongly encapsulated class is selected as a property
and only fails on read/write (previously a GroovyBugError); with it the MOP
reports the member as missing, which is what 4.x did. It no longer influences
the indy path's decision. While doing this I found makeAccessible() recorded
success unconditionally, so one failed setAccessible made the flag report the
field reachable afterwards — also fixed in that commit.
--
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]
