blackdrag commented on code in PR #2842:
URL: https://github.com/apache/groovy/pull/2842#discussion_r3884849658
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
Review Comment:
This is actually also wrong.
##########
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:
From the perspective of Selector I think this is actually using the wrong
lookup, same for the code in 443. There is a lookup object from the callsite.
This gives the access rights the caller has for the field. My favorite example
is: assume we have a Groovy module G, a module written in Groovy GM and a
module written in Java JM. GM uses JM and of course GM uses G. But why should G
have rights to read JM? the result is that even if code from GM can read JM,
making a field accessible from G can fail to do so. Access must be evaluated in
the context of the call-site using Lookup, not by trying to establish
reflective access from the Groovy module. That is why
ReflectionUtils.makeAccessible should be avoided. That is why I think adding
that field to CachedField is probably not a good idea. Lookup already handles
all the access rules, checking them manually is surplus.
The fix to replace the GroovyBugError is good I think. It also moves
potential reflective access problems to the MetaClass, but here access can
still fail.
##########
src/test/groovy/org/codehaus/groovy/transform/packageScope/DifferentPackageTest.groovy:
##########
@@ -367,7 +367,8 @@ final class DifferentPackageTest {
'''
)
}
- assert err.message =~ /Access to p.One#value is forbidden/
+ // GROOVY-12314: rejected by the type checker, no longer during class
generation
+ assert err.message =~ /No such property: value for class: p.One/
Review Comment:
access forbidden and no such property are not the same.
--
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]
