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.y...@nifty.ne.jp>
--
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
