Draft JVMS changes for Nestmates
david.holmes at oracle.com
Wed Apr 19 06:40:46 UTC 2017
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:
> 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.
> 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
> 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 (126.96.36.199) 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 ________.)
Now looking at specific proposed changes to the spec wording ...
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:
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
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
"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?
"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).
188.8.131.52 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 (184.108.40.206)
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.
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).
220.127.116.11 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.
More information about the valhalla-spec-observers