[PATCH] Cygwin: pthread: Fix a race issue introduced by the commit 2c5433e5da82

[PATCH] Cygwin: pthread: Fix a race issue introduced by the commit 2c5433e5da82

[Takashi Yano]

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. With this patch, do not use pthread_mutex in
pthread::once() to avoid both issues. Instead, InterlockedExchage()
is used.

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:
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 winsup/cygwin/thread.cc | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
index 0f8327831..1e5f9362b 100644
--- a/winsup/cygwin/thread.cc
+++ b/winsup/cygwin/thread.cc
@@ -2045,27 +2045,10 @@ 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)
-    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)
-    {
-      init_routine ();
-      once_control->state = 1;
-      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);
+  /* The type of &once_control->state is int *, which is compatible with
+     LONG * (the type of the first argument of InterlockedExchange()). */
+  if (!InterlockedExchange (&once_control->state, 1))
+    init_routine ();
   return 0;
 }
 
-- 
2.45.1

Re: [PATCH] Cygwin: pthread: Fix a race issue introduced by the commit 2c5433e5da82

[Bruno Haible]

Takashi Yano wrote:
> 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. With this patch, do not use pthread_mutex in
> pthread::once() to avoid both issues. Instead, InterlockedExchage()
> is used.

This patch is bogus as well, because it allows one thread to return
from a pthread_once call while the other thread is currently
executing the init_routine and not yet done with it.

> +  if (!InterlockedExchange (&once_control->state, 1))
> +    init_routine ();
>    return 0;
>  }

There is no code after the init_routine () call here. This means
that other threads are not notified when the init_routine () call
is complete. Therefore this implementation *cannot* be correct.

See: Assume thread1 and thread2 call pthread_once on the same
once_control.

            thread1                      thread2
            -------                      -------

         enters pthread_once       enters pthread_once

         sets state to 1

                                   sees that state == 1

                                   returns from pthread_once

                                   executes code that assumes
                                   init_routine has completed

         starts executing
         init_routine

         finished executing
         init_routine

         returns from pthread_once

Bruno

Re: [PATCH] Cygwin: pthread: Fix a race issue introduced by the commit 2c5433e5da82

[Takashi Yano]

On Wed, 29 May 2024 13:17:47 +0200
Bruno Haible wrote:
> Takashi Yano wrote:
> > 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. With this patch, do not use pthread_mutex in
> > pthread::once() to avoid both issues. Instead, InterlockedExchage()
> > is used.
> 
> This patch is bogus as well, because it allows one thread to return
> from a pthread_once call while the other thread is currently
> executing the init_routine and not yet done with it.
> 
> > +  if (!InterlockedExchange (&once_control->state, 1))
> > +    init_routine ();
> >    return 0;
> >  }
> 
> There is no code after the init_routine () call here. This means
> that other threads are not notified when the init_routine () call
> is complete. Therefore this implementation *cannot* be correct.
> 
> See: Assume thread1 and thread2 call pthread_once on the same
> once_control.
> 
>             thread1                      thread2
>             -------                      -------
> 
>          enters pthread_once       enters pthread_once
> 
>          sets state to 1
> 
>                                    sees that state == 1
> 
>                                    returns from pthread_once
> 
>                                    executes code that assumes
>                                    init_routine has completed
> 
>          starts executing
>          init_routine
> 
>          finished executing
>          init_routine
> 
>          returns from pthread_once

Thanks for pointing out that.

I'll submit a v2 patch. Please have a look.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>