Draft JVMS changes for Nestmates

Wed Apr 19 06:40:46 UTC 2017

Hi Dan,

First, thanks for getting to this so quickly! Much appreciated.

On 19/04/2017 4:42 AM, Dan Smith wrote:
> Hi, all.
>
> I've uploaded a draft of JVMS changes for JEP 181 "Align JVM Checks with Java Language Rules for Nested Classes" to:
> http://cr.openjdk.java.net/~dlsmith/private-access.html
>
> Some comments below on my thinking in drafting the spec text, and on the JEP generally.
>
> JEP name
>
> I don't know how permanent JEP names are supposed to be, but I'd prefer a different name at this point. Something like: "Expanded JVM Access to Private Members"—shorter, focused on the feature itself rather than its relationship to the Java language. Or maybe "Class Nests for Access to Private Members".

The intent of the name is to aid in the contextual positioning of this 
JEP ie address why do we even need this. The JEP addresses an 
inconsistency between access controls in the Java programming language 
and access controls in the JVM. The primary goal here is to align the 
two, while at the same time allowing for additional usage of nestmates 
in the future.

> Terminology
>
> The term "nest" is nice because it's short and mostly unspoiled by overloading in this context (I think?); it's not great because it's informal and doesn't mean anything the first time you hear it. I thought about something more clinical like "access control context", but I'm not convinced that's an improvement. How do others feel?

I think John has been talking about "nests" for at least 4 years now. :) 
It fits nicely with the existing terminology of "nested types" - it 
seems quite natural that "nested types" form a "nest". I have no problem 
with it being "informal" in some sense, but we are using it to define a 
specific relationship between certain types.

> The JEP uses "nest top" to describe the class that nest members reference; I prefer "host class", which better describes the class's role and isn't tied to the Java "top level class" concept. I know we use "host class" internally in Hotspot, perhaps when working with anonymous classes (of the JVM flavor), but I think in that context it will ultimately mean the same thing? Are we comfortable repurposing the term in this way?

Given that a nest-top type must be a top-level type, I think nest-top 
fits perfectly. I dislike "host class" because that is already used for 
the quite different case of VMACs. I can live with other names but I 
think they should incorporate "nest" eg. nest-holder, nest-host. But I 
think nest-top is an ideal match. (And no I didn't invent this 
terminology :) ).

> I follow Brian's model when it comes to nest membership (5.4.4): every class belongs to a nest, possibly (in the absence of MemberOfNest) the nest hosted by itself. Many nests are singletons without any associated explicit attributes.

I have not yet seen a follow up from John as to whether we require or 
just allow, an empty NestMembers attribute to indicate a singleton nest 
member.

> Verification of MemberOfNest
>
> I include a discussion block about different options of validating MemberOfNest. I think the consensus, and my preference, is to do it during verification of the member class. (NestMembers, on the other hand, is never validated, except as a side-effect of checking MemberOfNest.)

Given verification is optional the current approach is to validate 
during linking just prior to the verification step. Whether that can 
conceptually be considered part of verification I am unsure.

> Verification of invokespecial
>
> Allowing invokespecial to refer to classes other than the current class and its supers is a significant change. I noticed and relied heavily on the parallel with invokevirtual making protected method calls. So I tried to make the two as similar as possible. In a few places, the treatment of protected methods doesn't seem ideal, and rather than trying to mirror that with non-private invokespecial, I modified the protected method treatment.

This concerns me because protected-access is a somewhat complex/messy 
issue so aligning private-access with it seems to go in the wrong 
direction. Cleaning up the protected access rules, while perhaps 
desirable, is technically out-of-scope for this JEP in my opinion.

> The "protected check" of verification, in particular, was a mess before, and I think I've made it a lot more manageable, and compatible with a parallel rule for invokespecial. I could use some feedback on exactly what Hotspot's verifier has been doing here, though, since I'm pretty sure it didn't match the old specified rules.

Might be better to take this up separately.

> MethodHandle resolution
>
> The spec (5.4.3.5) is vague about what errors can occur during MethodHandle resolution. I assume any linkage error specified for the instruction can also occur via MethodHandle resolution, and that will include failures due to invokespecial improperly referencing a class.

This is an area I have least understanding of, but this seems to cover it:

"In each step, any exception that can be thrown as a result of failure 
of resolution of a class or interface or field or method reference can 
be thrown as a result of failure of method handle resolution."

> Dynamic checking of invokespecial receivers
>
> When invokespecial involves interface types, the verifier can't guarantee that receiver objects actually implement that interface (JDK-8134358). It's an open question whether this is really a problem, but I included a tentative fix in the invokespecial runtime rules.

This seems out of scope in the general case, but there may be nestmate 
specific actions required.

