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

Mon May 21 18:48:29 UTC 2018

David,

Many thanks for the extremely careful attention to detail. And thank you for the annotated master webrev.
I still owe you a review of the tests.

I had a number of questions/comments about the sources - mostly requests for clarifying comments:
1. LinkResolver::check_method_accessability:
line 591: worth adding something like "e.g. linkage errors due to nest_host resolution

2. LinkResolver.cpp line 782
  cleanup:
  FIXME:

3. JVM_AreNestMates
If member is a primitive or an array - what happens?

4. reflection.cpp line 714:
  cleanup: FIXME - perhaps add assertions?

5. reflection.cpp lines 721-722:
  perhaps add a comment that it is the caller's responsibility to check
  HAS_PENDING_EXCEPTION if return is false - specifically to decide
  context-specific handling of nestmate resolution/validation

6. instanceKlass.cpp line 255:
FIXME: an exception from this is perhaps impossible
  did you want an assertion here?

7. JVM_GetNestHost is the only caller of IK->nest_host that passes in a NULL.
  I would expect one of these to clear PENDING_EXCEPTION,it would be helpful
  for the comments to clarify whose responsibility this is.

8. templateTable_x86.cpp
line 3789:
  I belive that rax is reference klass (from f1) if interface method OR if
   j.l.Object final/private method
line 3794:
  I think this is more precisely:
  "First check for Object (not private/final) case, then private/final interface method,
   then regular interface method"
 and line 3810
  "Check for private/final method invocation - indicated by vfinal"

9. cpCache.cpp line 191
  set_f1(holder); // interface klass*
Could you possibly add a comment that for private/final method invocations
  REFC == DECC

also line 180:
check for private/final interface method invocations

10. klassVtable.cpp
Could please add a line to the comment in initialize_itable_for_interface
before 1217: call to lookup_instance_method_in_klasses
// Invokespecial does not perform selection based on the receiver, so it does
// not used the cached itable

11. instanceKlass
 has_nest_member
has this comment:
// can't have two names the same, so we're done
Where is that ensured?

thanks,
Karen

> On May 14, 2018, at 8:52 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> This review is being spread across four groups: langtools, core-libs, hotspot and serviceability. This is the specific review thread for hotspot - webrev:
> 
> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.hotspot.v1/
> 
> See below for full details - including annotated full webrev guiding the review.
> 
> The intent is to have JEP-181 targeted and integrated by the end of this month.
> 
> Thanks,
> David
> -----
> 
> The nestmates project (JEP-181) introduces new classfile attributes to identify classes and interfaces in the same nest, so that the VM can perform access control based on those attributes and so allow direct private access between nestmates without requiring javac to generate synthetic accessor methods. These access control changes also extend to core reflection and the MethodHandle.Lookup contexts.
> 
> Direct private calls between nestmates requires a more general calling context than is permitted by invokespecial, and so the JVMS is updated to allow, and javac updated to use, invokevirtual and invokeinterface for private class and interface method calls respectively. These changed semantics also extend to MethodHandle findXXX operations.
> 
> At this time we are only concerned with static nest definitions, which map to a top-level class/interface as the nest-host and all its nested types as nest-members.
> 
> Please see the JEP for further details.
> 
> JEP: https://bugs.openjdk.java.net/browse/JDK-8046171
> Bug: https://bugs.openjdk.java.net/browse/JDK-8010319
> CSR: https://bugs.openjdk.java.net/browse/JDK-8197445
> 
> All of the specification changes have been previously been worked out by the Valhalla Project Expert Group, and the implementation reviewed by the various contributors and discussed on the valhalla-dev mailing list.
> 
> Acknowledgments and contributions: Alex Buckley, Maurizio Cimadamore, Mandy Chung, Tobias Hartmann, Vladimir Ivanov, Karen Kinnear, Vladimir Kozlov, John Rose, Dan Smith, Serguei Spitsyn, Kumar Srinivasan
> 
> Master webrev of all changes:
> 
> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/
> 
> Annotated master webrev index:
> 
> http://cr.openjdk.java.net/~dholmes/8010319-JEP181/jep181-webrev.html
> 
> Performance: this is expected to be performance neutral in a general sense. Benchmarking and performance runs are about to start.
> 
> Testing Discussion:
> ------------------
> 
> The testing for nestmates can be broken into four main groups:
> 
> -  New tests specifically related to nestmates and currently in the runtime/Nestmates directory
> 
> - New tests to complement existing tests by adding in testcases not previously expressible.
>  -  For example java/lang/invoke/SpecialInterfaceCall.java tests use of invokespecial for private interface methods and performing receiver typechecks, so we add java/lang/invoke/PrivateInterfaceCall.java to do similar tests for invokeinterface.
> 
> -  New JVM TI tests to verify the spec changes related to nest attributes.
> 
> -  Existing tests significantly affected by the nestmates changes, primarily:
>   -  runtime/SelectionResolution
> 
>   In most cases the nestmate changes makes certain invocations that were illegal, legal (e.g. not requiring invokespecial to invoke private interface methods; allowing access to private members via reflection/Methodhandles that were previously not allowed).
> 
> - Existing tests incidentally affected by the nestmate changes
> 
>  This includes tests of things utilising class redefinition/retransformation to alter nested types but which unintentionally alter nest relationships (which is not permitted).
> 
> There are still a number of tests problem-listed with issues filed against them to have them adapted to work with nestmates. Some of these are intended to be addressed in the short-term, while some (such as the runtime/SelectionResolution test changes) may not eventuate.
> 
> - https://bugs.openjdk.java.net/browse/JDK-8203033
> - https://bugs.openjdk.java.net/browse/JDK-8199450
> - https://bugs.openjdk.java.net/browse/JDK-8196855
> - https://bugs.openjdk.java.net/browse/JDK-8194857
> - https://bugs.openjdk.java.net/browse/JDK-8187655
> 
> There is also further test work still to be completed (the JNI and JDI invocation tests):
> - https://bugs.openjdk.java.net/browse/JDK-8191117
> which will continue in parallel with the main RFR.
> 
> Pre-integration Testing:
> - General:
>    - Mach5: hs/jdk tier1,2
>    - Mach5: hs-nightly (tiers 1 -3)
> - Targetted
>   - nashorn (for asm changes)
>   - hotspot: runtime/*
>              serviceability/*
>              compiler/*
>              vmTestbase/*
>   - jdk: java/lang/invoke/*
>          java/lang/reflect/*
>          java/lang/instrument/*
>          java/lang/Class/*
>          java/lang/management/*
>  - langtools: tools/javac
>               tools/javap
> 