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

[Peter Levart]

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

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