Fix shutdown race between mono_thread_manage and mono_thread_detach_internal. (#5599)
authorJohan Lorensson <lateralusx.github@gmail.com>
Tue, 26 Sep 2017 17:25:23 +0000 (19:25 +0200)
committerLudovic Henry <luhenry@microsoft.com>
Tue, 26 Sep 2017 17:25:23 +0000 (13:25 -0400)
commited1884bb9b43ddd69daba52302494ad2d02bbbd9
tree518f98666114982a6c35d89c1096dc0d7325d557
parent8c0645dab9ccc90da201e714de20804ba5339cfc
Fix shutdown race between mono_thread_manage and mono_thread_detach_internal. (#5599)

* Add support for mono_threads_add_joinable_thread and friends on Windows.

Current Windows implementation doesn't have support for the joinable thread
implementation in threads.c. Joinable threads are used by the runtime during
shutdown to coordinate with exit of platform threads.

Since threads detaching and exiting during shutdown could still use or provide
important resources, like GC memory, lacking support for this feature could
potentially expose the implementation to various shutdown race conditions.

Commit also change requested permission on a thread handle when opening it for
synchronization purposes. No need to request all thread access when handle is only
used for synchronization, like in WaitForXXX API calls.

* Fix race between mono_thread_detach_internal and mono_thread_manage.

There is a race during shutdown when main tread is running mono_thread_manage and
attached threads are running mono_thread_detach_internal. The problem is related to
the removal from the registered threads list since mono_thread_manage will pick
threads to wait upon from that list. If a thread removes itself from the list
using mono_thread_detach_internal it will then race against runtime shutdown
and the removal of used resources still in used by mono_thread_detach_intenral,
like MonoInternalThread.

This race could trigger the assert seen in bugzilla:

https://bugzilla.xamarin.com/show_bug.cgi?id=58770

or other undefined behavior depending on where the detached thread resume execution
after main thread has shutdown the runtime, but not yet terminated the process.

The fix makes sure threads that are detaching during shutdown ends up on the joinable
threads list. Threads on that list will be waited upon by the main thread during shutdown,
mono_thread_cleanup. This will make sure the detached thread finishes the remaining of
mono_thread_detach_internal and potential other code still using GC memory during shutdown.
Threads already on the threads list will already be waited upon by mono_thread_manage during
shutdown (if not removed by mono_thread_detach_internal triggering this race), so this change
won't add additional threads to be waited up by the runtime. It just makes the shutdown more
reliable using already available concepts (like the joinable threads list).

* Fix warning C4141 on Windows MSVC compiler.

* Updated with review feedback.

* Prevent race between finalizer and main thread to join same thread multiple times.

Checking against joinable_threads list in mono_threads_add_joinable_thread is not enough.
There is a small chance that the same runtime thread could get on the list twice since
mono_threads_add_joinable_thread could be called multiple times for the the same tid.
This will work under Windows but is undefined behavior on POSIX's platforms.

This could happen if the finalizer thread calls mono_threads_join_threads when a thread
is detaching and have added itself to the joinable_threads, but before unregister_thread
has been called. Finalizer thread will then take tid from list and wait, this will cause
the second call to mono_threads_add_joinable_thread to not find itself in the joinable_threads
and add tid a second time. NOTE, this race only applies to runtime threads added to the
joinable_threads list.

Fix adds an additional method to add runtime threads to the list. This method will only add to
list if its indeed a runtime thread and doesn’t already have an outstanding pending native join
request. This method is called by code previously checking runtime_thread flag before adding to
joinable_threads list. This will prevent the scenario when the same MonoThreadInfo add its tid to
the joinable_threads list multiple times due to above race condition.
mono/metadata/gc.c
mono/metadata/sgen-mono.c
mono/metadata/threads-types.h
mono/metadata/threads.c
mono/utils/mono-threads-windows.c
mono/utils/mono-threads.h
mono/utils/unlocked.h