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

[Mandy Chung]

On 4/1/13 5:24 PM, John Rose wrote:
> On Mar 27, 2013, at 10:35 AM, Mandy Chung <mandy.chung at oracle.com 
> <mailto: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/ 
>> <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.

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