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

[David Holmes]

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.

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