[threadpool] Fix race on runtime shutdown (#4263)

We cannot free the threadpool, because there is a race on shutdown where a managed thread may request a new threadpool thread, but we already destroyed the threadpool. So to avoid a use-after-free, we simply do not free the threadpool, as we won't be able to access the threadpool anyway because the ref count will be 0

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

diff --git a/mono/metadata/threadpool.c b/mono/metadata/threadpool.c
index 217384853490ca102fbde6ddfbb81730f6c5b18b..75de23a393c84f22bcd12730204933c9b73458d6 100644 (file)
--- a/mono/metadata/threadpool.c
+++ b/mono/metadata/threadpool.c
@@ -132,7 +132,13 @@ destroy (gpointer unused)
        mono_coop_mutex_destroy (&threadpool->threads_lock);
        mono_coop_cond_destroy (&threadpool->threads_exit_cond);
 
-       g_free (threadpool);
+       /* We cannot free the threadpool, because there is a race
+        * on shutdown where a managed thread may request a new
+        * threadpool thread, but we already destroyed the
+        * threadpool. So to avoid a use-after-free, we simply do
+        * not free the threadpool, as we won't be able to access
+        * the threadpool anyway because the ref count will be 0 */
+       // g_free (threadpool);
 }
 
 static void
@@ -797,11 +803,17 @@ ves_icall_System_Threading_ThreadPool_RequestWorkerThread (void)
        if (mono_domain_is_unloading (domain))
                return FALSE;
 
+       if (!mono_refcount_tryinc (threadpool)) {
+               /* threadpool has been destroyed, we are shutting down */
+               return FALSE;
+       }
+
        domains_lock ();
 
        /* synchronize with mono_threadpool_remove_domain_jobs */
        if (mono_domain_is_unloading (domain)) {
                domains_unlock ();
+               mono_refcount_dec (threadpool);
                return FALSE;
        }
 
@@ -814,14 +826,14 @@ ves_icall_System_Threading_ThreadPool_RequestWorkerThread (void)
        domains_unlock ();
 
        COUNTER_ATOMIC (threadpool, counter, {
-               if (counter._.starting == 16)
+               if (counter._.starting == 16) {
+                       mono_refcount_dec (threadpool);
                        return TRUE;
+               }
 
                counter._.starting ++;
        });
 
-       mono_refcount_inc (threadpool);
-
        mono_threadpool_worker_enqueue (threadpool->worker, worker_callback, NULL);
 
        return TRUE;

diff --git a/mono/utils/refcount.h b/mono/utils/refcount.h
index b3a1dcfd3fa08cfc20c9a4abb25188fc9db47e6a..ed6f9ab4d488ad9f5103f89e9151425fdd3cdec1 100644 (file)
--- a/mono/utils/refcount.h
+++ b/mono/utils/refcount.h
@@ -23,6 +23,7 @@ typedef struct {
 
 #define mono_refcount_init(v,destructor) do { mono_refcount_initialize (&(v)->ref, (destructor)); } while (0)
 #define mono_refcount_inc(v) (mono_refcount_increment (&(v)->ref),(v))
+#define mono_refcount_tryinc(v) (mono_refcount_tryincrement (&(v)->ref))
 #define mono_refcount_dec(v) do { mono_refcount_decrement (&(v)->ref); } while (0)
 
 static inline void
@@ -32,8 +33,8 @@ mono_refcount_initialize (MonoRefCount *refcount, void (*destructor) (gpointer d
        refcount->destructor = destructor;
 }
 
-static inline void
-mono_refcount_increment (MonoRefCount *refcount)
+static inline gboolean
+mono_refcount_tryincrement (MonoRefCount *refcount)
 {
        guint32 oldref, newref;
 
@@ -42,10 +43,19 @@ mono_refcount_increment (MonoRefCount *refcount)
        do {
                oldref = refcount->ref;
                if (oldref == 0)
-                       g_error ("%s: cannot increment a ref with value 0", __func__);
+                       return FALSE;
 
                newref = oldref + 1;
        } while (InterlockedCompareExchange ((gint32*) &refcount->ref, newref, oldref) != oldref);
+
+       return TRUE;
+}
+
+static inline void
+mono_refcount_increment (MonoRefCount *refcount)
+{
+       if (!mono_refcount_tryincrement (refcount))
+               g_error ("%s: cannot increment a ref with value 0", __func__);
 }
 
 static inline void