DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5273D3858C60
To: cygwin@cygwin.com, Takashi Yano <takashi.yano@nifty.ne.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.53724-1-takashi.yano@nifty.ne.jp>
 <20240530205012.2aff4d507acac144e50df2a4@nifty.ne.jp>
 <20240530205918.7c730117b567bb3bec3a0c3f@nifty.ne.jp>
MIME-Version: 1.0
Precedence: list
From: Bruno Haible via Cygwin <cygwin@cygwin.com>
Reply-To: Bruno Haible <bruno@clisp.org>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: cygwin-bounces+archive-cygwin=delorie.com@cygwin.com
Sender: "Cygwin" <cygwin-bounces+archive-cygwin=delorie.com@cygwin.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