Thanks for the review. I forgot to mention that Chris contributed the
initial patch (thanks).
On 3/27/2013 1:13 PM, Christian Thalinger wrote:
On Mar 27, 2013, at 10:35 AM, Mandy Chung <mandy.ch...@oracle.com> wrote:
This is the JDK change for JEP 176: JEP 176: Mechanical Checking of
Caller-Sensitive Methods [1]. Christian has posted the webrev for the hotspot
VM change a couple weeks ago [2].
Webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7198429/webrev.00/
src/share/classes/java/lang/ClassLoader.java:
+ static void checkClassLoaderPermission(ClassLoader cl, Class<?> caller) {
I think we should rename that method to:
+ static void checkGetClassLoaderPermission(ClassLoader cl, Class<?> caller)
{
I think checkClassLoaderPermission and needsClassLoaderPermissionCheck
are just fine. I'd like to keep it as it is not to make the method name
too long.
src/share/classes/java/lang/invoke/MethodHandleImpl.java:
+ @sun.reflect.CallerSensitive
+ Class<?> actual = sun.reflect.Reflection.getCallerClass();
Why are we not using imports here?
imports are for convenience and ease of development. For only one
reference, I don't see any difference to import or not.
src/share/classes/java/util/logging/Logger.java:
// 0: Reflection 1: Logger.demandLogger 2: Logger.getLogger 3:
caller
final int SKIP_FRAMES = 3;
Please remove these lines as well.
Removed. Thanks for catching the leftover comment.
src/share/native/sun/reflect/Reflection.c:
Could you put back this comment:
+ // Let's do at least some basic handshaking:
+ const int depth = -1;
It makes it clearer why it's -1.
I added this comment:
32 // with the presence of @CallerSensitive annotation,
33 // JVM_GetCallerClass asserts depth == -1 as the basic handshaking
test/sun/reflect/GetCallerClass.sh:
Could you please don't use a shell script to copy the class file?
The shell test doesn't do a copy. It compiles the source file in a
separate directory that will be specified in -Xbootclasspath/a option in
javac and java commands.
jtreg in the code-tool repo has added the bootclasspath support:
http://hg.openjdk.java.net/code-tools/jtreg/rev/98387c9f36e3
You can specify in the @run tag:
@run main/bootclasspath opt class
This will be a better way to run a test on the bootclasspath.
For example this test:
http://hg.openjdk.java.net/hsx/hotspot-main/hotspot/file/tip/test/compiler/whitebox/DeoptimizeAllTest.java
does the same thing using a little Java program ClassFileInstaller:
http://hg.openjdk.java.net/hsx/hotspot-main/hotspot/file/tip/test/testlibrary/ClassFileInstaller.java
This is a nice workaround to avoid shell tests. It compiles the source
file in $TESTCLASSES and copies the one in a different location (dest)
that will be used in a @run main -Xbootclasspath/a:dest class.
I prefer to use the new jtreg bootclasspath support when it's released
rather than adding yet another workaround to avoid shell tests. We
should replace many, if not all, existing shell tests that currently put
classes in the bootclasspath with the jtreg bootclasspath support in one
patch. I keep the test as it is.
Mandy
-- Chris
While it touches many files, the fix is simple and straight-forward for review.
This fix annotates all methods that call Reflection.getCallerClass() method
with @sun.reflect.CallerSensitive annotation so that it enables the VM to
reliably enforce that methods looking up its immediate caller class are marked
as caller-sensitive. The JVM will set a new caller-sensitive bit when resolving
a MemberName and java.lang.invoke.MethodHandleNatives.isCallerSensitive is
upgraded to query it directly.
The hand-maintained method list in MethodHandleNatives is removed.
A couple things to mention:
1. I am working on a fix for 8007035 that proposes to deprecate
SecurityManager.checkMemberAccess method as it requires the caller’s frame to
be at a stack depth of four, which is fragile and difficult to enforce.
2. NashornScriptEngineFactory.getAppClassLoader()
The change is to workaround the issue until 8009783 is resolved.
The current implementation walks the stack to find the classloader of the user context
that NashornScriptEngine is running on which is fragile. Also other script engine
implementations may require similiar capability. 8009783 has been filed to revisit the
scripting API to pass the user "context" to the script engine rather than
relying the implementation to find it magically.
Thanks
Mandy
[1] http://openjdk.java.net/jeps/176
[2] http://mail.openjdk.java.net/pipermail/hotspot-dev/2013-March/008915.html
