[Nestmates] RFR: 8197395: [Nestmates] VerifyAccess.isMemberAccessible must not allow private access between legacy nested types

[Karen Kinnear]

David,

Great way to resolve this. Thank you for splitting up these issues.

Thank you for the webrev relative to the default branch - that makes it much easier for
me to see where we are in nestmates relative to no nestmates. That was the only
additional webrev I was looking for.

You can Mandy can work out how to do the assertion - I appreciate having the check there.
I don’t know where else that is enforced with MethodHandles - but if it is enforced elsewhere
then this would not be needed.

thanks,
Karen

> On Feb 11, 2018, at 7:35 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Karen, Mandy,
> 
> I'm reverting back to my original change for this:
> 
> http://cr.openjdk.java.net/~dholmes/8197395/webrev/
> 
> with an added comment that myassert is a temporary check. That way this change addresses only the issue it was intended to address - that legacy classfiles don't get additional access capabilities.
> 
> I've filed:
> 
> https://bugs.openjdk.java.net/browse/JDK-8197539
> 
> to revert all changes to VerifyAccess.isSameMemberPackage and Lookup.in behaviour.
> 
> On 10/02/2018 2:02 AM, Karen Kinnear wrote:
>> David,
>> 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.
> 
> Here's the webrev of the changed files in this fix (only 1) compared to the default branch:
> 
> http://cr.openjdk.java.net/~dholmes/8197395/webrev.vs_mainline/
> 
> A full annotated webrev of the nestmate changes relative to the mainline/default code, as of a couple of weeks ago is here:
> 
> http://cr.openjdk.java.net/~dholmes/8010319/nestmates-webrev.htm
> 
> I'm generating a current, but not annotated, webrev into:
> 
> http://cr.openjdk.java.net/~dholmes/8010319/webrev.Feb-12-2018
> 
> Thanks,
> David
> -----
> 
>> more below …
>> 1. VerifyAccess.java isMemberAccessible changes look good.
>>> On Feb 9, 2018, at 1:46 AM, David Holmes <david.holmes at oracle.com <mailto: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.
>> 3. MethodHandles.java
>> I would put back the original check for isSamePackageMember() with the original meaning here.
>> hope this is clearer,
>> Karen
>>> 
>>> 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/
>>> 
>>>>> 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.
>>> 
>>> Thanks,
>>> David
>>> 
>>>> Mandy