RFR (L, tedious) 8160399: is_oop_or_null involves undefined behavior

[coleen.phillimore at oracle.com]

On 8/18/17 2:31 AM, Stefan Karlsson wrote:
> On 2017-08-18 01:47, Kim Barrett wrote:
>>> On Aug 17, 2017, at 7:30 PM, Stefan Karlsson 
>>> <stefan.karlsson at oracle.com> wrote:
>>>
>>> Hi Coleen,
>>>
>>> Could you motivate why you changed is_oop() to a static functions?
>>
>> Quoting myself from a comment in 8160399:
>>
>> For it [is_oop] to fail one has to take a non-oop, make it appear to 
>> be an oop, and
>> call an oop member function on that not really an oop. That sounds 
>> like UB to me.
>>
>
> So, what about the implementation of is_oop():
>
>  536 // used only for asserts and guarantees
>  537 inline bool oopDesc::is_oop(oop obj, bool ignore_mark_word) {
>
>  538   if (!check_obj_alignment(obj)) return false;
>  539   if (!Universe::heap()->is_in_reserved(obj)) return false;
>  540   // obj is aligned and accessible in heap
>  541   if (Universe::heap()->is_in_reserved(obj->klass_or_null())) 
> return false;
>  542
>  543   // Header verification: the mark is typically non-NULL. If we're
>  544   // at a safepoint, it must not be null.
>  545   // Outside of a safepoint, the header could be changing (for 
> example,
>  546   // another thread could be inflating a lock on this object).
>  547   if (ignore_mark_word) {
>  548     return true;
>  549   }
>  550   if (obj->mark() != NULL) {
>  551     return true;
>  552   }
>  553   return !SafepointSynchronize::is_at_safepoint();
>  554 }
>
> It calls both obj->klass_or_null() and obj->mark(). Shouldn't those 
> calls be considered UB as well? Should we fix those calls?

Hm, yes, we should probably fix them to use SafeFetchN(*(obj+8)_ and 
compare vtable for the address of the Klass pointer.   Looking at this 
code and callers, it seems the test for is_in_reserved(obj) is the most 
useful part of this code.

I could file an RFE to fix this UB inside of oopDesc::is_oop().  I think 
the usefulness of this change is that the UB isn't at each call site.

thanks,
Coleen

>
> StefanK
>
>>