DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5273D3858C60
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: Fri, 31 May 2024 16:01:35 +0200
Message-ID: <4338587.3DMzsUbDvx@nimes>
In-Reply-To: <20240530205918.7c730117b567bb3bec3a0c3f@nifty.ne.jp>
References: <20240530050538 DOT 53724-1-takashi DOT yano AT nifty DOT ne DOT jp>
 <20240530205012 DOT 2aff4d507acac144e50df2a4 AT nifty DOT ne DOT jp>
 <20240530205918 DOT 7c730117b567bb3bec3a0c3f 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>

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

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.

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