[hs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Thu May 24 02:00:59 UTC 2018
Hi David, replies below.
On 5/23/18 12:07 AM, David Holmes wrote:
> 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);
>
>
Thanks.
>> 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.
Yes, I think this might be something that could be cleaned up. I didn't
know if all the callers of verify_field_acess could handle a pending
exception and it took a while to look at these to convince myself they
weren't broken.
>
>> 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.
>
I think a decent C++ optimizer could handle the two conditional
statements. This is another case that I had to follow because it looked
suspicious.
>>
>> 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.
>
Ok.
>>
>> + }
>> + 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.
>
Is that just this one file? There must be more else statements in
hotspot. Thanks for fixing these.
>> 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.
This is the one thing I like about Java. I've been adding public:
private: around the functions that are private but still keeping them
together. If something is marked private, we know nothing should be
calling it so it's good to have.
>
>> 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, my review is complete.
Coleen
> 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