DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E70963858416
To: cygwin AT cygwin DOT com, Takashi Yano <takashi DOT yano AT nifty DOT ne DOT jp>
Subject: Re: [PATCH v2] Cygwin: pthread: Fix a race issue introduced by the
 commit 2c5433e5da82
Date: Thu, 30 May 2024 12:14:10 +0200
Message-ID: <5613635.1WZ037k8cV@nimes>
In-Reply-To: <20240530050538.53724-1-takashi.yano@nifty.ne.jp>
References: <20240530050538 DOT 53724-1-takashi DOT yano AT nifty DOT ne DOT jp>
MIME-Version: 1.0
Precedence: list
From: Bruno Haible via Cygwin <cygwin AT cygwin DOT com>
Reply-To: Bruno Haible <bruno AT clisp DOT org>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: cygwin-bounces+archive-cygwin=delorie DOT com AT cygwin DOT com
Sender: "Cygwin" <cygwin-bounces+archive-cygwin=delorie DOT com AT cygwin DOT com>

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


-- 
Problem reports:      https://cygwin.com/problems.html
FAQ:                  https://cygwin.com/faq/
Documentation:        https://cygwin.com/docs.html
Unsubscribe info:     https://cygwin.com/ml/#unsubscribe-simple