[mono-threads] Fix lifetime of MonoThreadInfo->handle (#3539)

This handle should have a total of maximum 3 copies:
 - in MonoThreadInfo->handle
 - in the return value of mono_threads_create_thread
 - in MonoInternalThread->handle (if it's a managed thread)

Before, there would always be 2 references (in MonoThreadInfo->handle and another another for MonoInternalThread->handle), even if it would be referenced at one place only (MonoThreadInfo->handle), which would lead to a handle leak.

We also need to duplicate the returned handle, to avoid having a use-after-free of the handle (in case the thread exits before we access that handle)

diff --git a/mono/metadata/appdomain.c b/mono/metadata/appdomain.c
index 79c8406d46f11f8706ebe024dfd451fcc5918e6f..bd6487f3b5b3c3cdbbc8e5dda1debf0ac40d2b59 100644 (file)
--- a/mono/metadata/appdomain.c
+++ b/mono/metadata/appdomain.c
@@ -2599,11 +2599,14 @@ mono_domain_try_unload (MonoDomain *domain, MonoObject **exc)
                if (mono_thread_internal_has_appdomain_ref (mono_thread_internal_current (), domain) && (mono_thread_interruption_requested ())) {
                        /* The unload thread tries to abort us */
                        /* The icall wrapper will execute the abort */
+                       mono_threads_close_thread_handle (thread_handle);
                        unload_data_unref (thread_data);
                        return;
                }
        }
 
+       mono_threads_close_thread_handle (thread_handle);
+
        if (thread_data->failure_reason) {
                /* Roll back the state change */
                domain->state = MONO_APPDOMAIN_CREATED;

diff --git a/mono/metadata/threads.c b/mono/metadata/threads.c
index 4c3ca385941daa308a8c4c1a04d5b34284ac81af..d2c38f69d665fd2ed66ed8a3b07a5a94ed3c47aa 100644 (file)
--- a/mono/metadata/threads.c
+++ b/mono/metadata/threads.c
@@ -595,7 +595,7 @@ mono_thread_attach_internal (MonoThread *thread, gboolean force_attach, gboolean
        info = mono_thread_info_current ();
 
        internal = thread->internal_thread;
-       internal->handle = mono_thread_info_get_handle (info);
+       internal->handle = mono_thread_info_duplicate_handle (info);
        internal->tid = MONO_NATIVE_THREAD_ID_TO_UINT (mono_native_thread_id_get ());
        internal->thread_info = info;
        internal->small_id = info->small_id;
@@ -910,6 +910,8 @@ create_thread (MonoThread *thread, MonoInternalThread *internal, MonoObject *sta
 
        mono_coop_sem_wait (&start_info->registered, MONO_SEM_FLAGS_NONE);
 
+       mono_threads_close_thread_handle (thread_handle);
+
        THREAD_DEBUG (g_message ("%s: (%"G_GSIZE_FORMAT") Done launching thread %p (%"G_GSIZE_FORMAT")", __func__, mono_native_thread_id_get (), internal, (gsize)internal->tid));
 
        ret = !start_info->failed;

diff --git a/mono/mini/aot-compiler.c b/mono/mini/aot-compiler.c
index 34614773907bd8c6818564cc7b2d2aead45f5cf3..5379f3bcf069d932f91ce5f850fad728f5f23121 100644 (file)
--- a/mono/mini/aot-compiler.c
+++ b/mono/mini/aot-compiler.c
@@ -9816,6 +9816,7 @@ compile_methods (MonoAotCompile *acfg)
 
                for (i = 0; i < threads->len; ++i) {
                        WaitForSingleObjectEx (g_ptr_array_index (threads, i), INFINITE, FALSE);
+                       mono_threads_close_thread_handle (g_ptr_array_index (threads, i));
                }
        } else {
                methods_len = 0;

diff --git a/mono/utils/mono-threads-posix.c b/mono/utils/mono-threads-posix.c
index 67b470467b96339e10e560a26f99a1f2ee644b07..ab40c33e1c775e244b816d4c973c512147f33922 100644 (file)
--- a/mono/utils/mono-threads-posix.c
+++ b/mono/utils/mono-threads-posix.c
@@ -52,11 +52,6 @@ mono_threads_platform_register (MonoThreadInfo *info)
        if (thread_handle == INVALID_HANDLE_VALUE)
                g_error ("%s: failed to create handle", __func__);
 
-       /* We need to keep the handle alive, as long as the corresponding managed
-        * thread object is alive. The handle is going to be unref when calling
-        * the finalizer on the MonoThreadInternal object */
-       mono_w32handle_ref (thread_handle);
-
        g_assert (!info->handle);
        info->handle = thread_handle;
 }
@@ -141,6 +136,14 @@ mono_threads_get_max_stack_size (void)
        return (int)lim.rlim_max;
 }
 
+gpointer
+mono_threads_platform_duplicate_handle (MonoThreadInfo *info)
+{
+       g_assert (info->handle);
+       mono_w32handle_ref (info->handle);
+       return info->handle;
+}
+
 HANDLE
 mono_threads_platform_open_thread_handle (HANDLE handle, MonoNativeThreadId tid)
 {

diff --git a/mono/utils/mono-threads-windows.c b/mono/utils/mono-threads-windows.c
index 4f8495054334310bb2f7289e8778e657915e59f2..3e7245062e1d21eb19902cc80051626d82955cb1 100644 (file)
--- a/mono/utils/mono-threads-windows.c
+++ b/mono/utils/mono-threads-windows.c
@@ -253,6 +253,17 @@ mono_threads_get_max_stack_size (void)
        return INT_MAX;
 }
 
+gpointer
+mono_threads_platform_duplicate_handle (MonoThreadInfo *info)
+{
+       HANDLE thread_handle;
+
+       g_assert (info->handle);
+       DuplicateHandle (GetCurrentProcess (), info->handle, GetCurrentProcess (), &thread_handle, THREAD_ALL_ACCESS, TRUE, 0);
+
+       return thread_handle;
+}
+
 HANDLE
 mono_threads_platform_open_thread_handle (HANDLE handle, MonoNativeThreadId tid)
 {

diff --git a/mono/utils/mono-threads.c b/mono/utils/mono-threads.c
index 22eea19051cb75bd82a0003e0d0214a9dd406d06..3ec45d07c89be9165e9487df3ca5f3b97f7c6648 100644 (file)
--- a/mono/utils/mono-threads.c
+++ b/mono/utils/mono-threads.c
@@ -1138,7 +1138,7 @@ typedef struct {
        gpointer start_routine_arg;
        gint32 priority;
        MonoCoopSem registered;
-       MonoThreadInfo *info;
+       gpointer handle;
 } CreateThreadData;
 
 static gsize WINAPI
@@ -1165,7 +1165,7 @@ inner_start_thread (gpointer data)
 
        mono_threads_platform_set_priority (info, priority);
 
-       thread_data->info = info;
+       thread_data->handle = mono_thread_info_duplicate_handle (info);
 
        mono_coop_sem_post (&thread_data->registered);
 
@@ -1195,7 +1195,6 @@ HANDLE
 mono_threads_create_thread (MonoThreadStart start, gpointer arg, MonoThreadParm *tp, MonoNativeThreadId *out_tid)
 {
        CreateThreadData *thread_data;
-       MonoThreadInfo *info;
        gint res;
        gpointer ret;
 
@@ -1217,10 +1216,7 @@ mono_threads_create_thread (MonoThreadStart start, gpointer arg, MonoThreadParm
        res = mono_coop_sem_wait (&thread_data->registered, MONO_SEM_FLAGS_NONE);
        g_assert (res == 0);
 
-       info = thread_data->info;
-       g_assert (info);
-
-       ret = info->handle;
+       ret = thread_data->handle;
        g_assert (ret);
 
 done:
@@ -1666,10 +1662,10 @@ mono_thread_info_set_exited (THREAD_INFO_TYPE *info)
 }
 
 gpointer
-mono_thread_info_get_handle (THREAD_INFO_TYPE *info)
+mono_thread_info_duplicate_handle (MonoThreadInfo *info)
 {
-       g_assert (info->handle);
-       return info->handle;
+       g_assert (mono_thread_info_is_current (info));
+       return mono_threads_platform_duplicate_handle (info);
 }
 
 void

diff --git a/mono/utils/mono-threads.h b/mono/utils/mono-threads.h
index 5aafac7d7c28cad1ff71a1500dc066f03e945766..30308902e3ede834be57d9aa74b199246cf907b8 100644 (file)
--- a/mono/utils/mono-threads.h
+++ b/mono/utils/mono-threads.h
@@ -551,6 +551,7 @@ void mono_threads_platform_own_mutex (THREAD_INFO_TYPE *info, gpointer mutex_han
 void mono_threads_platform_disown_mutex (THREAD_INFO_TYPE *info, gpointer mutex_handle);
 MonoThreadPriority mono_threads_platform_get_priority (THREAD_INFO_TYPE *info);
 void mono_threads_platform_set_priority (THREAD_INFO_TYPE *info, MonoThreadPriority priority);
+gpointer mono_threads_platform_duplicate_handle (THREAD_INFO_TYPE *info);
 
 void mono_threads_coop_begin_global_suspend (void);
 void mono_threads_coop_end_global_suspend (void);
@@ -666,7 +667,7 @@ gboolean
 mono_thread_info_is_current (THREAD_INFO_TYPE *info);
 
 gpointer
-mono_thread_info_get_handle (THREAD_INFO_TYPE *info);
+mono_thread_info_duplicate_handle (THREAD_INFO_TYPE *info);
 
 void
 mono_thread_info_describe (THREAD_INFO_TYPE *info, GString *text);