From 659f0def364d9d08759fe30173fc4a113badc248 Mon Sep 17 00:00:00 2001 From: Bruno Haible Date: Fri, 31 May 2024 15:25:30 +0200 Subject: [PATCH] pthread-once: Fix race in Cygwin workaround implementation. * lib/pthread-once.c (pthread_once): Test the 'done' word after incrementing num_threads. Make sure to invoke pthread_mutex_destroy only once. --- ChangeLog | 5 +++ lib/pthread-once.c | 98 +++++++++++++++++++++++++++------------------- 2 files changed, 63 insertions(+), 40 deletions(-) diff --git a/ChangeLog b/ChangeLog index e822cb74bd..2ebc8a792b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ 2024-05-31 Bruno Haible + pthread-once: Fix race in Cygwin workaround implementation. + * lib/pthread-once.c (pthread_once): Test the 'done' word after + incrementing num_threads. Make sure to invoke pthread_mutex_destroy + only once. + pthread-once: Simplify Cygwin workaround implementation. * lib/pthread-once.c (pthread_once): Use separate 16-bit words to store the parts of the state. diff --git a/lib/pthread-once.c b/lib/pthread-once.c index ef583daed4..4b4a18d2af 100644 --- a/lib/pthread-once.c +++ b/lib/pthread-once.c @@ -45,30 +45,6 @@ pthread_once (pthread_once_t *once_control, void (*initfunction) (void)) int pthread_once (pthread_once_t *once_control, void (*initfunction) (void)) { -# if 0 - /* This would be the code, for - typedef struct - { - pthread_mutex_t mutex; - _Atomic unsigned int num_threads; - _Atomic unsigned int done; - } - pthread_once_t; - */ - if (once_control->done == 0) - { - once_control->num_threads += 1; - pthread_mutex_lock (&once_control->mutex); - if (once_control->done == 0) - { - (*initfunction) (); - once_control->done = 1; - } - pthread_mutex_unlock (&once_control->mutex); - if ((once_control->num_threads -= 1) == 0) - pthread_mutex_destroy (&once_control->mutex); - } -# else /* In this implementation, we reuse the type typedef struct { pthread_mutex_t mutex; int state; } pthread_once_t; #define PTHREAD_ONCE_INIT { PTHREAD_MUTEX_INITIALIZER, 0 } @@ -80,34 +56,76 @@ pthread_once (pthread_once_t *once_control, void (*initfunction) (void)) struct actual_state { _Atomic unsigned short num_threads; + /* done == 0: initial state + done == 1: initfunction executed, lock still active + done == 2: initfunction executed, lock no longer usable */ _Atomic unsigned short done; }; struct actual_state *state_p = (struct actual_state *) &once_control->state; - /* Test the 'done' bit. */ + /* This test is not necessary. It's only an optimization, to establish + a fast path for the common case that the 'done' word is already > 0. */ if (state_p->done == 0) { - /* The 'done' bit is still zero. Increment num_threads (atomically). */ + /* Increment num_threads (atomically), to indicate that this thread will + possibly take the lock. */ state_p->num_threads += 1; - /* We have incremented num_threads. Now take the lock. */ - pthread_mutex_lock (&once_control->mutex); - /* Test the 'done' bit again. */ + /* Test the 'done' word. */ if (state_p->done == 0) { - /* Execute the initfunction. */ - (*initfunction) (); - /* Set the 'done' bit to 1 (atomically). */ - state_p->done = 1; + /* The 'done' word is still zero. Now take the lock. */ + pthread_mutex_lock (&once_control->mutex); + /* Test the 'done' word again. */ + if (state_p->done == 0) + { + /* Execute the initfunction. */ + (*initfunction) (); + /* Set the 'done' word to 1 (atomically). */ + state_p->done = 1; + } + /* Now the 'done' word is 1. Release the lock. */ + pthread_mutex_unlock (&once_control->mutex); } - /* Now the 'done' bit is 1. Release the lock. */ - pthread_mutex_unlock (&once_control->mutex); + /* Here, done is > 0. */ /* Decrement num_threads (atomically). */ if ((state_p->num_threads -= 1) == 0) - /* num_threads is now zero, and done is 1. - No other thread will need to use the lock. - We can therefore destroy the lock, to free resources. */ - pthread_mutex_destroy (&once_control->mutex); + { + /* num_threads is now zero, and done is > 0. + No other thread will need to use the lock. + We can therefore destroy the lock, to free resources. */ + if (__sync_bool_compare_and_swap (&state_p->done, 1, 2)) + pthread_mutex_destroy (&once_control->mutex); + } } -# endif + /* Proof of correctness: + * num_threads is incremented and then decremented by some threads. + Therefore, num_threads always stays >= 0, and is == 0 at the end. + * The 'done' word, once > 0, stays > 0 (since it is never assigned 0). + * The 'done' word is changed from == 0 to > 0 only while the lock + is taken. Therefore, only the first thread that succeeds in taking + the lock executes the initfunction and sets the 'done' word to a + value > 0; the other threads that take the lock do no side effects + between taking and releasing the lock. + * The 'done' word does not change any more once it is 2. + Therefore, it can be changed from 1 to 2 only once. + * pthread_mutex_destroy gets invoked right after 'done' has been changed + from 1 to 2. Therefore, pthread_mutex_destroy gets invoked only once. + * After a moment where num_threads was 0 and done was > 0, no thread can + reach the pthread_mutex_lock invocation. Proof: + - At such a moment, no thread is in the code range between + state_p->num_threads += 1 + and + state_p->num_threads -= 1 + - After such a moment, some thread can increment num_threads, but from + there they cannot reach the pthread_mutex_lock invocation, because the + if (state_p->done == 0) + test prevents that. + * From this it follows that: + - pthread_mutex_destroy cannot be executed while the lock is taken + (because pthread_mutex_destroy is only executed after a moment where + num_threads was 0 and done was > 0). + - Once pthread_mutex_destroy has been executed, the lock is not used any + more. + */ return 0; } -- 2.39.5