Merge pull request #2260 from xmcclure/fix-testwaitany

Cleanup and crash fix around UnixSignal.WaitAny (bug #36214)

2 files changed, 161 insertions, 45 deletions

diff --git a/support/map.h b/support/map.h
index b121f032be7..9f89fcde93b 100644
--- a/support/map.h
+++ b/support/map.h
@@ -2018,8 +2018,9 @@ struct Mono_Unix_UnixSignal_SignalInfo {
 	int   count;
 	int   read_fd;
 	int   write_fd;
-	int   have_handler;
 	int   pipecnt;
+	int   pipelock;
+	int   have_handler;
 	void* handler;
 };
 
diff --git a/support/signal.c b/support/signal.c
index a018bc67928..993e2e21661 100644
--- a/support/signal.c
+++ b/support/signal.c
@@ -107,23 +107,28 @@ int Mono_Posix_FromRealTimeSignum (int offset, int *r)
 
 #ifndef HOST_WIN32
 
+// Atomicity rules: Fields of signal_info read or written by the signal handler
+// (see UnixSignal.cs) should be read and written using atomic functions.
+// (For simplicity, we're protecting some things we don't strictly need to.)
+
+// Because we are in MonoPosixHelper, we are banned from linking mono.
+// We can still use atomic.h because that's all inline functions--
+// unless WAPI_NO_ATOMIC_ASM is defined, in which case atomic.h calls linked functions.
 #ifndef WAPI_NO_ATOMIC_ASM
 	#define mph_int_get(p)     InterlockedExchangeAdd ((p), 0)
 	#define mph_int_inc(p)     InterlockedIncrement ((p))
 	#define mph_int_dec_test(p)     (InterlockedDecrement ((p)) == 0)
-	#define mph_int_set(p,o,n) InterlockedExchange ((p), (n))
+	#define mph_int_set(p,n) InterlockedExchange ((p), (n))
+	// Pointer, original, new
+	#define mph_int_test_and_set(p,o,n) (o == InterlockedCompareExchange ((p), (n), (o)))
 #elif GLIB_CHECK_VERSION(2,4,0)
 	#define mph_int_get(p) g_atomic_int_get ((p))
  	#define mph_int_inc(p) do {g_atomic_int_inc ((p));} while (0)
 	#define mph_int_dec_test(p) g_atomic_int_dec_and_test ((p))
-	#define mph_int_set(p,o,n) do {                                 \
-		while (!g_atomic_int_compare_and_exchange ((p), (o), (n))) {} \
-	} while (0)
+	#define mph_int_set(p,n) g_atomic_int_set ((p),(n))
+	#define mph_int_test_and_set(p,o,n) g_atomic_int_compare_and_exchange ((p), (o), (n))
 #else
-	#define mph_int_get(p) (*(p))
-	#define mph_int_inc(p) do { (*(p))++; } while (0)
-	#define mph_int_dec_test(p) (--(*(p)) == 0)
-	#define mph_int_set(p,o,n) do { *(p) = n; } while (0)
+	#error "GLIB 2.4 required because building without ASM atomics"
 #endif
 
 #if HAVE_PSIGNAL
@@ -166,6 +171,90 @@ keep_trying (int r)
 	return r == -1 && errno == EINTR;
 }
 