> Compiler changes
>
> The JEP text can't seem to decide if compiler changes are part of it, or a follow-up exercise. I think we're best off explicitly including the compiler changes, which will provide opportunities for design validation and testing.

Not sure what gives you that impression, but javac changes are an 
essential part of this. Lacking a compiler spec I assume this will be 
handled informally. But the attribute definitions (see comments below) 
should dictate what a source compiler is required to do.

> API changes
>
> I haven't tried to address changes that need to be made to APIs. Somebody will need to. For example, Lookup.findSpecial probably needs to make adjustments to account for private members in nestmates, and to parallel the new verification/linkage rules (e.g., follow Lookup.findVirtual in restricting the receiver type).

API changes will be addressed as each API needs to be modified.

> 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.

> (I guess this argument really ought to be made from the top: declaring things with package access is worse than inventing a new level of access because ________.)
>
> —Dan

Now looking at specific proposed changes to the spec wording ...

4.7 Attributes

Regarding the Exceptions attribute, it seems to be used only to store 
data to be passed back to the libverify and the reflection API.

Regarding the definitions of NestMembers and MemberOfNest ... I modelled 
the definitions in:

https://bugs.openjdk.java.net/browse/JDK-8177020

based on those for the innerclass (and related) attributes. I'm not sure 
why you moved away from those definitions as they pinned down exactly 
when these attributes are expected to appear. Without those parts the 
current definitions seem optional - ie they define what the attribute 
means if present, but they do not require its presence. I do not believe 
these attributes should be optional for classfile version 54, but 
required whenever a nest does exist. A java source compiler writer 
should look at these definitions and know when they must generate these 
attributes.

---

4.7.26 The MemberOfNest Attribute

"the class is implicitly a member of its own nest"

I suggest changing "a" to "the sole". Otherwise "a" implies there may be 
other members. As these are static properties being defined I don't 
think we need to be accounting for some future dynamic expansion of a nest.

---

4.9.2 Structural Constraints

The changes to invokespecial seem okay, but I have a problem with 
terminology. In:

"If invokespecial is used to invoke a non-private, non-<init> method, 
referenced via a superclass or a direct superinterface, ..."

what does "referenced via" mean?

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.

---

4.10.1 Verification by Type Checking

Does the order in the rules mandate the order in which the VM must 
actually check things? classHasValidNest seems much more complex than 
the actual runtime process of validation (though it may just be exposing 
distinct steps that are lumped together at runtime).

---

4.10.1.8 Type Checking for Restricted Member References

I'm not very good at reading or understanding these rules, but I'm 
surprised that the invokespecial rules seem to make no mention of 
nestmates at all:

"A method reference is allowed by an invokespecial instruction if it is 
not a restricted invokespecial reference, ..."

This seems to open things up to all private method references??

---

5.4.4 Access Control

I don't think you need to restate:

"A class with a MemberOfNest attribute belongs to the nest hosted by the 
referenced host class.

A class without a MemberOfNest attribute implicitly belongs to a nest 
hosted by the class itself. (If the class also lacks a NestMembers 
attribute, then the nest has only one member.)"

The rule is simply stated as is: "belonging to the same nest as D" - 
belonging to the same nest is, or should be, already defined elsewhere.

I'm avoiding commenting on the protected access changes but do want to 
raise a concern that there are no changes to Method resolution (5.4.3.3) 
yet resolution relies on the definition of access control to 
prune/discard inaccessible methods. It appears now that we're allowing 
more potential successful resolutions, then using the additional rules 
on the invoke* bytecodes to try and discard any undesirable results.

---

6.5 getfield (and others)

You note "These rules are redundant: verification already guarantees 
them " but this brings me back to an area I keep raising concerns about: 
the split between static verification based checks and dynamic runtime 
checks. Yes the verifier precludes certain things but if we run without 
verification the rules expressed for the bytecodes are considered to be 
still required at runtime. If you delete them because they overlap with 
verifier rules we have no way to tell which rules must be enforced 
regardless of verification status.

---

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).

---

5.4.3.3 Method resolution

You did not make any changes here, but as per my comment in the bug 
report we do require, IMHO, further tightening here to ensure nestmate 
invokespecial invocations do not resolve to method implementations that 
they should not. The example here is a hierarchy of nestmates (C extends 
B extends A) where A and C both declare a private method "void m()" and 
we have an invocation c.m() where c is an instance of class C. We then 
"separately compile" C to remove the definition of m(). At runtime 
method resolution will locate A.m, even though the method reference was 
for C.m. Normally the accessibility rules would exclude A.m from being a 
viable candidate but here the classes are nestmates so A.m is 
accessible. But it would be wrong to invoke A.m. This case should throw 
ICCE or NSME.

Thanks,
David
-----