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.chung@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/w...

does the same thing using a little Java program ClassFileInstaller:

http://hg.openjdk.java.net/hsx/hotspot-main/hotspot/file/tip/test/testlibrar...

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

On 4/1/13 12:28 PM, Alan Bateman wrote:

On 27/03/2013 17:35, Mandy Chung 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/

While it touches many files, the fix is simple and straight-forward for review. I went through the latest webrev.01 and this looks very good.

I guess I was initially surprised to see @CS on some of the AccessController.doPrivileged methods as these usages aren't checked (although they are caller sensitive).

These few methods are the special case that their usage are not checked. This raises a good point how we could enforce the check and whether it's appropriate to check in JVM_DoPrivileged. I will file a bug to follow up this separately if you are okay.

Did you consider adding a constant, JVM_DEPTH (-1) or some name like that, to jvm.h for the depth parameter?

Chris and I both agree it's a good thing to define this constant in jvm.h. I have made the change in the jdk side and will file a bug to update that in the hotspot repo.

I see Reflection.isCallerSensitive uses isExtClassLoader, we'll probably have to re-visit this in the future.

Yes.

Doug and Chris are on this list but might not have seen that this updates AtomicXXXXFieldUpdaters. They need to be aware of this for future merges.

On the tests, does GetCallerClassTest really need to check the stack trace? It seems unnecessary.

The stack trace check tries to catch if InternalError is thrown for other reason. Such regression might be rare but I prefer to perform an appropriate check while the test can reliably run.

One thing on the shell test (I read exchange about jtreg boot class path support) is that it needs the GPL header.

Fixed.

I was surprised to see CallerSensitiveFinder in the webrev and I'm curious how long it takes to run.

This is one discussion point I'd like to have. I was debating myself initially if this test should be enabled or not. What I found it takes 5-14 sec. Some sample timing on the jprt machines: linux_i586 jdk_lang took 14 mins and CallerSensitiveFinder took 8.5 sec macosx_x64 jdk_lang took 20.5 mins and CallerSensitiveFinder took 14.5 sec solaris_i586 jdk_lang took 15.5 mins and CallerSensitiveFinder took 10 sec windows_x64 jdk_lang took 16.5 mins and CallerSensitiveFinder took 10 sec We discussed tagging stress tests or long running tests so that they can be run on demand. I think this test would also be appropriate to be run in post-build hudson job, kind of tests. Mandy

On 04/02/2013 09:07 PM, Mandy Chung wrote:

On 4/1/13 5:24 PM, John Rose wrote:

...

On Mar 27, 2013, at 10:35 AM, Mandy Chung <mandy.chung@oracle.com <mailto:mandy.chung@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/ <http://cr.openjdk.java.net/%7Emchung/jdk8/webrevs/7198429/webrev.00/>

This is very good work. It makes certain aspects of the system much easier to understand.

I have reviewed it all, with the following comments:

Thanks for the review.

...

The transform moves caller sensitivity into a clearly visible position. In many cases, the call to Reflection.getCallerClass now precedes the check of the security manager. For some customer codes, this may cause a performance regression, although it will probably be in the noise in most cases.

Agree. I have restored the cases that should find caller class lazily.

...

If necessary, we can replace some uses of R.getCC() by something like (S.getSecurityManager() == null ? null : R.getCC()), recovering the original laziness of the stack check. This won't be necessary everywhere. Note that the server compiler can usually remove the stack walk. We may need to put something similar into the client compiler.

In the case of the reflective calls (Field, Constructor, Method), the check of the caller class is gated by both AccessibleObject.override and quickCheckMemberAccess, and you have preserved this for Constructor and Method (which are probably the most important performance case). Consider gating it also for Field, as noted below.

Per-file comments:

-- Class.java

(See below about checking for checkMemberAccess override.)

Done.

...

-- ClassLoader.java

Did you consider using Class.cast to get a runtime check instead of @SuppressWarnings("unchecked")?

Good point - I have modified it to do runtime check using Class.asSubclass.

...

-- MethodHandleNatives.java

