[core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[David Holmes]

Hi Alan,

Many thanks for looking at this!

Let me start by saying that generally any spec changes (even 
non-normative) have to pass a high bar given the number of levels of 
review the spec, ie API, has already had. That isn't to say that no 
changes will be considered but I'm very wary of making last minute 
changes during code review, without all the parties previously involved 
with, and content with, the spec being party to the conversation. Such 
changes also affect the CSR request. So my initial position will be that 
of the immovable object. ;)

On 15/05/2018 11:53 PM, Alan Bateman wrote:
> On 15/05/2018 01:52, David Holmes wrote:
>> This review is being spread across four groups: langtools, core-libs, 
>> hotspot and serviceability. This is the specific review thread for 
>> core-libs - webrev:
>>
>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/ 
> The API additions mostly look good, just a few comments:
> 
> - Can the spec for Class::getNestHost be explicit that it returns "this" 
> when the class is a primitive or array class?

This is covered by:

"A class or interface that is not explicitly a member of a nest, is a 
member of the nest consisting only of itself, and is the nest host. 
Every class and interface is a member of exactly one nest."

Do we really need to spell out the case for primitives and arrays? If so 
would it suffice to add the following:

"A class or interface that is not explicitly a member of a nest *(such 
as primitive or array classes)*, is a member of the nest consisting only 
of itself, and is the nest host. Every class and interface is a member 
of exactly one nest."

> - Can Class::getNestMembers specify if it runs the class initializer of 
> any classes that it loads? Less of an issue for getNestHost as the host 
> is likely loaded already.

None of these methods specifically or intentionally initialize any 
classes. A class will be loaded if needed but that is all. The only 
Class methods that refer to class initialization are the forName() 
variants. I don't see that the nest related methods should do any more 
or less in this regard than any other Class methods that may require 
classes to be loaded.

> - I suspect the @throws SecurityException in getNestMembers was copied 
> from getNestHost as it uses "returned class" (singular). 

It refers to "If any returned class ..." and "that returned class". I 
don't see any problematic singular uses - can you elaborate please.

> As the host and 
> members are in the same runtime package then maybe it can be specified 
> in terms of the host or members package?

I'm not sure how to accurately formulate that. The current wording was 
based on similar @throws in getEnclosingClass, as suggested by Mandy:

http://mail.openjdk.java.net/pipermail/valhalla-dev/2018-March/003955.html

and then refined a little.

> - isNestmateOf could be a bit clearer that it returns false in the event 
> that the attributes can't be parsed. I realize it's specified in terms 
> of getNestHost but I'm sure you see what I mean.

Just to be clear, parsing of nest attributes occurs at class load time 
and must be syntactically correct else the class won't load. Validation 
of those attributes (i.e agreement between the purported nest-host class 
and the claimed nest-member class) is what is deferred until either an 
access check is needed, or these new Class reflection methods are 
called. So when getNestHost is called the current class is obviously 
already loaded, but the purported nest-host may need to be loaded which 
may encounter:
- linkage-errors (NoClassDefFoundError, VerifyError, 
ClassfileFormatError etc)
- runtime errors (OOME, stackoverflow)
- disagreement between the nest-host and this class about nest membership

I just noticed that there is a wording error in getNestHost() as it states:

"If there is any error accessing the nest host, ... then this is returned."

but in the implementation that's only true for LinkageErrors. Runtime 
errors get propagated. I think this is what we want (other places in MH 
runtime where runtime-errors were getting swallowed or converted have 
since been switched back to propagate them.) But I'll need to run this 
past the EG to check.

That notwithstanding, exactly what would you propose here? The current 
spec is clear and concise and precise, but defers to getNestHost 
somewhat for details on what happens when things go wrong.

> The tests for core reflection are in test/jdk/java/lang/reflect and I'm 
> wondering if there has been any discussion about adding at least some 
> basic tests there? I see the tests in 
> test/hotspot/jtreg/runtime/Nestmates/reflectionAPI but this isn't an 
> obvious location for someone touching this code.

Good catch! It was always my intent to redistribute the new nestmate 
tests into their component areas where applicable and appropriate - but 
I forgot to do that before calling for the RFR.  I will move them to 
java/lang/reflect/Nestmates.

> The test in InterfaceAccessFlagsTest.java can be disabled with 
> @Test(enabled=false), might be cleaner than commenting it out.

Thanks - will fix.

> Maybe a question for Kumar but are we planning to pull in any ASM 
> updates for JDK 11? NestMembers extends Attribute looks okay, I'm less 
> sure about the change to ClassReader as I don't know if there is 
> somewhere else in ASM that has the list of attributes to always parse.

Remi already responded on this. Seems what we have now is okay but 
eventually it will have to be reconciled when we update from upstream ASM.

> The update to the access check in Reflection.java looks okay (just need 
> to use 4-space indent, not 2).  

Will fix the indent - thanks for spotting.

> I think it's okay in VerifyAccess too 
> but the "FIX ME" suggests there are doubts so I assume this needs more 
> eyes to triple check.

The "FIX ME" is not about the access check but the form of the 
assertion. The assert is verification that resolution of a private 
method always leads to selection of that private method: refc == defc. I 
used "myassert" so that this check was always enabled during testing. 
The "FIX ME" was to either convert to language assert statement or else 
remove it (having validated that it never fires). Obviously "myassert" 
has not fired in all the testing that I have done so either choice seems 
fine. Do you have a preference?

> In passing, you might want to double check the javadoc links. I noticed 
> one #getNestHost that is missing parenthesis and I'm not sure how 
> javadoc handles that.

Should have generated a warning so I'll fix that - thanks.

Thanks again for the review.

David

> -Alan.