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

[Jesper Wilhelmsson]

Den 3/2/16 kl. 16:14, skrev Daniel D. Daugherty:
> On 2/3/16 8:10 AM, Markus Gronlund wrote:
>> Hi Coleen,
>>
>> Thanks for looking into this. Maybe it could be simplified like:
>
> Very nicely done...
>
>>
>> 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) {
>
> In keeping with our latest style migration:
>
>      if (NULL == this) {
>
> Yes, I know I have said I don't like it... but we're
> going that way and it's starting to grow on me... :-)

Don't give in Dan!
I think this truly awful and everyone I've asked around here dislikes this 
style. Is there a reason to turn things around like this?
(Compilers from the Jurassic age cant detect "if (this = NULL)" is not a valid 
argument.)
/Jesper


>
> Dan
>
>
>>      return false;
>>    }
>>    // Quick sanity check on pointer.
>>    if ((intptr_t(this) & (wordSize-1)) != 0) {
>
> Spaces around the '-' please...
>
> Dan
>
>>      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
>>>>>>>>
>