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.

diff --git a/mono/metadata/gc.c b/mono/metadata/gc.c
index 702857858eb25c4d50ca5485e92d32a91c37cd28..4cda6ddb320b7b922fc56033c80a6b3594322aed 100644 (file)
--- a/mono/metadata/gc.c
+++ b/mono/metadata/gc.c
@@ -997,7 +997,7 @@ mono_gc_cleanup (void)
                                        ret = guarded_wait (gc_thread->handle, MONO_INFINITE_WAIT, FALSE);
                                        g_assert (ret == MONO_THREAD_INFO_WAIT_RET_SUCCESS_0);
 
-                                       mono_threads_add_joinable_thread (GUINT_TO_POINTER (gc_thread->tid));
+                                       mono_threads_add_joinable_thread ((gpointer)(MONO_UINT_TO_NATIVE_THREAD_ID (gc_thread->tid)));
                                        break;
                                }
 
@@ -1022,7 +1022,7 @@ mono_gc_cleanup (void)
 
                                        g_assert (ret == MONO_THREAD_INFO_WAIT_RET_SUCCESS_0);
 
-                                       mono_threads_add_joinable_thread (GUINT_TO_POINTER (gc_thread->tid));
+                                       mono_threads_add_joinable_thread ((gpointer)(MONO_UINT_TO_NATIVE_THREAD_ID (gc_thread->tid)));
                                        break;
                                }
 

diff --git a/mono/metadata/sgen-mono.c b/mono/metadata/sgen-mono.c
index ac977c0e5902e4133b05074a7edab4b20680a20a..33d1e52dd8d54de2bb03247eb072253093d70f88 100644 (file)
--- a/mono/metadata/sgen-mono.c
+++ b/mono/metadata/sgen-mono.c
@@ -2261,8 +2261,7 @@ sgen_client_thread_detach_with_lock (SgenThreadInfo *p)
 
        tid = mono_thread_info_get_tid (p);
 
-       if (p->client_info.info.runtime_thread)
-               mono_threads_add_joinable_thread ((gpointer)tid);
+       mono_threads_add_joinable_runtime_thread (&p->client_info.info);
 
        if (mono_gc_get_gc_callbacks ()->thread_detach_func) {
                mono_gc_get_gc_callbacks ()->thread_detach_func (p->client_info.runtime_data);

diff --git a/mono/metadata/threads-types.h b/mono/metadata/threads-types.h
index 013046d0ee808d6cd1bfcd4cd0894d628faeed1e..2feb0ba4f8488713a117dd3a8156fb28de455e03 100644 (file)
--- a/mono/metadata/threads-types.h
+++ b/mono/metadata/threads-types.h
@@ -242,6 +242,7 @@ gboolean
 mono_thread_create_checked (MonoDomain *domain, gpointer func, gpointer arg, MonoError *error);
 
 /* Can't include utils/mono-threads.h because of the THREAD_INFO_TYPE wizardry */
+void mono_threads_add_joinable_runtime_thread (gpointer thread_info);
 void mono_threads_add_joinable_thread (gpointer tid);
 void mono_threads_join_threads (void);
 void mono_thread_join (gpointer tid);

diff --git a/mono/metadata/threads.c b/mono/metadata/threads.c
index 7133185e36cc984c7741442b72b5e48b672c43a6..94882b1f776911132a814332406bd9f66c140960 100644 (file)
--- a/mono/metadata/threads.c
+++ b/mono/metadata/threads.c
@@ -62,6 +62,9 @@
 
 #if defined(HOST_WIN32)
 #include <objbase.h>
+
+extern gboolean
+mono_native_thread_join_handle (HANDLE thread_handle, gboolean close_handle);
 #endif
 
 #if defined(HOST_ANDROID) && !defined(TARGET_ARM64) && !defined(TARGET_AMD64)
@@ -758,6 +761,15 @@ mono_thread_detach_internal (MonoInternalThread *thread)
        thread->abort_exc = NULL;
        thread->current_appcontext = NULL;
 
+       /*
+       * Prevent race condition between execution of this method and runtime shutdown.
+       * Adding runtime thread to the joinable threads list will make sure runtime shutdown
+       * won't complete until added runtime thread have exited. Owner of threads attached to the
+       * runtime but not identified as runtime threads needs to make sure thread detach calls won't
+       * race with runtime shutdown.
+       */
+       mono_threads_add_joinable_runtime_thread (thread->thread_info);
+
        /*
         * thread->synch_cs can be NULL if this was called after
         * ves_icall_System_Threading_InternalThread_Thread_free_internal.
@@ -1115,7 +1127,7 @@ create_thread (MonoThread *thread, MonoInternalThread *internal, MonoObject *sta
        else
                stack_set_size = 0;
 
-       if (!mono_thread_platform_create_thread (start_wrapper, start_info, &stack_set_size, &tid)) {
+       if (!mono_thread_platform_create_thread ((MonoThreadStart)start_wrapper, start_info, &stack_set_size, &tid)) {
                /* The thread couldn't be created, so set an exception */
                mono_threads_lock ();
                mono_g_hash_table_remove (threads_starting_up, thread);
@@ -3006,7 +3018,7 @@ thread_detach (MonoThreadInfo *info)
 static void
 thread_detach_with_lock (MonoThreadInfo *info)
 {
-       return mono_gc_thread_detach_with_lock (info);
+       mono_gc_thread_detach_with_lock (info);
 }
 
 static gboolean
@@ -5003,6 +5015,78 @@ mono_thread_is_foreign (MonoThread *thread)
        return info->runtime_thread == FALSE;
 }
 
