[8u] RFR: JDK-8147451: Crash in Method::checked_resolve_jmethod_id(_jmethodID*)

[Shafi Ahmad]

Hi Serguei,

 

Thanks for the review comment.  

 

I will send the updated webrev  link after removing the suggested unreachable code. 

 

Regards,

Shafi 

 

From: Serguei Spitsyn 
Sent: Thursday, June 09, 2016 3:03 AM
To: Jiangli Zhou; Shafi Ahmad
Cc: hotspot-runtime-dev at openjdk.java.net; Coleen Phillimore
Subject: Re: [8u] RFR: JDK-8147451: Crash in Method::checked_resolve_jmethod_id(_jmethodID*)

 

Hi Jiangli,

Nice catch!

The check_resolve_jmethod_id() is used in the jniCheck contexts, so it can be Ok not to be so light-weight.
It seems, the new is_method_id() is a nice check to have there.
Besides the whitebox.cpp the validate_jmethod_id() function is the only caller of it.

But it does not look right to call the is_method_id in the validate_jmethod_id().
I'd suggest to remove the lines:

  // jmethodIDs are supposed to be weak handles in the class loader data,

  // but that can be expensive so check it last

  else if (!Method::is_method_id(method_id)) {

    ReportJNIFatalError(thr, fatal_non_weak_method);

  }


The only impact is that a different JNI fatal error will be reported.
Not sure, it is important though.

Thanks,
Serguei


On 6/8/16 11:24, Jiangli Zhou wrote:

Hi Shafi, 

 

The following in src/share/vm/prims/jniCheck.cpp calls Method::checked_resolve_jmethod_id() as a quick check before  calling the Method::is_method_id(). It seems Method::checked_resolve_jmethod_id() is expected to be a light-weight check. Your change calling is_method_id() from check_resolve_jmethod_id() changes that. Also, is_method_id() will be called twice in the jniCheck::validate_jmethod_id() call path.

 

Method* jniCheck::validate_jmethod_id(JavaThread* thr, jmethodID method_id) {

  ASSERT_OOPS_ALLOWED;

  // do the fast jmethodID check first

  Method* moop = Method::checked_resolve_jmethod_id(method_id);

  if (moop == NULL) {

    ReportJNIFatalError(thr, fatal_wrong_class_or_method);

  }

  // jmethodIDs are supposed to be weak handles in the class loader data,

  // but that can be expensive so check it last

  else if (!Method::is_method_id(method_id)) {

    ReportJNIFatalError(thr, fatal_non_weak_method);

  }

  return moop;

}

 

Thanks,

Jiangli

 

On Jun 8, 2016, at 2:08 AM, Shafi Ahmad <HYPERLINK "mailto:shafi.s.ahmad at oracle.com"shafi.s.ahmad at oracle.com> wrote:

 

Hi,



Thanks Serguei for looking into it.



Please find the updated webrev with suggested code change.

Updated webrev link: HYPERLINK "http://cr.openjdk.java.net/%7Eshshahma/8147451/webrev.01/"http://cr.openjdk.java.net/~shshahma/8147451/webrev.01/



Regards,

Shafi



From: Serguei Spitsyn 
Sent: Wednesday, June 08, 2016 11:40 AM
To: HYPERLINK "mailto:hotspot-runtime-dev at openjdk.java.net"hotspot-runtime-dev at openjdk.java.net; Shafi Ahmad
Cc: Coleen Phillimore
Subject: Re: [8u] RFR: JDK-8147451: Crash in Method::checked_resolve_jmethod_id(_jmethodID*)



Once again with Shafi's email address added ...

Hi Shafi,

You did not reply on my email above.
It can be because your email address was not directly included into the list.

Thanks,
Serguei


On 6/3/16 17:32, HYPERLINK "mailto:serguei.spitsyn at oracle.com"HYPERLINK "mailto:serguei.spitsyn at oracle.com"serguei.spitsyn at oracle.com wrote:

Hi Shafi,

I agree that this change is safe.
However, there are still two more spots that need to be fixed in the jdk8:

  // During class unloading the methods are cleared, which is different
  // than freed.
  void clear_all_methods() {
    for (JNIMethodBlock* b = this; b != NULL; b = b->_next) {
      for (int i = 0; i< number_of_methods; i++) {
-      _methods[i] = NULL;
+      b->_methods[i] = NULL;
      }
    }
  }
@@ -1799,7 +1811,7 @@
    int count = 0;
    for (JNIMethodBlock* b = this; b != NULL; b = b->_next) {
      for (int i = 0; i< number_of_methods; i++) {
-        if (_methods[i] != _free_method) count++;
+        if (b->_methods[i] != _free_method) count++;
      }
    }
    return count;
@@ -1871,6 +1883,10 @@
  return o;
};


You can find this information in one of the bug report comments.

Thanks,
Serguei



On 6/3/16 15:18, Coleen Phillimore wrote: 

This seems like a safe change. Coleen On 5/24/16 4:34 AM, Shafi Ahmad wrote: 

Hi,   Please review the small code change for bug: "JDK-8147451: Crash in Method::checked_resolve_jmethod_id(_jmethodID*)" on jdk8u-dev   Summary: resolve_jmethod_id() is getting called with invalid jmethodID and there is no check for validity of the method id inside this function. So before calling resolve_jmethod_id() we should check its validity. This code change add this check. Also inside Method::is_method_id() we are not checking return value of method_holder(). It may return NULL if method id is not valid so I have added null check for this too.   Webrev: HYPERLINK "http://cr.openjdk.java.net/%7Eshshahma/8147451/webrev.00/"http://cr.openjdk.java.net/~shshahma/8147451/webrev.00/ Jdk8 bug: https://bugs.openjdk.java.net/browse/JDK-8147451   Test:  Run jprt   Regards, Shafi