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

Ioi Lam ioi.lam at oracle.com
Thu Nov 1 01:01:35 UTC 2018

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.

- 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 

More information about the core-libs-dev mailing list