RFR (M): 7198429: need checked categorization of caller-sensitive methods in the JDK

[Mandy Chung]

On 3/19/2013 5:29 PM, Christian Thalinger wrote:
> On Mar 19, 2013, at 1:14 PM, Mandy Chung <mandy.chung at oracle.com> wrote:
>
>> I do a partial review in particular to make sure the jdk and hotspot change are in sync.
>>
>> javaClasses.hpp - MN_CALLER_SENSITIVE and MN_SEARCH_SUPERCLASSES have the same value.  Should they be different?
>>
>> 1057     MN_CALLER_SENSITIVE     = 0x00100000, // @CallerSensitive annotation detected
>> 1061     MN_SEARCH_SUPERCLASSES  = 0x00100000, // walk super classes
> They can have the same value because they are used for different things in different places.  I talked to John about this and he said that the MN_SEARCH_* guys don't add much value and might be removed.

Thanks. That's fine.

>>
>> method.hpp - Is caller_sensitive set to true if @CallerSensitive annotation is present and must be loaded by null class loader?  Does it only check annotations if the class of that method is defined by the null class loader?  Per our offline discussion early, classes loaded by the extension class loader may also be caller-sensitive.
> Right.  I forgot to add that code.  Here is an incremental webrev:
>
> http://cr.openjdk.java.net/~twisti/7198429/edit/
>
> And the full thing:
>
> http://cr.openjdk.java.net/~twisti/7198429/
>
> Let me know if that works for you.

I'll take your patch and let you know.

>> If a method calls Reflection.getCallerClass but its class is defined by other class loader (non-null and not extension class loader), your fix will throw InternalError with the same error message even if that method is annotated with @CS.  Is there a way to improve the error message so that we can differentiate this case (i.e. @CS is present but not supported)?
> Not easily.  We set a flag on the method when parse the class.  At that point we decide if the annotation is there or not.  If the annotation is not allowed in parsed class because it's not loaded by the right class loader then it does not "exist".

701         THROW_MSG_NULL(vmSymbols::java_lang_InternalError(), err_msg("CallerSensitive annotation expected at frame %d", n));

Perhaps it could check the class loader of the class of the method or print out additional info?  It should be rare to run into this error but it would greatly help diagnosing the problem if @CS method loaded by unexpected class loader.

>> jvm.cpp: have you considered adding a new entry point instead of having JVM_GetCallerClass to behave differently depending on the existence of sun.reflect.CallerSensitive class? There are pros and cons of both options. Having a new entry point is cleaner and enables the opportunity to remove JVM_GetCallerClass(int) in the future.  I am fine with either approach but just to bring it up.
> Yes.  I talked to Vladimir about that yesterday.  The better solution seems to be to leave the old entry point.  If we add a new one that kind of adds a new method to the "official" VM interface.  Other VM implementors would have to implement that one as well because the JDK then links to this new method.
>

Yup, that's one of the cons of adding a new entry point. Other VM
implementation may need to support JVM_GetCallerClass(-1) to run on
our JDK library implementation in either approach.

Good and keep the code entry point. I'll send out the jdk change for code review.

Mandy