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

[David Holmes]

Hi Shafi,

On 1/08/2016 6:47 PM, Shafi Ahmad wrote:
> Hi David,
>
> Sorry for my half mail.
>
>> -----Original Message-----
>> From: David Holmes
>> Sent: Monday, August 01, 2016 5:32 AM
>> To: Shafi Ahmad; Coleen Phillimore; hotspot-runtime-dev at openjdk.java.net
>> Subject: Re: [8u] RFR JDK-8161144: Fix for JDK-8147451 failed: Crash in
>> Method::checked_resolve_jmethod_id(_jmethodID*)
>>
>> Hi Shafi,
>>
>> On 30/07/2016 1:10 AM, Shafi Ahmad wrote:
>>> Hi All,
>>>
>>> Could I have 2nd Reviewer's review for this change, please?
>>
>> I didn't see you respond to Coleen's query re JDK9. If this is not applicable to
>> JDK9 please add a 9-na label to the bug report.
>>
>> Looking at the code:
>>
>> +   void clear_method(Method* m) {
>> +     for (JNIMethodBlock* b = this; b != NULL; b = b->_next) {
>> +       for (int i = 0; i< number_of_methods; i++) {
>> +         if (b->_methods[i] == m) {
>> +           b->_methods[i] = NULL;
>> +         }
>> +       }
>> +     }
>> +     // not found
>> +   }
>>
>> Based on the "not found" comment I assume you intended to do a return
>> after NULLing out the method?
>>
>> Nit: need space after i in i<
>>
>> ---
>>
>> 1882 void Method::clear_jmethod_id(ClassLoaderData* loader_data) {
>> 1883   loader_data->jmethod_ids()->clear_method(this);
>> 1884 }
>>
>> Not sure why you felt the need to add a new member function for this
>> instead of just doing line #1883 directly at line #113
>
> Just to make it consistent with the existing method like void Method::clear_jmethod_ids(ClassLoaderData* loader_data).

I would not add to the API unnecessarily as it just makes the API harder 
to understand.

>> ---
>>
>>  >>> After this change I am seeing Method::is_method_id() is getting  >>>
>> called with NULL and I have done below change to avoid crash  >>>  >>> -
>> assert(m != NULL, "should be called with non-null method");  >>> +  if (m ==
>> NULL) {
>>  >>> +    return false;
>>  >>> +  }
>>
>> This is the only call I can see to is_method_id:
>
> Yes, this is the only call.
>
>>
>> 1870 Method* Method::checked_resolve_jmethod_id(jmethodID mid) {
>> 1871   if (mid == NULL) return NULL;
>> 1872   if (!Method::is_method_id(mid)) {
>> 1873     return NULL;
>> 1874   }
>>
>
>
>
>> So I don't see how it can be being passed NULL. If it is then you have a
>> problem!
>
> Here actual parameter 'mid"  of  method is_method_id is not null but this  we are calling another method resolve_jmethod_id(mid) which returns NULL i.e m becomes null in below code.
>
> 1885 bool Method::is_method_id(jmethodID mid) {
> 1886   Method* m = resolve_jmethod_id(mid);
> 1887   if (m == NULL) {
> 1888     return false;
> 1889   }

Ah I see - sorry. Thanks for clarifying.

David

>
> Regards,
> Shafi
>
>>
>> Thanks,
>> David
>> -----
>>
>>
>>> Regards,
>>> Shafi
>>>
>>>> -----Original Message-----
>>>> From: Coleen Phillimore
>>>> Sent: Monday, July 25, 2016 5:52 PM
>>>> To: hotspot-runtime-dev at openjdk.java.net
>>>> Subject: Re: [8u] RFR JDK-8161144: Fix for JDK-8147451 failed: Crash
>>>> in
>>>> Method::checked_resolve_jmethod_id(_jmethodID*)
>>>>
>>>>
>>>> This looks good.  Was this a backport or is it still broken in 9?
>>>> thanks,
>>>> Coleen
>>>>
>>>> On 7/25/16 7:53 AM, Shafi Ahmad wrote:
>>>>> Hi,
>>>>>
>>>>> Please review the small code change for bug: "JDK-8161144: Fix for
>>>>> JDK-
>>>> 8147451 failed: Crash in
>>>> Method::checked_resolve_jmethod_id(_jmethodID*)" on jdk8u-dev
>>>>>
>>>>> Summary:
>>>>> Method::deallocate_contents() should clear 'this' from list of
>>>>> Methods in
>>>> JNIMethodBlock, similarly to clear_all_methods() does it, when class
>>>> is unloaded.
>>>>> After this change I am seeing Method::is_method_id() is getting
>>>>> called with
>>>> NULL and I have done below change to avoid crash
>>>>>
>>>>> -  assert(m != NULL, "should be called with non-null method");
>>>>> +  if (m == NULL) {
>>>>> +    return false;
>>>>> +  }
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~shshahma/8161144/webrev/
>>>>> Jdk8 bug: https://bugs.openjdk.java.net/browse/JDK-8161144
>>>>>
>>>>> Test:  Run jprt
>>>>>
>>>>> Regards,
>>>>> Shafi
>>>>