(series) Fix serious missing synchronization bug between internal locks and threads_minus_1 1->0 transition

Submitted by Rich Felker on May 22, 2020, 9:56 p.m.

Details

Message ID 20200522215649.GC1079@brightrain.aerifal.cx
State New
Series "(series) Fix serious missing synchronization bug between internal locks and threads_minus_1 1->0 transition"
Headers show

Commit Message

Rich Felker May 22, 2020, 9:56 p.m.
On Fri, May 22, 2020 at 12:21:42PM -0400, Rich Felker wrote:
> Anyway, first fix coming soon. This will be important for distros to
> pick up.

And here's a patch series.

I found and fixed a second bug, independent of the first, where
threads_minus_1 was being decremented too early and causing dlerror
cleanup (__dl_thread_cleanup) to skip locking.

I also have a solution for returning to skipping locks after the
process becomes single-threaded again.

See attached series.
From 4d5aa20a94a2d3fae3e69289dc23ecafbd0c16c4 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Fri, 22 May 2020 17:35:14 -0400
Subject: [PATCH 1/4] reorder thread list unlink in pthread_exit after all
 locks

since the backend for LOCK() skips locking if single-threaded, it's
unsafe to make the process appear single-threaded before the last use
of lock.

this fixes potential unsynchronized access to a linked list via
__dl_thread_cleanup.
---
 src/thread/pthread_create.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c
index 5f491092..6a3b0c21 100644
--- a/src/thread/pthread_create.c
+++ b/src/thread/pthread_create.c
@@ -90,14 +90,7 @@  _Noreturn void __pthread_exit(void *result)
 		exit(0);
 	}
 
-	/* At this point we are committed to thread termination. Unlink
-	 * the thread from the list. This change will not be visible
-	 * until the lock is released, which only happens after SYS_exit
-	 * has been called, via the exit futex address pointing at the lock. */
-	libc.threads_minus_1--;
-	self->next->prev = self->prev;
-	self->prev->next = self->next;
-	self->prev = self->next = self;
+	/* At this point we are committed to thread termination. */
 
 	/* Process robust list in userspace to handle non-pshared mutexes
 	 * and the detached thread case where the robust list head will
@@ -121,6 +114,16 @@  _Noreturn void __pthread_exit(void *result)
 	__do_orphaned_stdio_locks();
 	__dl_thread_cleanup();
 
+	/* Last, unlink thread from the list. This change will not be visible
+	 * until the lock is released, which only happens after SYS_exit
+	 * has been called, via the exit futex address pointing at the lock.
+	 * This needs to happen after any possible calls to LOCK() that might
+	 * skip locking if libc.threads_minus_1 is zero. */
+	libc.threads_minus_1--;
+	self->next->prev = self->prev;
+	self->prev->next = self->next;
+	self->prev = self->next = self;
+
 	/* This atomic potentially competes with a concurrent pthread_detach
 	 * call; the loser is responsible for freeing thread resources. */
 	int state = a_cas(&self->detach_state, DT_JOINABLE, DT_EXITING);