[hs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control
David Holmes
david.holmes at oracle.com
Wed May 23 06:58:04 UTC 2018
Hi Karen,
Thanks for looking (yet again!) at this.
On 22/05/2018 4:48 AM, Karen Kinnear wrote:
> 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
Done
> 2. LinkResolver.cpp line 782
> cleanup:
> FIXME:
Debugging code removed - thanks!
> 3. JVM_AreNestMates
> If member is a primitive or an array - what happens?
We crash or hit asserts - only instanceKlasses support nestmate
attributes. That's why they get filtered in the Java code.
> 4. reflection.cpp line 714:
> cleanup: FIXME - perhaps add assertions?
I worry that there may be a valid code path that allows us to get there
with non-instanceKlasses, in which case in product we would crash.
Ideally I'd like to be able to prove it is impossible, but I don't know
how to do that.
I will remove the comment.
> 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
That code is now less visible through use of CHECK_false on the
has_nestmate_access_to call (suggested by Coleen). But I've expanded the
comment before it.
> 6. instanceKlass.cpp line 255:
> FIXME: an exception from this is perhaps impossible
> did you want an assertion here?
Again this is a case where I _think_ it's not possible but I can't be
sure and I would not want product code to crash. My choice with these
kinds of FIXME is to either prove the answer to the question (which I'm
unable to do) or else delete the comment. So comment deleted.
>
> 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.
The exception does not get cleared but propagates to the Java code. The
Java code then filters out LinkageErrors and allows runtime exceptions
to propagate.
> 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
I can't see that in ConstantPoolCacheEntry::set_direct_or_vtable_call.
Only for the interface method case do we explicitly call set_f1. rax is
not passed to invokevirtual_helper, which is what processes the Object
case - in fact rax gets stomped by that code.
> 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"
No, invokevirtual_helper processes all the Object cases. My most recent
changes in this area stopped the final Object methods from taking the
new private interface method path unintentionally.
> and line 3810
> "Check for private/final method invocation - indicated by vfinal"
At this point final Object methods have already been handled, all that
is left are private (interface) methods.
> 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
I'm not quite sure what form of comment you are looking for here.
holder() is the DEFC but there is no concept of REFC in this code that I
can see. ??
> 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
I've added it but to be honest I don't understand it's relevance in the
context of initializing the itable. ??
> 11. instanceKlass
> has_nest_member
> has this comment:
> // can't have two names the same, so we're done
> Where is that ensured?
In walking through the explanation I realized the comment is wrong. This
was one of several cleanups John suggested (though I added the comment):
https://bugs.openjdk.java.net/browse/JDK-8199567
Resolving a CP entry will cause a class load to occur for "name", and if
I have multiple CP entries with the same "name" (somehow) then they must
all resolve to the same klass. So once I've found a matching name and
examined the actual klass, searching for additional entries with the
same name is pointless as they must resolve to the same klass. Hence the
loop can terminate once we have a name match. The comment should read:
// can't have different classes for the same name, so we're done
BTW there is no test case for the case where the klass comparison fails.
I could not see how to construct one. It may well be impossible as per
the earlier comment in the code:
// names match so check actual klass - this may trigger class loading if
// it doesn't match (but that should be impossible)
If has_nest_member underpinned a public API (which I initially thought
it might) then it would be possible by simply having two same-named
classes from different classloaders. But the way this API is used is for
validation between a nest-member and it's nest-host as part of nest-host
resolution, and I can't see any way to get two resolved klasses that
somehow exist in different loaders with the same name.
Thanks again!
David
-----
>
> thanks,
> Karen
>
>> On May 14, 2018, at 8:52 PM, David Holmes <david.holmes at oracle.com
>> <mailto: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
>>
>
More information about the hotspot-dev
mailing list