+// This tiny ad-hoc read/write lock is needed because of the very specific
+// synchronization needed between default_handler and teardown_pipes:
+// - Many default_handlers can be running at once
+// - The signals_mutex already ensures only one teardown_pipes runs at once
+// - If teardown_pipes starts while a default_handler is ongoing, it must block
+// - If default_handler starts while a teardown_pipes is ongoing, it must *not* block
+// Locks are implemented as ints.
+
+// The lock is split into a teardown bit and a handler count (sign bit unused).
+// There is a teardown running or waiting to run if the teardown bit is set.
+// There is a handler running if the handler count is nonzero.
+#define PIPELOCK_TEARDOWN_BIT (  (int)0x40000000 )
+#define PIPELOCK_COUNT_MASK   (~((int)0xC0000000))
+#define PIPELOCK_GET_COUNT(x)      ((x) & PIPELOCK_COUNT_MASK)
+#define PIPELOCK_INCR_COUNT(x, by) (((x) & PIPELOCK_TEARDOWN_BIT) | (PIPELOCK_GET_COUNT (PIPELOCK_GET_COUNT (x) + (by))))
+
+static inline void
+acquire_pipelock_teardown (int *lock)
+{
+	int lockvalue_draining;
+	// First mark that a teardown is occurring, so handlers will stop entering the lock.
+	while (1) {
+		int lockvalue = mph_int_get (lock);
+		lockvalue_draining = lockvalue | PIPELOCK_TEARDOWN_BIT;
+		if (mph_int_test_and_set (lock, lockvalue, lockvalue_draining))
+			break;
+	}
+	// Now wait for all handlers to complete.
+	while (1) {
+		if (0 == PIPELOCK_GET_COUNT (lockvalue_draining))
+			break; // We now hold the lock.
+		// Handler is still running, spin until it completes.
+		sched_yield (); // We can call this because !defined(HOST_WIN32)
+		lockvalue_draining = mph_int_get (lock);
+	}
+}
+
+static inline void
+release_pipelock_teardown (int *lock)
+{
+	while (1) {
+		int lockvalue = mph_int_get (lock);
+		int lockvalue_new = lockvalue & ~PIPELOCK_TEARDOWN_BIT;
+		// Technically this can't fail, because we hold both the pipelock and the mutex, but
+		if (mph_int_test_and_set (lock, lockvalue, lockvalue_new))
+			return;
+	}
+}
+
+// Return 1 for success
+static inline int
+acquire_pipelock_handler (int *lock)
+{
+	while (1) {
+		int lockvalue = mph_int_get (lock);
+		if (lockvalue & PIPELOCK_TEARDOWN_BIT) // Final lock is being torn down
+			return 0;
+		int lockvalue_new = PIPELOCK_INCR_COUNT (lockvalue, 1);
+		if (mph_int_test_and_set (lock, lockvalue, lockvalue_new))
+			return 1;
+	}
+}
+
+static inline void
+release_pipelock_handler (int *lock)
+{
+	while (1) {
+		int lockvalue = mph_int_get (lock);
+		int lockvalue_new = PIPELOCK_INCR_COUNT (lockvalue, -1);
+		if (mph_int_test_and_set (lock, lockvalue, lockvalue_new))
+			return;
+	}
+}
+
+// This handler is registered once for each UnixSignal object. A pipe is maintained
+// for each one; Wait users read at one end of this pipe, and default_handler sends
+// a write on the pipe for each signal received while the Wait is ongoing.
+//
+// Notice a fairly unlikely race condition exists here: Because we synchronize with
+// pipe teardown, but not install/uninstall (in other words, we are only trying to
+// protect against writing on a closed pipe) it is technically possible a full
+// uninstall and then an install could complete after signum is checked but before
+// the remaining instructions execute. In this unlikely case count could be
+// incremented or a byte written on the wrong signal handler.
 static void
 default_handler (int signum)
 {
@@ -175,29 +264,36 @@ default_handler (int signum)
 		signal_info* h = &signals [i];
 		if (mph_int_get (&h->signum) != signum)
 			continue;
+
 		mph_int_inc (&h->count);
+
+		if (!acquire_pipelock_handler (&h->pipelock))
+			continue; // Teardown is occurring on this object, no one to send to.
+
 		fd = mph_int_get (&h->write_fd);
-		if (fd > 0) {
+		if (fd > 0) { // If any listener exists to write to
 			int j,pipecounter;
-			char c = signum;
-			pipecounter = mph_int_get (&h->pipecnt);
+			char c = signum; // (Value is meaningless)
+			pipecounter = mph_int_get (&h->pipecnt); // Write one byte per pipe listener
 			for (j = 0; j < pipecounter; ++j) {
 				int r;
 				do { r = write (fd, &c, 1); } while (keep_trying (r));
 			}
 		}
+		release_pipelock_handler (&h->pipelock);
 	}
 }
 
 static pthread_mutex_t signals_mutex = PTHREAD_MUTEX_INITIALIZER;
 
