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

[Jiangli Zhou]

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 <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: http://cr.openjdk.java.net/~shshahma/8147451/webrev.01/
> 
> 
> 
> Regards,
> 
> Shafi
> 
> 
> 
> From: Serguei Spitsyn 
> Sent: Wednesday, June 08, 2016 11:40 AM
> To: 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"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