RFR: 8267842: SIGSEGV in get_current_contended_monitor

[Daniel D.Daugherty]

On Thu, 27 May 2021 09:56:22 GMT, Martin Doerr <mdoerr at openjdk.org> wrote:

> We need a fix for crashes in get_current_contended_monitor due to concurrent modification of memory locations which are not declared volatile. See bug for details.

Thumbs up.

The code that you're fixing is indeed a special case and I wrote the new test
(serviceability/monitoring/ThreadInfo/GetLockOwnerName/GetLockOwnerName.java)
specifically to stress the crashing code path. Unfortunately, I have never seen the
new test fail in our test in Mach5 or in my testing in my lab. We did get a single closed
test failure back on 2021.03.20 which is what started me down the path of creating
the new test. That single failure has never reproduced in the original closed test nor
in the targeted test that I wrote (GetLockOwnerName.java).

The similar usage in JVM/TI is safe for exactly the reasons explained in the comment so a
general Atomic::load() solution in current_waiting_monitor() or current_pending_monitor()
is not necessary.

I think your solution of adding `volatile` to `wait_obj` and `enter_obj` is a good solution.
I would like to see a comment added to explain the need for the `volatile`. I'll add an
embedded comment in the other PR view.

Using either `wait_obj` or `enter_obj` after it has been cleared from the JavaThread is
safe. The ObjectMonitor can only have gone through the first stage of async deflation.
The ObjectMonitor's memory cannot be freed until after a handshake with all threads
is completed and that cannot happen while this thread is executing the code that is
using `wait_obj` or `enter_obj`.

src/hotspot/share/services/threadService.cpp line 232:

> 230:   // ObjectMonitor by the time this object reference is processed
> 231:   // by the caller.
> 232:   ObjectMonitor* volatile wait_obj = thread->current_waiting_monitor();

Please add a comment above this declaration. Something like:


// Using 'volatile' to prevent the compiler from generating code that
// reloads 'wait_obj' from memory when used below.

src/hotspot/share/services/threadService.cpp line 239:

> 237:     obj = wait_obj->object();
> 238:   } else {
> 239:     ObjectMonitor* volatile enter_obj = thread->current_pending_monitor();

Please add a comment above this declaration. Something like:


// Using 'volatile' to prevent the compiler from generating code that
// reloads 'enter_obj' from memory when used below.

-------------

Marked as reviewed by dcubed (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4224