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

[dean.long at oracle.com]

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.

> ---
>
> 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 
>>
>>