Review request for 7198429: need checked categorization of caller-sensitive methods in the JDK

[John Rose]

On Mar 27, 2013, at 10:35 AM, Mandy Chung <mandy.chung at 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/

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:

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.  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.)

-- ClassLoader.java

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

-- MethodHandleNatives.java

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

Good cleanups!

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

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

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

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;


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(...);
}

(And a similar comment for Class.java.)

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?  Does it process all of rt.jar?  How long does it take to complete?

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?