[hs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
David Holmes
david.holmes at oracle.com
Thu May 24 03:01:43 UTC 2018
Hi Coleen,
Okay I will make the changes you suggest. Once done I'll post a v3 RFR
update (well once all v3 updates are in place).
I'll need a v3 anyway to add in some test changes for tests that just
got moved to open.
Thanks,
David
On 24/05/2018 12:00 PM, coleen.phillimore at oracle.com wrote:
>
> 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