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

[David Holmes]

Hi Peter,

Thanks for looking at this. I'm glad I didn't try to respond last night 
before your follow up emails came through :)

On 22/05/2018 8:36 PM, Peter Levart wrote:
> Hi David,
> 
> On 05/15/2018 02:52 AM, David Holmes wrote:
>> Master webrev of all changes:
>>
>> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
> 
> I skimmed over reflection API changes.
> 
> In jl.Class:
> 
> 3911         // returning a different class requires a security check
> 3912         SecurityManager sm = System.getSecurityManager();
> 3913         if (sm != null) {
> 3914             checkPackageAccess(sm,
> 3915 ClassLoader.getClassLoader(Reflection.getCallerClass()), true);
> 3916         }
> 
> ...so here the "different" class is expected to be in the same package 
> as "this" class. Is this invariant enforced in VM so it need not be 
> checked here?

Yes. All nestmates have to be in the same runtime package, and this is 
enforced in the VM. As Mandy explained this covers the case where you 
manage to get hold of an implementation object (potentially from deep 
inside some other package - perhaps module?) and call getClass() on it 
(which is allowed) and then try to examine it's nest. If the 
nest-host/member being returned is not the current class, then you 
should perform the SM check. (As you already have the current class it 
doesn't matter if it is returned.)

> 
> 3871      * @apiNote The source language compiler is responsible for 
> deciding which classes
> 3872      * and interfaces are nestmates, by generating the appropriate 
> attributes
> 3873      * (JVMS 4.7.28 and 4.7.29) in the class file format (JVMS 4).
> 3874      * For example, the {@code javac} compiler
> 3875      * places a top-level class or interface into a nest with all 
> of its direct,
> 3876      * and indirect, {@linkplain #getDeclaredClasses() nested 
> classes and interfaces}
> 3877      * (JLS 8).
> 3878      * The top-level {@linkplain #getEnclosingClass() enclosing 
> class or interface}
> 3879      * is designated as the nest host.
> 
> Should the text warn about not relying on this implementation detail to 
> extract knowledge about nested/enclosing classes? Users should keep 
> using getDeclaredClasses()/getEnclosingClass() for that purpose as 
> nestmates may in future be used for other things too. OTOH, if users 
> want to do an equivalent of private access check (on behalf of real 
> caller and callee - for example in some kind of language runtime), it 
> would be better they use the nestmate API...

I think the fact this is an apiNote that explicitly calls out how the 
javac compiler maps from the language concept of nested types to the VM 
concept of nests, should be enough to guide the programmer here. As you 
note there is no guarantee that the nest-host is the top-level class. 
But beyond that the existing API's work quite differently in terms of 
exploring the actual structure of nested types, whereas the nestmate API 
is a "flat world".

It is possible (likely) that in the future VM nests will be used for 
additional things - not all of which need be observable via a Java 
specific API. That will be determined later.

For a language runtime that wants to know before hand whether a direct 
private access will work, then yes isNestmate() is what they would use.

> in j.l.r.AccessibleMember:
> 
>    49  * <p> Java language access control prevents use of private 
> members *outside*
>    50  * *their top-level class;* package access members outside their 
> package; protected members
>    51  * outside their package or subclasses; and public members outside 
> their
>    52  * module unless they are declared in an {@link 
> Module#isExported(String,Module)
>    53  * exported} package and the user {@link Module#canRead reads} 
> their module.
> 
> Could this be interpreted also as "private members can only be accessed 
> from the top-level class"? I know that "nest" is not a Java language 
> term, so it can not be used in the context of Java language access 
> control. (If speaking of Java language, then even previous version of 
> this text was wrong - if it was right, then it wouldn't have to be 
> changed at all). So what about the following:
> 
> "Java language access control prevents use of private members outside 
> the set of classes composed of top-level class and its transitive 
> closure of nested classes".
> 
> Or would this be to lengthy and hard to understand?

Yes. :)

This was a minor correction to what was, as you noted, already an 
inaccurate statement:

"Java language access control prevents use of private members outside 
their class."

That's obviously not true. As per JLS 6.6.1:

"Otherwise, the member or constructor is declared private, and access is
permitted if and only if it occurs within the body of the top level type 
(§7.6) that encloses the declaration of the member or constructor."

So "class" was changed to "top-level class" to match the JLS.


> in j.l.r.Reflection:
> 
>   140         // At this point we know that currentClass can access 
> memberClass.
>   141
>   142         if (Modifier.isPublic(modifiers)) {
>   143             return true;
>   144         }
>   145
>   146         // Check for nestmate access if member is private
>   147         if (Modifier.isPrivate(modifiers)) {
>   148           // assert: isSubclassof(targetClass, memberClass)
> 
> although just in a function of explaining the following comment, I think 
> the correct assert is
> 
> // assert targetClass == null || isSubclassof(targetClass, memberClass)
> 
> as for static members, targetClass is null.

Yes. I think I'll just delete the psuedo-assert though because I think 
it's only true if the nestmate access is true - plus it was more a 
mental note to myself when working on this.

>   149           // Note: targetClass may be outside the nest, but that 
> is okay
>   150           //       as long as memberClass is in the nest.
>   151           boolean nestmates = areNestMates(currentClass, memberClass);
>   152           if (nestmates) {
>   153             return true;
>   154           }
>   155         }
> 
> I'm wondering about the frequency of reflective accesses of various 
> kinds of members. I imagine that most frequent are accesses of public 
> members, others are less so (I'm not counting accesses made via 
> .setAccessible(true) modified Field/Method/Constructor objects as they 
> are bypassing this Reflection.verifyMemberAccess method). But among 
> others I imagine reflective access to private members is the least 
> frequent as there's no practical reason to use reflection inside and 
> among classes that are most intimately logically coupled and "know" each 
> other's internals at compile time. So it would make sense to check for 
> private access at the end, after protected and package-private checks 
> (that's how existing logic was structured).
> 
> So how about putting the nestmate access logic just before the last if 
> (!successSoFar) { return false; }:
> 
>          // Check for nestmate access if member is private
>          if (!successSoFar && Modifier.isPrivate(modifiers)) {
>            // assert: targetClass == null || isSubclassof(targetClass, 
> memberClass)
>            // Note: targetClass may be outside the nest, but that is okay
>            //       as long as memberClass is in the nest.
>            successSoFar = areNestMates(currentClass, memberClass);
>          }
> 
>          if (!successSoFar) {
>              return false;
>          }
> 
> 
> This would not penalize access to package-private and protected members 
> with areNestMates() JNI calls and maybe caching will not be needed at all.

As per your later email the check is guarded by the isPrivate check so 
we avoid the JNI call. Also I deliberately placed the check where it is 
(in contrast to VM code where it is almost last) to avoid the whole 
"successSoFar" mess. :) My view was that if this indeed turned out to 
impact performance then we could revisit the placement of it.

Thanks,
David

> 
> Regards, Peter
>