[newlib-cygwin] Cygwin: pthread: Fix a race issue introduced by the commit 2c5433e5da82

[Takashi Yano]

https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=5a859642f0dc4fb10926fd0cdd2025248a9e18ea

commit 5a859642f0dc4fb10926fd0cdd2025248a9e18ea
Author: Takashi Yano <takashi.yano@nifty.ne.jp>
Date:   Wed May 29 19:12:09 2024 +0900

    Cygwin: pthread: Fix a race issue introduced by the commit 2c5433e5da82
    
    To avoid race issues, pthread::once() uses pthread_mutex. This caused
    the handle leak which was fixed by the commit 2c5433e5da82. However,
    this fix introduced another race issue, i.e., the mutex may be used
    after it is destroyed. This patch fixes the issue. Special thanks to
    Bruno Haible for discussing how to fix this.
    
    Addresses: https://cygwin.com/pipermail/cygwin/2024-May/255987.html
    Reported-by: Bruno Haible <bruno@clisp.org>
    Fixes: 2c5433e5da82 ("Cygwin: pthread: Fix handle leak in pthread_once.")
    Reviewed-by: Ken Brown <kbrown@cornell.edu>
    Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>

Diff:
---
 winsup/cygwin/local_includes/thread.h |  2 +-
 winsup/cygwin/release/3.5.4           |  4 ++++
 winsup/cygwin/thread.cc               | 36 +++++++++++++++++++----------------
 3 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/winsup/cygwin/local_includes/thread.h b/winsup/cygwin/local_includes/thread.h
index 9939c4224..b3496281e 100644
--- a/winsup/cygwin/local_includes/thread.h
+++ b/winsup/cygwin/local_includes/thread.h
@@ -674,7 +674,7 @@ class pthread_once
 {
 public:
   pthread_mutex_t mutex;
-  int state;
+  volatile int state;
 };
 
 /* shouldn't be here */
diff --git a/winsup/cygwin/release/3.5.4 b/winsup/cygwin/release/3.5.4
index e1909865f..257e012fc 100644
--- a/winsup/cygwin/release/3.5.4
+++ b/winsup/cygwin/release/3.5.4
@@ -8,3 +8,7 @@ Fixes:
 - Fix regression introduced in 3.5.0 when reading surrogate pairs (i.e.,
   unicode chars >= 0x10000) from the DOS command line.  Addresses:
   https://cygwin.com/pipermail/cygwin/2024-April/255807.html
+
+- Fix regression of pthread::once() introduced in 3.5.0 (i.e., the race
+  issue regarding destroying mutex).
+  Addresses: https://cygwin.com/pipermail/cygwin/2024-May/255987.html
diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
index 0f8327831..0c6f57032 100644
--- a/winsup/cygwin/thread.cc
+++ b/winsup/cygwin/thread.cc
@@ -2045,27 +2045,31 @@ pthread::create (pthread_t *thread, const pthread_attr_t *attr,
 int
 pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
 {
-  // already done ?
-  if (once_control->state)
+  /* Sign bit of once_control->state is used as done flag.
+     Similarly, the next significant bit is used as destroyed flag. */
+  const int32_t DONE = 0x80000000;
+  const int32_t  DESTROYED = 0x40000000;
+  static_assert (sizeof (once_control->state) >= sizeof (int32_t));
+  static_assert (sizeof (once_control->state) == sizeof (LONG));
+  if (once_control->state & DONE)
     return 0;
 
-  pthread_mutex_lock (&once_control->mutex);
-  /* Here we must set a cancellation handler to unlock the mutex if needed */
-  /* but a cancellation handler is not the right thing. We need this in the thread
-   *cleanup routine. Assumption: a thread can only be in one pthread_once routine
-   *at a time. Stote a mutex_t *in the pthread_structure. if that's non null unlock
-   *on pthread_exit ();
-   */
-  if (!once_control->state)
+  /* The type of &once_control->state is int *, which is compatible with
+     LONG * (that is the type of the pointer argument of InterlockedXxx()). */
+  if ((InterlockedIncrement (&once_control->state) & DONE) == 0)
     {
-      init_routine ();
-      once_control->state = 1;
+      pthread_mutex_lock (&once_control->mutex);
+      if (!(once_control->state & DONE))
+	{
+	  init_routine ();
+	  InterlockedOr (&once_control->state, DONE);
+	}
       pthread_mutex_unlock (&once_control->mutex);
-      while (pthread_mutex_destroy (&once_control->mutex) == EBUSY);
-      return 0;
     }
-  /* Here we must remove our cancellation handler */
-  pthread_mutex_unlock (&once_control->mutex);
+  InterlockedDecrement (&once_control->state);
+  if (InterlockedCompareExchange (&once_control->state,
+				  DONE | DESTROYED, DONE) == DONE)
+    pthread_mutex_destroy (&once_control->mutex);
   return 0;
 }