Was this deletion intentional or was it something Eclipse or Netbeans did? -import static java.lang.invoke.MethodHandles.Lookup.IMPL_LOOKUP;

It was unintentional and I fixed it differently when I realized the reference of IMPL_LOOKUP. I now have restored to the original import static to be consistent with other files.

...

Good cleanups!

You can remove this since rAPC is static (error in original code): case "registerAsParallelCapable": return canBeCalledVirtual(mem, java.lang.ClassLoader.class);

Done.

...

Note that the subsequent call to canBeCalledVirtual will disqualify mem because it is static.

Yes.

...

-- MethodHandles.Lookup

Now that stack walking is out of the picture, make all the constructors private, and remove this scary comment:

...

* <p> * Also, don't make it private, lest javac interpose * an access$N method.

Done.

...

I like this comment, and the manual inlining technique (in this very special case): // Inlined SecurityManager.checkMemberAccess Since it tends to get lost in the previous comment block, I suggest you put it after the open brace of the inlined body.

Also, both parameters should be inlined at the top of the block, not just "which", so the rest of the block can a more or less verbatim copy.

+ { // Inlined SecurityManager.checkMemberAccess + final Class<?> clazz = refc/defc; + final int which = Member.PUBLIC/PRIVATE;

I considered that too and made the change. I actually took out the redundant check (which != PUBLIC) since we know the value.

...

As previously noted, smgr.getClass() == SecurityManager.class is too strict. Suggest hoisting the nasty logic into a boolean, and using it twice:

boolean standardCMA = (smgr.getClass() == SecurityManager.class); if (!standardCMA) try { standardCMA = smgr.getClass().getDeclaredMethod("checkMemberAccess", ...).getDeclaringClass() == SecurityManager.class; } catch (ReflectiveOperationException ex) { throw new InternalError(ex); }

if (standardCMA) { // // Inlined SecurityManager.checkMemberAccess final Class<?> clazz = refc/defc; final int which = Member.PUBLIC/PRIVATE; ... } else { smgr.checkMemberAccess(...); }

OK. I modified a little. For subclass case, I can simply check if Class.getDeclaredMethod finds the method; if exists, it's overrided; otherwise, NoSuchMethodException thrown indicates using the standard one.

Hi Mandy, There could be: public class SM1 extends SecurityManager { @Override public void checkMemberAccess(Class<?> clazz, int which) {... and: public class SM2 extends SM1 { ... // no checkMemberAccess override now if if you take SM2.class.getDeclaredMethod("checkMemberAccess", ...) it will throw NoSuchMethodException, but the method is overriden in SM1. So I think it's better to use what John suggested (although not using getDeclaredMethod, but getMethod instead): smgr.getClass().getMethod("checkMemberAccess", ...).getDeclaringClass() == SecurityManager.class Regards, Peter

...

(And a similar comment for Class.java.)

Done.

...

Lots of trouble for a corner case!

-- Field.java

Consider making R.getCC more lazy as follows:

+ return getFieldAccessor(obj, needsMemberAccessCheck() ? Reflection.getCallerClass() : clazz).get(obj);

+ private boolean needsMemberAccessCheck() { + return !override && !Reflection.quickCheckMemberAccess(clazz, modifiers); + }

(This might affect the performance of serialization.)

-- CallerSensitiveFinder.java

This is a remarkable test. Does it statically determine whether the annotations are missing?

Yes - that's the purpose to catch if any use of Reflection.getCallerClass() but not missing @CS annotation.

...

Does it process all of rt.jar? How long does it take to complete?

It processes all JRE classes (i.e. rt.jar, jar files in JAVA_HOME/lib and JAVA_HOME/lib/ext).

I have made it done the minimal necessary work only. It takes 6 seconds on my MacMini (2GHz core i7 and 8GB memory) to parse 24243 classes. It takes 5-14 seconds on the jprt machines and here are some sample timing: linux_i586 jdk_lang took 14 mins and CallerSensitiveFinder took 8.5 sec macosx_x64 jdk_lang took 20.5 mins and CallerSensitiveFinder took 14.5 sec solaris_i586 jdk_lang took 15.5 mins and CallerSensitiveFinder took 10 sec windows_x64 jdk_lang took 16.5 mins and CallerSensitiveFinder took 10 sec

...

And (I have to ask, though I'm sure the answer will be yes) have you run it on broken inputs (such as boot.GetCallerClass.missingCallerSensitiveAnnotation) and verified that it barks at them?

I ran it on an older JDK that calls Reflection.getCallerClass(int) and no @sun.reflect.CallerSensitive exist.

The test is currently tailored-made to only parses JRE classes. I can easily extend it to parse additional classes like boot.GetCallerClass that is a good test case to this tool itself. Will do so.

I'll post a new webrev once I finish the testing.

thanks for the detailed review. Mandy

Here is the updated webrev per John's and Alan's comments: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7198429/webrev.02/ In MethodHandles.java, it calls Class.getDeclaredMethod method to determine if a SecurityManager subclass overrides checkMemberAccess. It's called only if security manager is installed and it's a subclass. If it calls Class.getMethod instead, it will look up its superclasses and find the one in SecurityManager.class. For the case when the subclass doesn't override CMA, the difference in overhead will be throwing an exception vs. looking up CMA from the subclass and its superclasses and create the Method instance. I don't have the performance number to compare. This would be rare case and I tend to think instantiating and throwing an exception would be comparable to the reflection machinery. This check will go away when 8007035 is fixed that will deprecate SecurityManager.checkMemberAccess [1] and I'm fine going either way. FYI. 7198429 has been used for the hotspot changeset and will be resolved when it's integrated to jdk8/hotspot repo. The jdk changeset will be using 8010117, a subtask for 7198429, as specified in @bug tags in the tests. Mandy [1] http://hg.openjdk.java.net/hsx/hotspot-comp/hotspot/rev/16885e702c88 [2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-March/015547.html On 4/2/2013 3:25 PM, Mandy Chung wrote:

On 4/2/13 3:00 PM, Peter Levart wrote:

...

Hi Mandy,

There could be:

public class SM1 extends SecurityManager { @Override public void checkMemberAccess(Class<?> clazz, int which) {...

and:

public class SM2 extends SM1 { ... // no checkMemberAccess override

now if if you take SM2.class.getDeclaredMethod("checkMemberAccess", ...) it will throw NoSuchMethodException, but the method is overriden in SM1. So I think it's better to use what John suggested (although not using getDeclaredMethod, but getMethod instead):

smgr.getClass().getMethod("checkMemberAccess", ...).getDeclaringClass() == SecurityManager.class

Are you concerned the overhead of an exception thrown that we should avoid?

Mandy

This version has corrected to use Class.getMethod to determine if the checkMemberAccess method is overridden in the subclass: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7198429/webrev.02/ thanks Mandy On 4/2/2013 9:25 PM, John Rose wrote:

So getDM has a bug and getM is correct wrt inheritance. Thanks Peter!

On Apr 3, 2013, at 12:52 PM, Jeroen Frijters <jeroen@sumatra.nl> wrote:

Thanks for this. This is a really great change.

I reviewed the changes and my only comment is that there is a typo in java/lang/reflect/Field.java ("scurity").

Thanks!

Somewhat unrelated (but relevant for my implementation of CallerSensitive), but would it be possible to mark java.lang.Runtime as final, or at least make the CallerSensitive methods final?

The interaction between CallerSensitive and method overriding is non trivial, so IMO it is best avoided.

That is very true. We are working on separating those two patterns. Making Runtime and/or Runtime.load* final is an API change, which is harder to do than an implementation change. Given that Runtime.<init> is private, it seems to me that there is no way to execute an override of those load methods, even if one could (deviously) load a subclass into the JVM. It would be valid behavior, I think, for the JVM to quietly refuse to override them, because there would be no way to observe the refusal. Note that the technique of a private <init> method is the recommended way to prevent instantiation of subclasses, and is used throughout the JDK. This should be sufficient to prevent undesired interactions with @CS and overrides. What do you think? — John

On Apr 3, 2013, at 11:00 PM, Jeroen Frijters <jeroen@sumatra.nl> wrote:

Given the ability to create constructorless subclasses, it really should be combined with making the class final.

My current rules for @CallerID (which unlike @CallerSensitive is not just about semantics, but also about implementation strategy) allow it only to be used on static methods and instance methods in final classes (in theory constructors would also be allowed, but I didn't implement that since I didn't encounter any that were worthwhile).

Yes, making the class final fixes the interface problem, and in a portable way. It doesn't fix the ACC_SYNTHETIC problem.

An implicit rule is that a @CallerID instance methods may not implement an interface method.

Another way to skin that cat is to have the JVM refuse to populate an i-table (or non-HotSpot equivalent) with CallerID. It could populate the table entry with the thing that throws an AME or ICCE when a signature fails to match. In effect, there would be an extra signature match enforced on such calls.

Another thought that occurred to me was that there should probably be a test that goes through rt.jar and makes sure no ACC_BRIDGE or ACC_SYNTHETIC methods call @CallerSensitive methods.

That's an excellent point. Similar cat-skinning idea: The check could be done at link time, and throw something like ICCE. (These are all JVM-centric ideas; that's the biggest hammer in my own toolkit.) — John

Peter, Thanks. I misread your example and missed the fact that SM2 extends SM1. After seeing John's reply, I reread the example and realized the correctness issue you try to point out. I'll revise it and send out a new webrev. Mandy On 4/3/2013 12:39 AM, Peter Levart wrote:

On 04/03/2013 12:25 AM, Mandy Chung wrote:

...

On 4/2/13 3:00 PM, Peter Levart wrote:

...

Hi Mandy,

There could be:

public class SM1 extends SecurityManager { @Override public void checkMemberAccess(Class<?> clazz, int which) {...

and:

public class SM2 extends SM1 { ... // no checkMemberAccess override

now if if you take SM2.class.getDeclaredMethod("checkMemberAccess", ...) it will throw NoSuchMethodException, but the method is overriden in SM1. So I think it's better to use what John suggested (although not using getDeclaredMethod, but getMethod instead):

smgr.getClass().getMethod("checkMemberAccess", ...).getDeclaringClass() == SecurityManager.class

Are you concerned the overhead of an exception thrown that we should avoid?

Hi Mandy,

No, I was not thinking of performance. Just the correctness. Class.getDeclaredMethod(name, ...) only considers methods "declared" by the class (not inherited from a superclass - directy or indirectly). But you could have, like in example above, a hierarchy: SM2 extends SM1 extends SecurityManager where SM1 overrides the method in SecurityManager and SM2 doesn't. Now if you try SM2.class.getDeclaredMethod(), you will get NoSuchMethodException, but that does not mean the method is not overriden - it is in class SM1.

You could use getDeclaredMethod on the class and all superclasses until reaching SecurityManager to find out if the method is overriden, but it's fortunate that the method is public and so Class.getMethod(name, ...) can be used which does exactly that and already considers inheritance and overriding and returns the most specific method in the hierarchy. Explicit search using getDeclaredMethod() could skip searching the method in SecurityManager class though, but:

- negative answer via exception is slow - getMethod() only searches among public methods (which are separately cached in special array) and can therefore be faster if there is relatively large share of non-public methods declared in the class being searched.

Here's a quick micro-benchmark for the common case (only one level of subclassing the SecurityManager) and both variants (the method is overriden in SecurityManager subclass or not):

############################################################## # Java: 1.8.0-internal-peter_2013_01_16_13_44-b00 # VM: OpenJDK 64-Bit Server VM 25.0-b14 (mixed mode) # OS: Linux 3.7.9-104.fc17.x86_64 (amd64) # CPUs: 8 (virtual) # #------------------------------------------------------------- # GetDeclaredMethodOverridenSearch: run duration: 3,000 ms # # Warm up: # 1 threads, Tavg = 327.59 ns/op (σ = 0.00 ns/op) [ 327.59] # 1 threads, Tavg = 326.06 ns/op (σ = 0.00 ns/op) [ 326.06] # Measure: 1 threads, Tavg = 325.18 ns/op (σ = 0.00 ns/op) [ 325.18] # #------------------------------------------------------------- # GetPublicMethodOverridenSearch: run duration: 3,000 ms # # Warm up: # 1 threads, Tavg = 325.69 ns/op (σ = 0.00 ns/op) [ 325.69] # 1 threads, Tavg = 329.67 ns/op (σ = 0.00 ns/op) [ 329.67] # Measure: 1 threads, Tavg = 325.88 ns/op (σ = 0.00 ns/op) [ 325.88] # #------------------------------------------------------------- # GetDeclaredMethodNotOverridenSearch: run duration: 3,000 ms # # Warm up: # 1 threads, Tavg = 1,405.84 ns/op (σ = 0.00 ns/op) [ 1,405.84] # 1 threads, Tavg = 1,371.14 ns/op (σ = 0.00 ns/op) [ 1,371.14] # Measure: 1 threads, Tavg = 1,358.02 ns/op (σ = 0.00 ns/op) [ 1,358.02] # #------------------------------------------------------------- # GetPublicMethodNotOverridenSearch: run duration: 3,000 ms # # Warm up: # 1 threads, Tavg = 557.50 ns/op (σ = 0.00 ns/op) [ 557.50] # 1 threads, Tavg = 553.05 ns/op (σ = 0.00 ns/op) [ 553.05] # Measure: 1 threads, Tavg = 554.59 ns/op (σ = 0.00 ns/op) [ 554.59] # #------------------------------------------------------------- # END. ##############################################################

Here's the source of the test:

public class MethodSearchTest extends TestRunner {

static final Class[] paramTypes = {Class.class, int.class};

static class SMOverriden extends SecurityManager { @Override public void checkMemberAccess(Class<?> clazz, int which) { } }

static class SMNotOverriden extends SecurityManager { }

public static class GetDeclaredMethodOverridenSearch extends Test { @Override protected void doLoop(Loop loop, DevNull devNull1, DevNull devNull2, DevNull devNull3, DevNull devNull4, DevNull devNull5) { while (loop.nextIteration()) { try { SMOverriden.class.getDeclaredMethod("checkMemberAccess", paramTypes); devNull1.yield(true); } catch (NoSuchMethodException e) { devNull1.yield(false); } } } }

public static class GetPublicMethodOverridenSearch extends Test { @Override protected void doLoop(Loop loop, DevNull devNull1, DevNull devNull2, DevNull devNull3, DevNull devNull4, DevNull devNull5) { while (loop.nextIteration()) { try { devNull1.yield(SMOverriden.class.getMethod("checkMemberAccess", paramTypes).getDeclaringClass() != SecurityManager.class); } catch (NoSuchMethodException e) { throw new Error(e); } } } }

public static class GetDeclaredMethodNotOverridenSearch extends Test { @Override protected void doLoop(Loop loop, DevNull devNull1, DevNull devNull2, DevNull devNull3, DevNull devNull4, DevNull devNull5) { while (loop.nextIteration()) { try { SMNotOverriden.class.getDeclaredMethod("checkMemberAccess", paramTypes); devNull1.yield(true); } catch (NoSuchMethodException e) { devNull1.yield(false); } } } }

public static class GetPublicMethodNotOverridenSearch extends Test { @Override protected void doLoop(Loop loop, DevNull devNull1, DevNull devNull2, DevNull devNull3, DevNull devNull4, DevNull devNull5) { while (loop.nextIteration()) { try { devNull1.yield(SMNotOverriden.class.getMethod("checkMemberAccess", paramTypes).getDeclaringClass() != SecurityManager.class); } catch (NoSuchMethodException e) { throw new Error(e); } } } }

public static void main(String[] args) throws Exception { startTests(); doTest(GetDeclaredMethodOverridenSearch.class, 3000L, 1, 1, 1); doTest(GetPublicMethodOverridenSearch.class, 3000L, 1, 1, 1); doTest(GetDeclaredMethodNotOverridenSearch.class, 3000L, 1, 1, 1); doTest(GetPublicMethodNotOverridenSearch.class, 3000L, 1, 1, 1); endTests(); } }

Regards, Peter

...

Mandy