[hs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Fri May 18 22:36:16 UTC 2018
Hi, I haven't been following along or read through all of this, but I
had a look through the code and have some questions about it.
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/src/hotspot/share/prims/jvmtiRedefineClasses.cpp.udiff.html
There is commented out code at line 806 in
compare_and_normalize_class_versions. Could you make the added code a
separate function defined above so this isn't the longest function in
the jvm?
739 JvmtiThreadState *state =
JvmtiThreadState::state_for((JavaThread*)thread);
740 RedefineVerifyMark rvm(the_class, scratch_class, state);
Why does this need RedefineVerifyMark? It doesn't call the verifier.
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/src/hotspot/share/runtime/reflection.cpp.frames.html
720 bool access = cur_ik->has_nestmate_access_to(field_ik, THREAD);
721 if (HAS_PENDING_EXCEPTION) {
722 return false;
723 }
This looks suspicious. Do you want to CLEAR_PENDING_EXCEPTION? This
looks like the combination of these functions drops exceptions, that
could show up in inconvenient places. Or add TRAPS to
reflection::verify_field_access so that the callers properly handle the
pending exception?
This is CHECK_false. and also maybe has_nestmate_access_to, should pass
CHECK_false to its nest_host() calls, so it doesn't look so weird.
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.full.v1/src/hotspot/share/oops/instanceKlass.cpp.udiff.html
+ // names match so check actual klass - this may trigger class loading if
+ // it doesn't match (but that should be impossible)
+ Klass* k2 = _constants->klass_at(cp_index, CHECK_false);
If throwing an exception is impossible, this should probably have a
CATCH. This is code is odd about exceptions. It looks like
has_nestmate_access()
+ }
+ else {
Oh please can you fix these? The hotspot style is the } else { all on
one line (OTBS).
Since has_nest_member is only used in InstanceKlass, it should have
private access.
+ bool has_nest_member(InstanceKlass* k, TRAPS) const;
Looks like raw_nest_host() is uncalled (and shouldn't be public).
That's all I can do for now. I think you have good reviewers for the
feature itself, but if you want more, I'll spend more time with this.
thanks,
Coleen
On 5/14/18 8:52 PM, David Holmes 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
>
More information about the hotspot-dev
mailing list