From: Ludovic Henry <ludovic@xamarin.com>
Date: Thu, 19 Jan 2017 14:30:07 +0000 (-0500)
Subject: [threadpool] Fix race on runtime shutdown (#4263)
X-Git-Url: http://wien.tomnetworks.com/gitweb/?p=mono.git;a=commitdiff_plain;h=3c3e566002bfd185cc178b1bd139f1f606faee16

[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 21738485349..75de23a393c 100644
--- 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 b3a1dcfd3fa..ed6f9ab4d48 100644
--- 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