[Nestmates] RFR: 8197395: [Nestmates] VerifyAccess.isMemberAccessible must not allow private access between legacy nested types
david.holmes at oracle.com
Mon Feb 12 00:35:02 UTC 2018
Hi Karen, Mandy,
I'm reverting back to my original change for this:
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.
to revert all changes to VerifyAccess.isSameMemberPackage and Lookup.in
On 10/02/2018 2:02 AM, Karen Kinnear wrote:
> 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:
A full annotated webrev of the nestmate changes relative to the
mainline/default code, as of a couple of weeks ago is here:
I'm generating a current, but not annotated, webrev into:
> 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
>>> 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
>> 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
> 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,
>> 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.
More information about the valhalla-dev