From ed1884bb9b43ddd69daba52302494ad2d02bbbd9 Mon Sep 17 00:00:00 2001 From: Johan Lorensson Date: Tue, 26 Sep 2017 19:25:23 +0200 Subject: [PATCH] Fix shutdown race between mono_thread_manage and mono_thread_detach_internal. (#5599) MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit * 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 | 4 +- mono/metadata/sgen-mono.c | 3 +- mono/metadata/threads-types.h | 1 + mono/metadata/threads.c | 138 +++++++++++++++++++++++------- mono/utils/mono-threads-windows.c | 19 ++-- mono/utils/mono-threads.h | 2 + mono/utils/unlocked.h | 2 + 7 files changed, 127 insertions(+), 42 deletions(-) diff --git a/mono/metadata/gc.c b/mono/metadata/gc.c index 702857858eb..4cda6ddb320 100644 --- 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 ac977c0e590..33d1e52dd8d 100644 --- 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 013046d0ee8..2feb0ba4f84 100644 --- 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 7133185e36c..94882b1f776 100644 --- a/mono/metadata/threads.c +++ b/mono/metadata/threads.c @@ -62,6 +62,9 @@ #if defined(HOST_WIN32) #include + +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 c20cefd5cdf..563d210e8ed 100644 --- 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 be2f8264c71..9a67fb86331 100644 --- 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 406b686cfeb..8a1e98bb2ee 100644 --- 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 -- 2.25.1