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

[Takashi Yano]

Hi Bruno,

On Fri, 31 May 2024 16:01:35 +0200
Bruno Haible wrote:
> Hi Takashi Yano,
> 
> > With v3 patch:
> > int
> > pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
> > {
> >   /* Sign bit of once_control->state is used as done flag */
> >   if (once_control->state & INT_MIN)
> >     return 0;
> > 
>     // HERE: Point A.
> 
> >   /* The type of &once_control->state is int *, which is compatible with
> >      LONG * (the type of the first argument of InterlockedIncrement()). */
> >   InterlockedIncrement (&once_control->state);
> >   pthread_mutex_lock (&once_control->mutex);
> >   if (!(once_control->state & INT_MIN))
> >     {
> >       init_routine ();
> >       InterlockedOr (&once_control->state, INT_MIN);
> >     }
> >   pthread_mutex_unlock (&once_control->mutex);
> >   if (InterlockedDecrement (&once_control->state) == INT_MIN)
> 
>       // HERE: Point B.
> 
> >     pthread_mutex_destroy (&once_control->mutex);
> 
>       // HERE: Point C.
> 
> >   return 0;
> > }
> 
> I said "looks good to me", but unfortunately I have to withdraw this.
> The code to which I pointed you had two race conditions, unfortunately,
> and this code (v3) has the same two race conditions:
> 
> (1) It can happen that the pthread_mutex_destroy is executed twice, which is
>     undefined behaviour.
> 
>                  thread1                      thread2
>                  -------                      -------
> 
>                  Runs upto A.                 Runs upto A.
> 
>                  Runs upto B:
>                  sets state to 1,
>                  locks,
>                  sets state to INT_MIN + 1,
>                  unlocks,
>                  sets state to INT_MIN.
> 
>                                               Runs upto B:
>                                               sets state to INT_MIN + 1,
>                                               locks,
>                                               unlocks,
>                                               sets state to INT_MIN.
> 
>                  calls pthread_mutex_destroy
> 
>                                               calls pthread_mutex_destroy
> 
> (2) It can happen that pthread_mutex_lock is executed on a mutex that is
>     already destroyed, which is undefined behaviour.
> 
>                  thread1                      thread2
>                  -------                      -------
> 
>                  Runs upto A.                 Runs upto A.
> 
>                  Runs upto C:
>                  sets state to 1,
>                  locks,
>                  sets state to INT_MIN + 1,
>                  unlocks,
>                  sets state to INT_MIN,
>                  calls pthread_mutex_destroy
> 
>                                               Attempts to run upto B:
>                                               sets state to INT_MIN + 1,
>                                               locks  -> BOOM, SIGSEGV

I reconsidered how it can be fixed before reading the following your
idea for double-check. The result is as follows (submitted as v4 patch).

int
pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
{
  /* Sign bit of once_control->state is used as done flag.
     Similary, the next significant bit is used as destroyed flag. */
  const int done = INT_MIN;		/* 0b1000000000000000 */
  const int destroyed = INT_MIN >> 1;	/* 0b1100000000000000 */
  if (once_control->state & done)
    return 0;

  /* The type of &once_control->state is int *, which is compatible with
     LONG * (the type of the pointer argument of InterlockedXxx()). */
  if ((InterlockedIncrement (&once_control->state) & done) == 0)
    {
      pthread_mutex_lock (&once_control->mutex);
      if (!(once_control->state & done))
	{
	  init_routine ();
	  InterlockedOr (&once_control->state, done);
	}
      pthread_mutex_unlock (&once_control->mutex);
    }
  InterlockedDecrement (&once_control->state);
  if (InterlockedCompareExchange (&once_control->state,
				  destroyed, done) == done)
    pthread_mutex_destroy (&once_control->mutex);
  return 0;
}

Then, I read your idea below:

> A corrected implementation (that passes 100 runs of the test program)
> is in
> https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/pthread-once.c;h=4b4a18d2afbb915f8f97ac3ff981f519acaa5996;hb=HEAD#l41
> 
> The fix for race (1) is to extend the "done" part of the state to 2 bits
> instead of just 1 bit, and to use this extra bit to ensure that only one
> of the threads proceeds from B to C.
> 
> The fix for race (2) is to increment num_threads *before* testing the
> 'done' word and, accordingly, to decrement num_threads also when 'done'
> was zero.
> 
> You should be able to transpose the logic accordingly, by using the
> bit mask 0xC0000000 for the 'done' part and the bit mask 0x3FFFFFFF for
> the 'num_threads' part.

I believe both codes are equivalent. Could you please check?

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