Draft JVMS changes for Nestmates

[David Holmes]

Hi Dan,

<trimming>

On 21/04/2017 4:43 AM, Dan Smith wrote:
>>>>> Security risk
>>>>>
>>>>> The JEP text should acknowledge that, while this does allow compilers to grant finer-grained access to members shared by nestmates, it also pushes compilers to grant broader access to members that were previously kept private. It's a trade-off, and presumably a good one because nestmates are completely trusted, while package-mates might sometimes be suspect.
>>>>
>>>> I'm not following you here. What broader access is being granted to members that were previously kept private? This effort allows private-only access to members that were previously package-accessible.
>>>
>>> Old approach is to widen access to private members to the package level on a per-member basis.
>>>
>>> New approach is to widen access to private members to the nest level on a per-class basis.
>>>
>>> Nest is narrower than package (good!), but per-class is more permissive than per-member (bad). Some members that are compiled as totally private in JDK 9 will be incidentally shared with all nest members in <future JDK version>.
>>
>> I'm not sure how you are coming at this. For a private member in 9 to be incidentally shared in <future-JDK> the class of the member has to be added to a nest. With no context as to how this may happen I can't really comment further. From the Java language perspective if the class is in a nest then such access to private members already exists.
>
> Example:
>
> class Outer {
>     private int x;
>     private int y;
>     class Inner { private int z = x; }
> }
>
> In JDK 9, this compiles to:
>
> class Outer {
>     private int x;
>     private int y;
>     Outer() {}
>     static int access$000(Outer arg) { return x; }
> }
>
> class Outer$Inner {
>     private int z;
>     final Outer this$0;
>     Outer$Inner(Outer arg) {
>         this$0 = arg;
>         z = Outer.access$000(this$0);
>     }
> }
>
> In <future-JDK>, this compiles to:
>
> class Outer [NestMembers:{Outer$Inner}] {
>     private int x;
>     private int y;
>     Outer() {}
> }
>
> class Outer$Inner [MemberOfNest:Outer] {
>     private int z;
>     final Outer this$0;
>     Outer$Inner(Outer arg) {
>         this$0 = arg;
>         z = this$0.x;
>     }
> }
>
> Notice that in JDK 9, Outer shares access to y with nobody, while in <future-JDK>, Outer shares access to y with its NestMembers.

Only because in JDK9 javac doesn't generate all potential accessors 
(which it could). The current language rules say that y is accessible to 
nestmates. As none try to actually access it javac doesn't bother 
generating the accessor.

>>>> In:
>>>>
>>>> "If getfield or putfield is used to access a protected field referenced in a superclass ..."
>>>>
>>>> and following, you changed "declared" to read "referenced". Again I do not understand what this is supposed to mean. It is the declaration site of the field or method that is needed to determine whether that member is in the same or another package. To me "declared in a superclass ..." was exactly correct. Your note does not make the change any clearer to me.
>>>
>>> Good catch. It's important that the rule be limited to cases that reference a superclass*; but it's also important that the declaring class be used for the package check.
>>>
>>> (*Here's why: if I reference my subclass, and that resolves to a field declared in my superclass, the verifier will perform no check, because it may not know that the referenced class is a subclass.)
>>>
>>> Revised:
>>>
>>> "If `getfield` or `putfield` is used to access a `protected` field by reference to a superclass, and that field is declared in a different run-time package than the current class or interface, then the type of the class instance being accessed must be assignable to the current class or interface."
>>
>> I still think "declared in" is the correct terminology. There's a nuance to how you are expressing this that I'm just not getting.
>
> Example:
>
> public class p1.A {
>     protected int i;
> }
>
> public class p2.B extends p1.A {
>     int test(p2.C arg) {
>         aload_1
>         getfield p2.C.i:I
>         ireturn
>     }
> }
>
> public class p2.C extends p2.B {
> }
>
> When the verifier checks the getfield, it will apply the rules in 4.10.1.8. The check will trivially pass, because p2.C is not a superclass of the current class (p2.B). There will be no test that p2.C is "assignable to the current class or interface". Thus, p2.C need not be loaded during verification.
>
> Strictly reading the old rule, "field declared in a superclass" means that the verifier would have to load p2.C and resolve (or simulate resolution of) p2.C.i:I in order to determine whether the field is declared in a superclass of the current class (p2.B).

I'm not 100% sure of what needs loading when, but don't know why this is 
an issue either. The package check is needed to see if the more specific 
protected-access check is needed. As p2.C.i actually refers to the 'i' 
declared in p1.A, then we should be checking if B and A are in the same 
package.

>>>> 6.5 invokespecial
>>>>
>>>> You note regarding the IllegalAccessError:
>>>>
>>>> "This replaces a VerifyError previously specified by 4.9.2. The check must be delayed until after resolution in order to determine whether the referenced method is private."
>>>>
>>>> We know the access flags for the referenced method at verification time - shouldn't that be the sole basis for the verifier check? Afterall it is verifying the static properties of the classfile and bytecode. If the actual resolved method has different access to the referenced method then that may lead to ICCE (depending on the exact nature of the change - a private method made public may not be an issue for example).
>>>
>>> Example:
>>>
>>> invokespecial SomeOtherClass.foo(I)V
>>>
>>> Verification relies on the descriptor to tell it that this method expects an int and returns nothing. Those kinds of checks can be performed locally.
>>>
>>> The descriptor doesn't tell you if the method is private. Only way to find out is to resolve SomeOtherClass, then resolve "foo(I)V".
>>>
>>> If we wanted to do the check at verification time, we'd have to load every class named by an invokespecial instruction.
>>
>> Now I am confused. I thought the access modifier of the method described by the Method_Ref was available to us. If it is not then how can all these verification rules be expressed in terms of non-private when we do not have the means at verification time to determine if the access is private or not ???
>
> The rule is:
> 1) if the referenced method name is not <init> (cheap), and
> 2) if the referenced class name is the name of a superclass or direct superinterface (cheap—superclass chain is already loaded), and
> 3) if the loaded referenced class actually is a superclass or direct superinterface (almost always true and cheap, unless there's a name clash), and
> 4) if the resolved method is not private (pretty cheap, because referenced class is already loaded)
> *then* check that the stack type is assignable to the current class type.
>
> This is, essentially, the same logic being employed by the old protected check: if the reference is to a superclass, find the field/method and decide if it's protected; if the reference is to some other class, don't worry about it.

So here's the problem, in the spec 5.4 states:

"Linking a class or interface involves verifying and preparing that 
class or interface, its direct superclass, its direct superinterfaces, 
and its element type (if it is an array type), if necessary. Resolution 
of symbolic references in the class or interface is an optional part of 
linking."

So resolution is _optional_ at link time! But your updated spec requires 
resolution to happen before we can complete verification!

When we do the verification of invokespecial in the VM it is before 
resolution and we do not know if the target method is private or not.

David
-----

> —Dan
>