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

[Shafi Ahmad]

Hi Jiangli,

 

Thanks for the review comment. 

 

Yes you are right this change makes method 'jniCheck::validate_jmethod_id' more heavy. 

In current implementation of 'jniCheck::validate_jmethod_id' the validation  of method_id is done after its usage which seems to me not correct.  

The variable method_id is used as actual parameter of method 'Method::checked_resolve_jmethod_id()' and then later we are validating about it correctness. 

 

Current implementation looks like first we are dereferencing pointer variable and then we are doing its NULL check. 

 

With my change the below code is redundant as it will never be executed. Ideally I should remove this code.

 

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

  }

 

Regards,

Shafi

 

From: Jiangli Zhou 
Sent: Wednesday, June 08, 2016 11:54 PM
To: Shafi Ahmad
Cc: Serguei Spitsyn; hotspot-runtime-dev at openjdk.java.net
Subject: Re: [8u] RFR: JDK-8147451: Crash in Method::checked_resolve_jmethod_id(_jmethodID*)

 

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