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

[Christian Thalinger]

On Mar 19, 2013, at 6:02 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:

> 
> On Mar 19, 2013, at 5:21 PM, John Rose <john.r.rose at oracle.com> wrote:
> 
>> On Mar 14, 2013, at 8:31 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>> 
>>> [This is the HotSpot part of JEP 176]
>>> 
>>> http://cr.openjdk.java.net/~twisti/7198429
>>> 
>>> 7198429: need checked categorization of caller-sensitive methods in the JDK
>>> Reviewed-by:
>> 
>> 
>> Over all, great work on a tricky problem.  I'd add a few asserts and tweak a couple of lines; see below.  Reviewed as is or with suggested changes. — John
>> 
>> --- Method::is_ignored_by_security_stack_walk
>> I would like to see reflect_invoke_cache go away some day.  Would it be possible to strengthen the asserts to prove that it is an expensive alias for an intrinsic_id check?  (I realize this is a question mainly for folks working on JVMTI.)
> 
> That's what I tried to do:  if the intrinsic_id == _invoke it also must be the same method in reflect_invoke_cache.  More than that would mean to enhance ActiveMethodOopsCache because you can't ask for methods in the cache.
> 
>> 
>> --- JVM_GetCallerClass
>> Suggest an assert for vfst.method() == NULL.  Should not happen, and previous code would apparently have crashed already, but...
>> 
>> (The corner case I'm thinking of is a compiled frame with nmethod::method returning null after nmethod::make_unloaded.  Should not happen.)
> 
> Sure, I can add that assert but there is other code in jvm.cpp that relies on the fact that vfst.method() is non-null.  We should add asserts in all that places but that's for another RFE.
> 
>> 
>> --- JVM_GetClassContext
>> What do these lines do:
>> +   // Collect method holders
>> +   GrowableArray<KlassHandle>* klass_array = new GrowableArray<KlassHandle>();
>> 
>> It looks like a paste-o from another source base.
> 
> Left over.  I filed an RFE for that improvement:
> 
> JDK-8010124: JVM_GetClassContext: use GrowableArray instead of KlassLink
> 
>> 
>> --- LibraryCallKit::inline_native_Reflection_getCallerClass
>> 
>> I believe this assertion, but I would prefer to see it checked more forcibly:
>> +   // NOTE: Start the loop at depth 1 because the current JVM state does
>> +   // not include the Reflection.getCallerClass() frame.
>> 
>> Not sure there is a good way to do this.  But, perhaps put the comment here:
>>  case 0:
>>    // ...comment...
>>    ShouldNotReachHere();
> 
> How about:
> 
>    case 0:
>      fatal("current JVM state does not include the Reflection.getCallerClass() frame");
>      break;
> 
>> 
>> Also, if something goes wrong with caller sensitivity, we just get a "return false".  Perhaps do a "caller_jvm=NULL;break" to branch to the pretty failure message?  That makes it slightly easier to see what happened.
> 
> It seems easier to add printing code to the case statement:
> 
>    case 1:
>      // Frame 0 and 1 must be caller sensitive (see JVM_GetCallerClass).
>      if (!m->caller_sensitive()) {
> #ifndef PRODUCT
>        if ((PrintIntrinsics || PrintInlining || PrintOptoInlining) && Verbose) {
>          tty->print_cr("  Bailing out: CallerSensitive annotation expected at frame %d", n);
>        }
> #endif
>        return false;  // bail-out; let JVM_GetCallerClass do the work
>      }
>      break;
> 
>> 
>> The LogCompilation switch should leave a "paper trail".  Actually, I see that LogCompilation doesn't mention failed intrinsic inlines.  Rats.  At least PrintInlining or PrintIntrinsics (diagnostic flags) will give us some leverage if we need to debug.
>> 
>> --- JVM_RegisterUnsafeMethods
>> That's an improvement.  Thanks.
>> 
>> (A nagging worry:  How big are those static tables getting?)
> 
> We could remove some very old ones like 1.4.0 and 1.4.1.  This time, next time?
> 
>> 
>> --- vframeStreamCommon::security_get_caller_frame
>> This now does something odd if depth < 0.  Suggest an assert.
> 
> The behavior with depth < 0 in the current code is even worse.  An assert is a good idea.  As discussed I want to remove that method in the future because its uses are dubious.

I forgot to update the webrev.  Here you go:

http://cr.openjdk.java.net/~twisti/7198429/

-- Chris