[Nestmates] RFR: 8197395: [Nestmates] VerifyAccess.isMemberAccessible must not allow private access between legacy nested types
karen.kinnear at oracle.com
Fri Feb 9 16:02:46 UTC 2018
Could you do us a big favor?
Could you possibly send two web revs next round?
A webrev for the changes relative to the current nestmate repo is fine
A webrev relative to the valhalla parent repo would be more helpful to me - since I am really trying
to see the sum of your changes, not increment.
more below …
1. VerifyAccess.java isMemberAccessible changes look good.
> On Feb 9, 2018, at 1:46 AM, David Holmes <david.holmes at oracle.com> wrote:
> Hi Mandy,
> Thanks for looking at this!
> On 9/02/2018 10:13 AM, mandy chung wrote:
>> On 2/7/18 11:53 PM, David Holmes wrote:
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8197395
>>> webrev: http://cr.openjdk.java.net/~dholmes/8197395/webrev/
>>> With the fix for JDK-8196320 however, isSamePackageMember() (now renamed to areNestMates) first checks Reflection.areNestMates() and then if that is false, the old enclosing-class check. Consequently the check can now return true for both nestmate classfiles and earlier classfiles! That is wrong.
>>> To fix this we replace the call to VerifyAccess.areNestMates() with Reflection.areNestMates().
>> The fix looks fine. Thanks for the clear description of the issue. These two methods could cause confusion to the reader and always need to go the javadoc. What about renaming VerifyAccess::areNestMates to areNestmatesOrPreNestmates?
> Yes this needs renaming. We need to reserve "areNestMates" to mean they have matching NestHost attributes - as per Class.areNestMates() and Reflection.areNestMates(). I'm tempted to not only revert back to the original isSamePackageMember() but to also restore the original implementation with no Reflection.areNestMates() call - as nestmates will still pass the enclosing-class check. That also avoids the penalty that applies to pre-nestmate classfiles, by avoiding the guaranteed-to-fail nestmate check. My only reservation is that in the future, when nestmates need not originate in source code, and so the enclosing class check may not apply, then we would need to restore the direct nestmate check. - and again be confronted with an awkwardly named method.
2. I would put back isSamePackageMember
(If you want to rename this - it really is something like sharesOutermostEnclosingClass)
and I would leave the implementation alone
I am in support of reverting back to the original isSamePackageMember() with the original implementation with no
Reflection.areNestMates(). I believe this will continue to allow existing and future enclosing-classes to keep
their existing behavior.
In the future, we do not want the Lookup.in behavior for nestmates in general - that is a workaround today
for the partial creation of trampolines - those that are statically referenced in the source file, and only
applies to outer/inner classes not to all future uses of nestmates.
We don’t want the workaround for nestmates that are not inner/outer classes - we want the explicit check
on a private member, not the workaround for creating a Lookup with private mode.
I would put back the original check for isSamePackageMember() with the original meaning here.
hope this is clearer,
> I think I will turn this around and rename the method to havePrivateAccess(). I'm also going to reverse the order of checking so that the actual nestmate check comes last.
> Updated webrev: http://cr.openjdk.java.net/~dholmes/8197395/webrev.v2/ <http://cr.openjdk.java.net/~dholmes/8197395/webrev.v2/>
>>> There is also a temporary psuedo-assertion verifying that if we've granted private nestmate access then refc==defc (which should be assured by the resolution of the member being accessed).
>> Why do you want myassert?
> I wanted an always-on assertion independent of the language assert statement. This will either go in the final version of be replaced by a regular assert.
More information about the valhalla-dev