RFR 8146984: SIGBUS: bool Method::has_method_vptr(const void*)+0xc

[Coleen Phillimore]

Markus,  Thank you for reviewing.


On 2/3/16 10:10 AM, Markus Gronlund wrote:
> Hi Coleen,
>
> Thanks for looking into this. Maybe it could be simplified like:
>
> bool Method::has_method_vptr(const void* ptr) {
>    // Use SafeFetch to check if this is a valid pointer first
>    // This assumes that the vtbl pointer is the first word of a C++ object.
>    // This assumption is also in universe.cpp patch_klass_vtable
>    if (1 == SafeFetchN((intptr_t*)ptr, intptr_t(1))) {
>      return false;
>    }
>    const Method m;
>    return dereference_vptr(&m) == ptr;
> }
>
> // Check that this pointer is valid by checking that the vtbl pointer matches
> bool Method::is_valid_method() const {
>    if (this == NULL) {
>      return false;
>    }
>    // Quick sanity check on pointer.
>    if ((intptr_t(this) & (wordSize-1)) != 0) {
>      return false;
>    }
>
>    return has_method_vptr(this);
> }

I'll change as suggested, it looks more nicer and more compact.

>
> I think you also need:
>
> #include "runtime/stubRoutines.hpp

Ok, I usually don't use precompiled headers but will include it because 
it's used.
>
> Can't really comment on the use of SafeFetchN() as I am not familiar with it? Do all platforms support this?

Yes, I think so.  I believe I sponsored the patch and the patch to make 
it work on Zero too.

Thanks!
Coleen

>
> Looks like a good improvement for stability.
>
> No need to see any updated reviews.
>
> Thanks
> Markus
>
>
> -----Original Message-----
> From: Coleen Phillimore
> Sent: den 2 februari 2016 18:54
> To: hotspot-dev at openjdk.java.net; Lindenmaier, Goetz
> Subject: Re: RFR 8146984: SIGBUS: bool Method::has_method_vptr(const void*)+0xc
>
>
> Goetz, Can you review this since it's using SafeFetchN?
>
> thanks,
> Coleen
>
> On 2/1/16 1:04 PM, Coleen Phillimore wrote:
>>
>> Thanks Dan!
>>
>> On 2/1/16 12:21 PM, Daniel D. Daugherty wrote:
>>> On 1/30/16 7:39 AM, Coleen Phillimore wrote:
>>>> I've moved the SafeFetch to has_method_vptr as suggested and retested.
>>>>
>>>> http://cr.openjdk.java.net/~coleenp/8146984.02/webrev/
>>> src/share/vm/oops/method.cpp
>>>      (old) L2114:     return has_method_vptr((const void*)this);
>>>      (new) L2120:       return has_method_vptr(this);
>>>          Just curious. I don't see anything that explains why the
>>>          cast is no longer needed (no type changes). Was this
>>>          simply cleaning up an unnecessary cast?
>> The cast is unnecessary.  I didn't add it back when I added the call
>> to has_method_vptr back.
>>
>> thanks,
>> Coleen
>>
>>> Thumbs up.
>>>
>>> Dan
>>>
>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>> On 1/29/16 11:58 AM, Coleen Phillimore wrote:
>>>>>
>>>>> On 1/29/16 11:02 AM, Daniel D. Daugherty wrote:
>>>>>> On 1/29/16 5:46 AM, Coleen Phillimore wrote:
>>>>>>> Summary: Add address check and use SafeFetchN for Method* vptr
>>>>>>> access when Method* may be bad pointer.
>>>>>>>
>>>>>>> Tested with RBT and failing test case (reproduced 1 in 100 times)
>>>>>>> with fatal in the 'return's in the change to verify.
>>>>>>>
>>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8146984/
>>>>>> This one caught my eye because it has to do with sampling...
>>>>> I should mention sampling in all my RFRs then!
>>>>>> src/share/vm/oops/method.cpp
>>>>>>      The old code checked "!is_metaspace_object()" and used
>>>>>>      has_method_vptr((const void*)this).
>>>>>>
>>>>>>      The new code skips the "!is_metaspace_object()" check even
>>>>>> after sanity
>>>>>>      checking the pointer, but you don't really explain why that's OK.
>>>>> is_metaspace_object is a very expensive check.   It has to traverse
>>>>> all the metaspace mmap chunks.   The new code is more robust in
>>>>> that it sanity checks the pointer first but uses Safefetch to get
>>>>> the vptr.
>>>>>
>>>>>
>>>>>>      The new code also picks up parts of Method::has_method_vptr()
>>>>>> which
>>>>>>      makes me wonder if that's the right place for the fix. Won't
>>>>>> other
>>>>>>      callers to Method::has_method_vptr() be subject to the same
>>>>>> crashing
>>>>>>      mode? Or was the crashing mode only due to the
>>>>>> "!is_metaspace_object()"
>>>>>>      check...
>>>>> I should have moved the SafeFetch in to the has_method_vptr. I
>>>>> can't remember why I copied it now.  It crashed because the pointer
>>>>> was in metaspace (is_metaspace_object returned true) but wasn't
>>>>> aligned, but the pointer could come from anywhere.
>>>>>
>>>>> Thanks,   I'll test out this fix and resend it.
>>>>> Coleen
>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8146984
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>>