+#ifndef HOST_WIN32
+static void
+threads_native_thread_join_lock (gpointer tid, gpointer value)
+{
+       pthread_t thread = (pthread_t)tid;
+       if (thread != pthread_self ()) {
+               MONO_ENTER_GC_SAFE;
+               /* This shouldn't block */
+               mono_threads_join_lock ();
+               mono_native_thread_join (thread);
+               mono_threads_join_unlock ();
+               MONO_EXIT_GC_SAFE;
+       }
+}
+static void
+threads_native_thread_join_nolock (gpointer tid, gpointer value)
+{
+       pthread_t thread = (pthread_t)tid;
+       MONO_ENTER_GC_SAFE;
+       mono_native_thread_join (thread);
+       MONO_EXIT_GC_SAFE;
+}
+
+static void
+threads_add_joinable_thread_nolock (gpointer tid)
+{
+       g_hash_table_insert (joinable_threads, tid, tid);
+}
+#else
+static void
+threads_native_thread_join_lock (gpointer tid, gpointer value)
+{
+       MonoNativeThreadId thread_id = (MonoNativeThreadId)(guint64)tid;
+       HANDLE thread_handle = (HANDLE)value;
+       if (thread_id != GetCurrentThreadId () && thread_handle != NULL && thread_handle != INVALID_HANDLE_VALUE) {
+               MONO_ENTER_GC_SAFE;
+               /* This shouldn't block */
+               mono_threads_join_lock ();
+               mono_native_thread_join_handle (thread_handle, TRUE);
+               mono_threads_join_unlock ();
+               MONO_EXIT_GC_SAFE;
+       }
+}
+
+static void
+threads_native_thread_join_nolock (gpointer tid, gpointer value)
+{
+       HANDLE thread_handle = (HANDLE)value;
+       MONO_ENTER_GC_SAFE;
+       mono_native_thread_join_handle (thread_handle, TRUE);
+       MONO_EXIT_GC_SAFE;
+}
+
+static void
+threads_add_joinable_thread_nolock (gpointer tid)
+{
+       g_hash_table_insert (joinable_threads, tid, (gpointer)OpenThread (SYNCHRONIZE, TRUE, (MonoNativeThreadId)(guint64)tid));
+}
+#endif
+
+void
+mono_threads_add_joinable_runtime_thread (gpointer thread_info)
+{
+       g_assert (thread_info);
+       MonoThreadInfo *mono_thread_info = (MonoThreadInfo*)thread_info;
+
+       if (mono_thread_info->runtime_thread) {
+               if (InterlockedCompareExchange (&mono_thread_info->thread_pending_native_join, TRUE, FALSE) == FALSE)
+                       mono_threads_add_joinable_thread ((gpointer)(MONO_UINT_TO_NATIVE_THREAD_ID (mono_thread_info_get_tid (mono_thread_info))));
+       }
+}
+
 /*
  * mono_add_joinable_thread:
  *
@@ -5012,21 +5096,24 @@ mono_thread_is_foreign (MonoThread *thread)
 void
 mono_threads_add_joinable_thread (gpointer tid)
 {
-#ifndef HOST_WIN32
        /*
         * We cannot detach from threads because it causes problems like
         * 2fd16f60/r114307. So we collect them and join them when
-        * we have time (in he finalizer thread).
+        * we have time (in the finalizer thread).
         */
        joinable_threads_lock ();
        if (!joinable_threads)
                joinable_threads = g_hash_table_new (NULL, NULL);
-       g_hash_table_insert (joinable_threads, tid, tid);
-       UnlockedIncrement (&joinable_thread_count);
+
+       gpointer orig_key;
+       gpointer value;
+       if (!g_hash_table_lookup_extended (joinable_threads, tid, &orig_key, &value)) {
+               threads_add_joinable_thread_nolock (tid);
+               UnlockedIncrement (&joinable_thread_count);
+       }
        joinable_threads_unlock ();
 
        mono_gc_finalize_notify ();
-#endif
 }
 
 /*
@@ -5038,11 +5125,9 @@ mono_threads_add_joinable_thread (gpointer tid)
 void
 mono_threads_join_threads (void)
 {
-#ifndef HOST_WIN32
        GHashTableIter iter;
        gpointer key;
-       gpointer tid;
-       pthread_t thread;
+       gpointer value;
        gboolean found;
 
        /* Fastpath */
