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

[serguei.spitsyn at oracle.com]

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 <shafi.s.ahmad at oracle.com 
>> <mailto: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/ 
>> <http://cr.openjdk.java.net/%7Eshshahma/8147451/webrev.01/>
>>
>>
>>
>> Regards,
>>
>> Shafi
>>
>>
>>
>> From: Serguei Spitsyn
>> Sent: Wednesday, June 08, 2016 11:40 AM
>> To: hotspot-runtime-dev at openjdk.java.net 
>> <mailto: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 
>> <mailto: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/ 
>> <http://cr.openjdk.java.net/%7Eshshahma/8147451/webrev.00/> Jdk8 bug: 
>> https://bugs.openjdk.java.net/browse/JDK-8147451   Test:  Run jprt 
>>   Regards, Shafi
>