RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean.long at oracle.com]

Thanks Ioi.

dl

On 10/31/18 6:01 PM, Ioi Lam wrote:
>
>
> On 10/31/18 5:13 PM, dean.long at oracle.com wrote:
>> On 10/31/18 4:06 PM, David Holmes wrote:
>>> Hi Dean,
>>>
>>> Looking only at the hotspot changes. The removal of the DoPrivileged 
>>> and related privileged_stack code seems okay. I have a few related 
>>> comments:
>>>
>>> src/hotspot/share/classfile/systemDictionary.hpp
>>>
>>> You added the java_security_AccessController class after 
>>> java_security_AccessControlContext. Did you actually verify where in 
>>> the load/initialization order the AccessController class appears 
>>> today, and where it appears after your change? (Note the comments at 
>>> the start of WK_KLASSES_DO). Changes to the initialization order 
>>> would be my biggest concern with this patch.
>>>
>>
>> No, I did not notice that comment and assumed alphabetical order was 
>> OK here.  However, these classes appear to be only resolved, not 
>> initialized (and AccessController does not have a static 
>> initializer), so could you explain how the order in this list can 
>> affect initialization order?
>>
>> I only need this in JVM_GetStackAccessControlContext, which is a 
>> static JNI method, so I could get the klass* from the incoming jclass 
>> instead of using a well-known class entry.
>
> Hi Dean,
>
> You're correct that those classes are only resolved, and not 
> initialized. So the order shouldn't matter too much. However, the 
> order of the first few classes is important, as the creation of 
> Object, Serializable, Cloneable, String, etc, has to be done in a 
> certain order.
>
> I'm not sure whether the order of the reference classes, 292 classes, 
> and box classes are important. I'll experiment of getting rid of the 
> separate phases after the call to Universe::fixup_mirrors(). This 
> might be relics from old days where the classes were once indeed 
> initialized in SystemDictionary::initialize_well_known_classes, which 
> was the old name for SystemDictionary::resolve_well_known_classes.
>
> (-XX:+TraceBytecodes shows no Java code is executed before 
> resolve_well_known_classes returns).
>
> I filed https://bugs.openjdk.java.net/browse/JDK-8213230
>
> For the time being, I think as long as you put the new 
> AccessController class near the existing class AccessControlContext, 
> you should be OK.
>
> Thanks
> - Ioi
>>
>>> ---
>>>
>>> I'm unclear about the change to the test:
>>>
>>> test/hotspot/jtreg/runtime/JVMDoPrivileged/DoPrivRunAbstract.jasm
>>>
>>> as it still refers to the now non-existent JVM_DoPrivileged in the 
>>> summary. Does this test still make sense? Should it be moved to the 
>>> Java side now it doesn't actually test anything in the VM?
>>>
>>
>> I think these tests still make sense, unless we have similar coverage 
>> somewhere else.  How about if I fix the reference to JVM_DoPrivileged 
>> for now and file a separate bug about moving or removing these tests?
>>
>>> ---
>>>
>>> There may be further dead code to remove now:
>>>
>>> vmSymbols.hpp: codesource_permissioncollection_signature is now 
>>> unreferenced and can be removed.
>>>
>>> javaClasses.*:
>>>   - java_lang_System::has_security_manager
>>>   - java_security_AccessControlContext::is_authorized
>>>
>>> ./share/memory/universe.hpp:  static Method* 
>>> protection_domain_implies_method()
>>>
>>
>> Good catches!  I have uploaded a new webrev:
>>
>> http://cr.openjdk.java.net/~dlong/8212605/webrev.2/
>> http://cr.openjdk.java.net/~dlong/8212605/webrev.2.incr/ (incremental 
>> diff)
>>
>> dl
>>
>>> ---
>>>
>>> Thanks,
>>> David
>>>
>>>
>>> On 1/11/2018 8:23 AM, dean.long at oracle.com wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8212605
>>>> http://cr.openjdk.java.net/~dlong/8212605/webrev.1
>>>>
>>>> This change implements AccessController.doPrivileged in Java. This 
>>>> gives a performance improvement while also being useful to Project 
>>>> Loom by removing the Java --> native --> Java transition.  One 
>>>> reason doPrivileged has historically been in native is because of 
>>>> the need to guarantee the cleanup of the privileged context when 
>>>> doPrivileged returns.  To do that in Java, the information that 
>>>> AccessController.getContext needs is pushed onto the Java stack as 
>>>> arguments to a method that getContext will recognize during its 
>>>> stack walk.  This allows us to remove the native privileged stack 
>>>> while guaranteeing that the privileged context goes away when the 
>>>> method returns.
>>>>
>>>> Tested with tier1-tier3 hotspot and jdk tests and JCK 
>>>> api/java_security tests.  For the first few rounds of testing, I 
>>>> kept the old native privileged stack and compared the results of 
>>>> the old and new implementations for each getContext call, which did 
>>>> catch some early bugs.  The changes were also examined by internal 
>>>> security experts and run through additional internal security tests.
>>>>
>>>> The improvement on this [1] doPrivileged microbenchmark is 
>>>> approximate 50x.
>>>>
>>>> There is no attempt to optimize getContext() or security permission 
>>>> checks in this change, however, this is intended to be a first step 
>>>> towards other possible improvements, for example those proposed 
>>>> here [2].
>>>>
>>>> dl
>>>>
>>>> [1] 
>>>> http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 
>>>>
>>>> [2] 
>>>> http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html 
>>>>
>>>>
>>
>