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

[David Holmes]

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
>