pthread-once: Fix race in Cygwin workaround implementation.
authorBruno Haible <bruno@clisp.org>
Fri, 31 May 2024 13:25:30 +0000 (15:25 +0200)
committerBruno Haible <bruno@clisp.org>
Fri, 31 May 2024 13:25:30 +0000 (15:25 +0200)
* lib/pthread-once.c (pthread_once): Test the 'done' word after
incrementing num_threads. Make sure to invoke pthread_mutex_destroy
only once.

ChangeLog
lib/pthread-once.c

index e822cb74bd94f584f1b817262f14fae9da87f7f1..2ebc8a792b5ba5de55af05e5d334e63860a7326b 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2024-05-31  Bruno Haible  <bruno@clisp.org>
 
+       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.
index ef583daed4996c6d9a38c5a6f5e7874b5940bda9..4b4a18d2afbb915f8f97ac3ff981f519acaa5996 100644 (file)
@@ -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;
 }