Draft JVMS changes for Nestmates

[John Rose]

On Apr 18, 2017, at 11:42 AM, Dan Smith <daniel.smith at oracle.com> 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

Nice.  More comments on that later.

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

+1 to Brian's suggestion

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

The terms for the JVM's circles of trust are "nest", "package", "module".
"Access control context" does not belong on that list of terms.

"Nest" connotes the motivation to support nested classes.
(Could also have said "block" or "scope"—those are also relevant language constructs.)
"Nest" also connotes a home-like place and therefore privacy.
(Could also have said "home" or "family"—those are also private spaces.  Or "crib"?)
Only "nest" connotes both AFAIK.

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

I am comfortable handing over the term "host" for this purpose.

I expect Lookup.defineClass to inject into the nest of the Lookup.lookupClass,
if the lookup has private access.  That's what we call hosting for VMACs,
almost exactly.  (+1 to Brian's comments to Remi.)

I would prefer "nest host" or "nest host class" or "host class of nest" to plain "host class",
because it will be better if we mention "nest" when we mention the term for a nest's host class.
Downside is pronunciation of "nest-host" sounds like a sneeze.

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

+1

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

See my previous message.  There are more structural constraints I'd like to see.
(But no additional class loading!)

I agree that MemberOfNest needs to be checked early.  I'd prefer to do it along
with the supers but that leads to vicious dependency cycles.  So verification time
is best.  IIRC that's also called "link time", which is intuitive to me:  One of the first
things you do when you link a class is determine which nest it is in.

I totally agree that NestMembers should only be checked as needed.
It's asymmetric, but the asymmetry lets us escape a bunch of class file loading.

Here's a corner case we need to cover:  I have to load my supers before I can load myself.
If one (or more) of my supers are in the same nest as me, the checking of MoN must succeed
no matter who is the host class of the nest.

This could get embarrassing if the host class is the subtype, if the super then needs to
recursively load the subtype (which is waiting on the supertype to load) before it can
validate its MoN attribute.  If verification (linking) is started after all of the classes are
loaded, things are OK, since the MoN checks happen only after everything is loaded.

N.B. The class file parser code in HotSpot sometimes says "verify" when I think it really
means "check constraints".  (The code also mentions constraints.)  It does not mean
verification in the sense that we are talking about here.

The extra constraint checking I propose should happen during parsing/loading not
linking/"verification".

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

Nice work.

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

I wish we could maximize common formulations of the rules for protected references and
invokespecial.  They are very similar.

I was surprised to see invokespecial occasionally mentioned cases where it only works on
private members.  Is that a current constraint, or a new one?

I thought a class could invokespecial any of its methods, willy-nilly, and this permission
would seem to extend naturally to nestmates.  (I think I'm forgetting something here.)

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

(Uh…  Paging Dr. Protector.  What?  He retired when!?)

Well, this will take some doing.  But I agree strongly that you are making an improvement
to the spec.

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

That is correct.  The errors are also morphed to ReflectiveOperationExceptions for
the corresponding Lookup API calls.

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

I am leaning towards putting this check in.  If we put it in, it should go in everywhere,
including invokespecial of default methods, both in and out of the caller's nest.
I think your language might have allowed an untyped loophole for nestmates.

My goal in this is simplicity of effect:  There are no loopholes, no exceptions
to typing of the incoming receiver (L0) of a default method.  If we are going to
check any of these invokespecial receivers, we must check them all.

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

Good point.

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

The spec. of findSpecial mentions the bytecode behavior of the invokespecial instruction.
This might mean we only need a few non-normative comments in the javadoc.  I can dream.

The spec. of Lookup.in is more subtle.  As I've said elsewhere, I would like to make a
clean break from what we currently do, which is grub through the InnerClasses attributes
to emulate nest-detection.  We still have to do this for compatibility, but if either class
has a non-trivial nest (MoN or NMs attr.) then Lookup.in will *only* consult that nest,
and *ignore* InnerClasses.

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

Because packages are generally unsealed and can accept new members.
The new thing about a nest is that its member list is fixed.  That's more secure.
And barring funny compilation tricks, the nestmates are all created at the same
time, and presumably from a consistent configuration of source code.
That's even more secure, since you can audit the nest as a whole in source.

— John