RFR: 8181085: Race condition in method resolution may produce spurious NullPointerException

[Andrew Haley]

On 30/05/17 16:40, Volker Simonis wrote:
> On Mon, May 29, 2017 at 10:55 PM, David Holmes <david.holmes at oracle.com> wrote:
>> <trimming>
>>
>> On 30/05/2017 3:02 AM, Volker Simonis wrote:
>>>
>>> Sorry for constantly "spamming" this thread with another problem (i.e.
>>> JDK-8129440 [2]) but I still think that it is related and important.
>>> In its current state, the way how "load_heap_oop()" and its
>>> application works is broken. And this is not because of a problem in
>>> OrderAccess, but because of missing compiler barriers:
>>>
>>> static inline oop       load_heap_oop(oop* p)       { return *p; }
>>> ...
>>> template <class T>
>>> inline void G1RootRegionScanClosure::do_oop_nv(T* p) {
>>>    // 1. load 'heap_oop' from 'p'
>>>    T heap_oop = oopDesc::load_heap_oop(p);
>>>    if (!oopDesc::is_null(heap_oop)) {
>>>      // 2. Compiler reloads 'heap_oop' from 'p' which may now be null!
>>>      oop obj = oopDesc::decode_heap_oop_not_null(heap_oop);
>>
>>
>> Do you mean that the compiler has not stashed heap_oop somewhere
>> and re-executes oopDesc::load_heap_oop(p) again? That would be
>> quite nasty I think in general as it breaks any logic that wants to
>> read a non-local variable once to get it into a local and reuse
>> that knowing that it won't change even if the real variable does!

Note that such racy code is undefined behaviour in C++11 (and IMO
before that as well, but there wasn't anything portable you could
do about it.)

> Yes, that's exactly what I mean and that's exactly what we've observed
> on AIX with xlc. Notice that the compiler is free to do such
> transformations if the load is not from a volatile field. That's why
> we've opened the bug and fixed out internal version. But we still
> think this fix needs to go into OpenJDK as well.

Given that the code above is now explicitly UB in C++ (and probably C
too, but I haven't looked) it'll have to be changed eventually to

  atomic_load_explicit(&p, memory_order_relaxed);

This approach has some advantages from the point of view of code
quality, too.  The compiler can do a better job if it knows what's
going on.  Maybe it would be worth a #ifdef C++11 set of OrderAccess
functions.

Andrew.