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

[John Rose]

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

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

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

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

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.

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?)

--- vframeStreamCommon::security_get_caller_frame
This now does something odd if depth < 0.  Suggest an assert.