RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged
dean.long at oracle.com
dean.long at oracle.com
Thu Nov 1 03:59:12 UTC 2018
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
>>>>
>>>>
>>
>
More information about the hotspot-dev
mailing list