RFR(S): 8199813: SIGSEGV in ThreadsList::includes()

[Daniel D. Daugherty]

Hi David!

Thanks for the review. Replies embedded below...


On 3/25/18 6:44 PM, David Holmes wrote:
> Hi Dan,
>
> On 24/03/2018 6:55 AM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I have a small fix for a Thread-SMR bug:
>>
>> JDK-8199813 SIGSEGV in ThreadsList::includes()
>> https://bugs.openjdk.java.net/browse/JDK-8199813
>>
>> Here's the webrev URL:
>>
>> http://cr.openjdk.java.net/~dcubed/8199813-webrev/0-for-jdk-hs/
>>
>> The failure mode for this bug is a SIGSEGV in ThreadsList::includes().
>>
>> The cause of this failure is use of a ThreadsListHandle by a JavaThread
>> that is not yet on Threads list. This failure mode is intermittent since
>> the ThreadsListHandle refers to a ThreadsList that may or may not get
>> deleted by a racing thread. In particular, it has to get deleted in such
>> a way that use of the ThreadsList causes a crash, e.g., page is freed.
>>
>> This fix has been tested on my Solaris-X64 server using my usual 24+
>> hour Thread-SMR stress testing config. It has also been through Mach5
>> hs-tier[1-5] testing. There have been no unexpected failures.
>>
>> This fix is currently going thru a Mach5 builds-tier1, jdk-tier[1-3],
>> and hs-tier[1-3] run after resyncing; we don't want any new asserts()
>> popping up in the jdk/jdk CI...
>>
>> Since this is a race fix, here are some details:
>>
>> As is usual for race fixes, there are more comments than code here.
>>
>> src/hotspot/share/runtime/thread.cpp
>>      Use of a ThreadsListHandle by verify_not_published() before the
>>      JavaThread is on the Thread list; this is the minimal fix for
>>      the failure mode.
>
> Okay.
>
>>      Save the shutdown thread in Threads::destroy_vm(). Also add a
>>      comment explaining why IdealGraphPrinter::clean_up() is done
>>      where it is.
>
> Why do you need the explicit VM_Exit::set_shutdown_thread(thread) 
> instead of doing this within VM_Exit::set_vm_exited() as previously 
> the case?

For the Threads::destroy_vm() use case, we call
VM_Exit::set_shutdown_thread(thread) before the call to

  L4267:   IdealGraphPrinter::clean_up();

which uses a ThreadsListHandle iterator to walk the Threads list.

The clean_up() call must be after the VMThread has done the final
safepoint and the shutdown thread must be known before the call
to clean_up(). The call to clean_up must happen before the call
to VM_Exit::set_vm_exited() so I had to separate the setting of
the shutdown thread from VM_Exit::set_vm_exited().

Hopefully this makes (some kind of) sense.

Dan


>
> Otherwise no other comments.
>
> Thanks,
> David
>
>> src/hotspot/os/linux/os_linux.cpp
>>      Another use of a ThreadsListHandle before the JavaThread is on
>>      the Threads list; found during testing of the fix by the new
>>      assert() in the ThreadsListHandle ctr.
>>
>> src/hotspot/share/runtime/threadSMR.cpp
>>      Add an assertion to the ThreadsListHandle ctr that catches
>>      use of a ThreadsList that Threads::threads_do() won't be
>>      able to protect.
>>
>>      Note: There is an exemption for the shutdown thread here.
>>
>> src/hotspot/share/runtime/vm_operations.hpp
>>      Expose the shutdown thread so the new assert() in the
>>      ThreadsListHandle ctr can do its job.
>>
>> src/hotspot/share/runtime/vm_operations.cpp
>>      VM_Exit::_shutdown_thread should be volatile since it is
>>      set by one thread and examined lock free by one or more
>>      others.
>>
>>      The callers of VM_Exit::set_vm_exited() should set the
>>      _shutdown_thread field since they have the earliest context.
>>
>>      Save the shutdown thread in VM_Exit::doit().
>>
>>
>> Thanks, in advance, for any comments, suggestions, or feedback.
>>
>> Dan
>>