RFR (M): 7198429: need checked categorization of caller-sensitive methods in the JDK
Christian Thalinger
christian.thalinger at oracle.com
Wed Mar 20 01:02:53 UTC 2013
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.
-- Chris
More information about the core-libs-dev
mailing list