On Mar 27, 2013, at 8:01 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 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, Well, it's actually two ;-) I was thinking about people grep'ing for @CallerSensitive and not getting a hit on this one. Just a suggestion. > 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 Thanks. > >> 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. That's great. > >> 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. I take your word for it that you will remove it. Checking back in a year... :-) -- Chris > > 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 >
