RFR: 8181085: Race condition in method resolution may produce spurious NullPointerException (original) (raw)
Andrew Haley aph at redhat.com
Sat Jun 3 08:27:13 UTC 2017
- Next message: RFR: 8181085: Race condition in method resolution may produce spurious NullPointerException
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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:
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 "loadheapoop()" 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 loadheapoop(oop* p) { return *p; } ... template inline void G1RootRegionScanClosure::dooopnv(T* p) { // 1. load 'heapoop' from 'p' T heapoop = oopDesc::loadheapoop(p); if (!oopDesc::isnull(heapoop)) { // 2. Compiler reloads 'heapoop' from 'p' which may now be null! oop obj = oopDesc::decodeheapoopnotnull(heapoop); Do you mean that the compiler has not stashed heapoop somewhere and re-executes oopDesc::loadheapoop(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.
- Next message: RFR: 8181085: Race condition in method resolution may produce spurious NullPointerException
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]