+// A UnixSignal object is being constructed
 void*
 Mono_Unix_UnixSignal_install (int sig)
 {
 #if defined(HAVE_SIGNAL)
 	int i;
-	signal_info* h = NULL; 
-	int have_handler = 0;
+	signal_info* h = NULL;        // signals[] slot to install to
+	int have_handler = 0;         // Candidates for signal_info handler fields
 	void* handler = NULL;
 
 	if (acquire_mutex (&signals_mutex) == -1)
@@ -211,13 +307,16 @@ Mono_Unix_UnixSignal_install (int sig)
 		if (sinfo.sa_handler != SIG_DFL || (void*)sinfo.sa_sigaction != (void*)SIG_DFL) {
 			pthread_mutex_unlock (&signals_mutex);
 			errno = EADDRINUSE;
-			return NULL;
+			return NULL; // This is an rt signal with an existing handler. Bail out.
 		}
 	}
 #endif /*defined (SIGRTMIN) && defined (SIGRTMAX)*/
 
+	// Scan through signals list looking for (1) an unused spot (2) a usable value for handler
 	for (i = 0; i < NUM_SIGNALS; ++i) {
-		if (h == NULL && signals [i].signum == 0) {
+		int just_installed = 0;
+		// We're still looking for a signal_info spot, and this one is available:
+		if (h == NULL && mph_int_get (&signals [i].signum) == 0) {
 			h = &signals [i];
 			h->handler = signal (sig, default_handler);
 			if (h->handler == SIG_ERR) {
@@ -226,27 +325,32 @@ Mono_Unix_UnixSignal_install (int sig)
 				break;
 			}
 			else {
-				h->have_handler = 1;
+				just_installed = 1;
 			}
 		}
-		if (!have_handler && signals [i].signum == sig &&
+		// Check if this slot has a "usable" (not installed by this file) handler-to-restore-later:
+		// (On the first signal to be installed, signals [i] will be == h when this happens.)
+		if (!have_handler && (just_installed || mph_int_get (&signals [i].signum) == sig) &&
 				signals [i].handler != default_handler) {
 			have_handler = 1;
 			handler = signals [i].handler;
 		}
-		if (h && have_handler)
+		if (h && have_handler) // We have everything we need
 			break;
 	}
 
-	if (h && have_handler) {
+	if (h) {
+		// If we reached here without have_handler, this means that default_handler
+		// was set as the signal handler before the first UnixSignal object was installed.
+		g_assert (have_handler);
+
+		// Overwrite the tenative handler we set a moment ago with a known-usable one
+		h->handler = handler;
 		h->have_handler = 1;
-		h->handler      = handler;
-	}
 
-	if (h) {
-		mph_int_set (&h->count, h->count, 0);
-		mph_int_set (&h->signum, h->signum, sig);
-		mph_int_set (&h->pipecnt, h->pipecnt, 0);
+		mph_int_set (&h->count, 0);
+		mph_int_set (&h->pipecnt, 0);
+		mph_int_set (&h->signum, sig);
 	}
 
 	release_mutex (&signals_mutex);
@@ -264,16 +368,17 @@ count_handlers (int signum)
 	int i;
 	int count = 0;
 	for (i = 0; i < NUM_SIGNALS; ++i) {
-		if (signals [i].signum == signum)
+		if (mph_int_get (&signals [i].signum) == signum)
 			++count;
 	}
 	return count;
 }
 
+// A UnixSignal object is being Disposed
 int
 Mono_Unix_UnixSignal_uninstall (void* info)
 {
-#if defined(HAVE_SIGNAL)	
+#if defined(HAVE_SIGNAL)
 	signal_info* h;
 	int r = -1;
 
@@ -286,14 +391,15 @@ Mono_Unix_UnixSignal_uninstall (void* info)
 		errno = EINVAL;
 	else {
 		/* last UnixSignal -- we can unregister */
-		if (h->have_handler && count_handlers (h->signum) == 1) {
-			mph_sighandler_t p = signal (h->signum, h->handler);
+		int signum = mph_int_get (&h->signum);
+		if (h->have_handler && count_handlers (signum) == 1) {
+			mph_sighandler_t p = signal (signum, h->handler);
 			if (p != SIG_ERR)
 				r = 0;
 			h->handler      = NULL;
 			h->have_handler = 0;
 		}
-		h->signum = 0;
+		mph_int_set (&h->signum, 0);
 	}
 
 	release_mutex (&signals_mutex);
@@ -305,6 +411,7 @@ Mono_Unix_UnixSignal_uninstall (void* info)
 #endif
 }
 
+// Set up a signal_info to begin waiting for signal
 static int
 setup_pipes (signal_info** signals, int count, struct pollfd *fd_structs, int *currfd)
 {
@@ -316,21 +423,22 @@ setup_pipes (signal_info** signals, int count, struct pollfd *fd_structs, int *c
 
 		h = signals [i];
 
-		if (mph_int_get (&h->pipecnt) == 0) {
+		if (mph_int_get (&h->pipecnt) == 0) { // First listener for this signal_info
 			if ((r = pipe (filedes)) != 0) {
 				break;
 			}
-			h->read_fd  = filedes [0];
-			h->write_fd = filedes [1];
+			mph_int_set (&h->read_fd,  filedes [0]);
+			mph_int_set (&h->write_fd, filedes [1]);
 		}
 		mph_int_inc (&h->pipecnt);
-		fd_structs[*currfd].fd = h->read_fd;
+		fd_structs[*currfd].fd = mph_int_get (&h->read_fd);
 		fd_structs[*currfd].events = POLLIN;
-		++(*currfd);
+		++(*currfd); // count is verified less than NUM_SIGNALS by caller
 	}
 	return r;
 }
 
+// Cleanup a signal_info after waiting for signal
 static void
 teardown_pipes (signal_info** signals, int count)
 {
@@ -338,21 +446,28 @@ teardown_pipes (signal_info** signals, int count)
 	for (i = 0; i < count; ++i) {
 		signal_info* h = signals [i];
 
-		if (mph_int_dec_test (&h->pipecnt)) {
-			if (h->read_fd != 0)
-				close (h->read_fd);
-			if (h->write_fd != 0)
-				close (h->write_fd);
-			h->read_fd  = 0;
-			h->write_fd = 0;
+		if (mph_int_dec_test (&h->pipecnt)) { // Final listener for this signal_info
+			acquire_pipelock_teardown (&h->pipelock);
+			int read_fd = mph_int_get (&h->read_fd);
+			int write_fd = mph_int_get (&h->write_fd);
+			if (read_fd != 0)
+				close (read_fd);
+			if (write_fd != 0)
+				close (write_fd);
+			mph_int_set (&h->read_fd, 0);
+			mph_int_set (&h->write_fd, 0);
+			release_pipelock_teardown (&h->pipelock);
 		}
 	}
 }
 
+// Given pipes set up, wait for a byte to arrive on one of them
 static int
 wait_for_any (signal_info** signals, int count, int *currfd, struct pollfd* fd_structs, int timeout, Mono_Posix_RuntimeIsShuttingDown shutting_down)
 {
 	int r, idx;
+	// Poll until one of this signal_info's pipes is ready to read.
+	// Once a second, stop to check if the VM is shutting down.
 	do {
 		struct timeval tv;
 		struct timeval *ptv = NULL;
@@ -367,7 +482,7 @@ wait_for_any (signal_info** signals, int count, int *currfd, struct pollfd* fd_s
 	idx = -1;
 	if (r == 0)
 		idx = timeout;
-	else if (r > 0) {
+	else if (r > 0) { // The pipe[s] are ready to read.
 		int i;
 		for (i = 0; i < count; ++i) {
 			signal_info* h = signals [i];
@@ -375,7 +490,7 @@ wait_for_any (signal_info** signals, int count, int *currfd, struct pollfd* fd_s
 				int r;
 				char c;
 				do {
-					r = read (h->read_fd, &c, 1);
+					r = read (mph_int_get (&h->read_fd), &c, 1);
 				} while (keep_trying (r) && !shutting_down ());
 				if (idx == -1)
 					idx = i;