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

[Bruno Haible]

Takashi Yano wrote in cygwin-patches:
>  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 */
> +  if (once_control->state & INT_MIN)
>      return 0;
>  
> +  /* 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);
> -  /* 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 ();
> -   */

Sorry, in a unified diff form this is unreadable. One needs to look at the
entire function. A context diff would have been better. So:

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;

  /* 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 ();
      once_control->state |= INT_MIN;
    }
  pthread_mutex_unlock (&once_control->mutex);
  if (InterlockedDecrement (&once_control->state) == INT_MIN)
    pthread_mutex_destroy (&once_control->mutex);
  return 0;
}

1) The overall logic seems right (using bit 31 of the state word as a
'done' bit), and the last thread that used the mutex destroys it.

2) However, the 'state' field is not volatile, and therefore the memory
model does not guarantee that an assignment

     once_control->state |= INT_MIN;

done in one thread has an effect on the values seen by other threads
that do

     if (once_control->state & INT_MIN)

As long as there is no synchronization between the two CPUs that execute
the two threads, one CPU may indefinitely see the old value of
once_control->state, while the other CPU's new value is not being
"broadcasted" to all CPUs.

3) Also, as noted by Noel Grandin, there is a race: If one thread does

     once_control->state |= INT_MIN;

while another thread does

     InterlockedIncrement (&once_control->state);
or
     InterlockedDecrement (&once_control->state)

the result can be that the increment or decrement is nullified. If it's
an increment that gets nullified, the pthread_mutex_destroy occurs too
early, with fatal consequences. If it's a decrement that get nullified,
the pthread_mutex_destroy never happens, and there is therefore a resource
leak.

4) Does the test program that I posted in
<https://cygwin.com/pipermail/cygwin/2024-May/255987.html> now pass?
Notes:
  - You should set
      #define ENABLE_DEBUGGING 0
    because with the debugging output, it nearly always succeeds.
  - You should run it 10 times in a row, not just once. It may well
    succeed 9 out of 10 times and fail 1 out of 10 times.

Bruno