@@ -5054,27 +5139,17 @@ mono_threads_join_threads (void)
                found = FALSE;
                if (g_hash_table_size (joinable_threads)) {
                        g_hash_table_iter_init (&iter, joinable_threads);
-                       g_hash_table_iter_next (&iter, &key, (void**)&tid);
-                       thread = (pthread_t)tid;
+                       g_hash_table_iter_next (&iter, &key, (void**)&value);
                        g_hash_table_remove (joinable_threads, key);
                        UnlockedDecrement (&joinable_thread_count);
                        found = TRUE;
                }
                joinable_threads_unlock ();
-               if (found) {
-                       if (thread != pthread_self ()) {
-                               MONO_ENTER_GC_SAFE;
-                               /* This shouldn't block */
-                               mono_threads_join_lock ();
-                               mono_native_thread_join (thread);
-                               mono_threads_join_unlock ();
-                               MONO_EXIT_GC_SAFE;
-                       }
-               } else {
+               if (found)
+                       threads_native_thread_join_lock (key, value);
+               else
                        break;
-               }
        }
-#endif
 }
 
 /*
@@ -5086,26 +5161,25 @@ mono_threads_join_threads (void)
 void
 mono_thread_join (gpointer tid)
 {
-#ifndef HOST_WIN32
-       pthread_t thread;
        gboolean found = FALSE;
+       gpointer orig_key;
+       gpointer value;
 
        joinable_threads_lock ();
        if (!joinable_threads)
                joinable_threads = g_hash_table_new (NULL, NULL);
-       if (g_hash_table_lookup (joinable_threads, tid)) {
+
+       if (g_hash_table_lookup_extended (joinable_threads, tid, &orig_key, &value)) {
                g_hash_table_remove (joinable_threads, tid);
                UnlockedDecrement (&joinable_thread_count);
                found = TRUE;
        }
        joinable_threads_unlock ();
+
        if (!found)
                return;
-       thread = (pthread_t)tid;
-       MONO_ENTER_GC_SAFE;
-       mono_native_thread_join (thread);
-       MONO_EXIT_GC_SAFE;
-#endif
+
+       threads_native_thread_join_nolock (tid, value);
 }
 
 void

diff --git a/mono/utils/mono-threads-windows.c b/mono/utils/mono-threads-windows.c
index c20cefd5cdf9b8bc367ba728166bed88bd504de2..563d210e8ed5cfe9ef657e7aba24d77d0f0429cf 100644 (file)
--- a/mono/utils/mono-threads-windows.c
+++ b/mono/utils/mono-threads-windows.c
@@ -218,19 +218,26 @@ mono_native_thread_create (MonoNativeThreadId *tid, gpointer func, gpointer arg)
        return CreateThread (NULL, 0, (func), (arg), 0, (tid)) != NULL;
 }
 
+gboolean
+mono_native_thread_join_handle (HANDLE thread_handle, gboolean close_handle)
+{
+       DWORD res = WaitForSingleObject (thread_handle, INFINITE);
+
+       if (close_handle)
+               CloseHandle (thread_handle);
+
+       return res != WAIT_FAILED;
+}
+
 gboolean
 mono_native_thread_join (MonoNativeThreadId tid)
 {
        HANDLE handle;
 
-       if (!(handle = OpenThread (THREAD_ALL_ACCESS, TRUE, tid)))
+       if (!(handle = OpenThread (SYNCHRONIZE, TRUE, tid)))
                return FALSE;
 
-       DWORD res = WaitForSingleObject (handle, INFINITE);
-
-       CloseHandle (handle);
-
-       return res != WAIT_FAILED;
+       return mono_native_thread_join_handle (handle, TRUE);
 }
 
 #if HAVE_DECL___READFSDWORD==0

diff --git a/mono/utils/mono-threads.h b/mono/utils/mono-threads.h
index be2f8264c71e3da8d38f6f730b5504560249132c..9a67fb863316e44896f975ef4d8f4e9c5e333367 100644 (file)
--- a/mono/utils/mono-threads.h
+++ b/mono/utils/mono-threads.h
@@ -231,6 +231,8 @@ typedef struct {
         */
        gint32 profiler_signal_ack;
 
+       gint32 thread_pending_native_join;
+
 #ifdef USE_WINDOWS_BACKEND
        gint32 thread_wait_info;
 #endif

diff --git a/mono/utils/unlocked.h b/mono/utils/unlocked.h
index 406b686cfeb850c2e064353c12ca696342f335cb..8a1e98bb2ee7068e6246e130b186c16abcca09d3 100644 (file)
--- a/mono/utils/unlocked.h
+++ b/mono/utils/unlocked.h
@@ -20,6 +20,8 @@
 
 #if MONO_HAS_CLANG_THREAD_SANITIZER
 #define MONO_UNLOCKED_ATTRS MONO_NO_SANITIZE_THREAD MONO_NEVER_INLINE static
+#elif defined(_MSC_VER)
+#define MONO_UNLOCKED_ATTRS MONO_ALWAYS_INLINE static
 #else
 #define MONO_UNLOCKED_ATTRS MONO_ALWAYS_INLINE static inline
 #endif