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
