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

[Markus Gronlund]

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 think you also need:

#include "runtime/stubRoutines.hpp

Can't really comment on the use of SafeFetchN() as I am not familiar with it? Do all platforms support this?

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