[hs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
David Holmes
david.holmes at oracle.com
Wed May 23 04:07:01 UTC 2018
Hi Coleen,
Thanks for looking at this!
On 19/05/2018 8:36 AM, coleen.phillimore at oracle.com wrote:
>
> 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.
The above was moved to and answered on the serviceability-dev review thread.
> 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.
There was a lot of changes to the way exceptions were handled as this
feature developed. The resulting code may well need some clean up ...
The exceptions have to propagate so I just updated with:
bool access = cur_ik->has_nestmate_access_to(field_ik, CHECK_false);
> Or add TRAPS to
> reflection::verify_field_access so that the callers properly handle the
> pending exception?
As Reflection::verify_field_access is a boolean function it gets used in
"if" expressions that make it unsuitable for use with TRAPS and CHECK
macros. I can either rewrite all those if's to introduce a local
variable assignment that is checked in the if, or else just do the
pending exception check explicitly. I chose the latter for simplicity -
and in one case (ciField) we need to clear the exception as well so
CHECK_ doesn't apply.
> 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.
So we currently have, for example:
InstanceKlass* cur_host = nest_host(icce, THREAD);
if (cur_host == NULL || HAS_PENDING_EXCEPTION) {
return false;
}
and you're suggesting:
InstanceKlass* cur_host = nest_host(icce, CHECK_false);
if (cur_host == NULL) {
return false;
}
I'm not really seeing a great difference in readability. Code-wise I
think the current code is slightly more efficient, but I'm not sure
about that.
>
> 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()
I _think_ it is impossible - it's a very odd situation and I could not
see how to construct a test that would hit this. But I don't know for
sure that it is impossible. So the code assumes classloading and thus
exceptions are possible.
>
> + }
> + else {
>
>
> Oh please can you fix these? The hotspot style is the } else { all on
> one line (OTBS).
Okay - 339 cases in hotspot as above versus 12913 as style guide
suggests :) I will fix my additions in instanceKlass.cpp.
> Since has_nest_member is only used in InstanceKlass, it should have
> private access.
>
> + bool has_nest_member(InstanceKlass* k, TRAPS) const;
True it could ... but I really prefer to have the nest functions grouped
together, and switching back and forth between private and public would
look messy.
> Looks like raw_nest_host() is uncalled (and shouldn't be public).
It was a public debugging hook. Now removed again.
> 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 again!
David
> 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