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.l...@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
