| delorie.com/archives/browse.cgi | search |
| DKIM-Filter: | OpenDKIM Filter v2.11.0 delorie.com 451EJV0b2947766 |
| Authentication-Results: | delorie.com; |
| dkim=pass (1024-bit key, unprotected) header.d=cygwin.com header.i=@cygwin.com header.a=rsa-sha256 header.s=default header.b=KfVMZncU | |
| X-Recipient: | archive-cygwin AT delorie DOT com |
| DKIM-Filter: | OpenDKIM Filter v2.11.0 sourceware.org 816163850204 |
| DKIM-Signature: | v=1; a=rsa-sha256; c=relaxed/relaxed; d=cygwin.com; |
| s=default; t=1717251569; | |
| bh=pPuIclRlJErNnjoU4D3dzGhGXDz75ShsNO44qFvgs2g=; | |
| h=Date:To:Cc:Subject:In-Reply-To:References:List-Id: | |
| List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: | |
| From:Reply-To:From; | |
| b=KfVMZncUUTSRhjhmkHXvhoibiiTRYYUuTae7Pm0WNinsDV8WWPB36T10A1HJ6ec3f | |
| tpGS3CWdUvdPlSBM6Ai2HTh0/dhXmmWXr4nt8WCLej04Wu9somv5p+K40lv0Rzd9GE | |
| 2jE2ocwc/9nrPwYCnmtK1ykjB0YcBrfjnIlXGuxY= | |
| X-Original-To: | cygwin AT cygwin DOT com |
| Delivered-To: | cygwin AT cygwin DOT com |
| DMARC-Filter: | OpenDMARC Filter v1.4.2 sourceware.org 0A1163858C35 |
| ARC-Filter: | OpenARC Filter v1.0.0 sourceware.org 0A1163858C35 |
| ARC-Seal: | i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1717251515; cv=none; |
| b=SqPA9565sjvOyyTH1yRjqxJSxnhWTuZCNd/U5lHMnvSTk+PCuiulDnWxIdglQBGJ2+NSLuPJkOQrY4qDsFymSYh9ekN8A+8k5SQoXV0CN4ofj0K6m7vXP+q7aacpL7rWXbHUL4hs1EyBUHkoPlVMAdb99L0EMSgY0hc0h7k39Oo= | |
| ARC-Message-Signature: | i=1; a=rsa-sha256; d=sourceware.org; s=key; |
| t=1717251515; c=relaxed/simple; | |
| bh=4GThRBqSeiHOZsoKUJbZ08jbfn0wPbt7++1ax7gRJRI=; | |
| h=Date:From:To:Subject:Message-Id:Mime-Version:DKIM-Signature; | |
| b=UhDfnA8Il12ziIHXKI/KSAVvRIItXopX8/iD5Ex9T7p2jhlmOuxOWwHtEQKU84cYOtE+95q/MLL4/5rzZmYaloIW3f4le7h2gndOWDcvxbnqMC4qLeGN4qdZS+u2jHlvQPSBqr3I/bQ8xotfFyuOdst8gS81cj6GQpLDkiMrwaw= | |
| ARC-Authentication-Results: | i=1; server2.sourceware.org |
| Date: | Sat, 1 Jun 2024 23:18:30 +0900 |
| To: | Bruno Haible <bruno AT clisp DOT org> |
| Cc: | cygwin AT cygwin DOT com |
| Subject: | Re: [PATCH v2] Cygwin: pthread: Fix a race issue introduced by the |
| commit 2c5433e5da82 | |
| Message-Id: | <20240601231830.882dc56aadb9c3087bcf4b9c@nifty.ne.jp> |
| In-Reply-To: | <4338587.3DMzsUbDvx@nimes> |
| 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> | |
| <4338587 DOT 3DMzsUbDvx AT nimes> | |
| X-Mailer: | Sylpheed 3.7.0 (GTK+ 2.24.30; i686-pc-mingw32) |
| Mime-Version: | 1.0 |
| X-Spam-Status: | No, score=-4.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, |
| DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_NUMSUBJECT, NICE_REPLY_A, | |
| RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_PASS, TXREP, | |
| T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 | |
| X-Spam-Checker-Version: | SpamAssassin 3.4.6 (2021-04-09) on |
| server2.sourceware.org | |
| X-BeenThere: | cygwin AT cygwin DOT com |
| X-Mailman-Version: | 2.1.30 |
| List-Id: | General Cygwin discussions and problem reports <cygwin.cygwin.com> |
| List-Unsubscribe: | <https://cygwin.com/mailman/options/cygwin>, |
| <mailto:cygwin-request AT cygwin DOT com?subject=unsubscribe> | |
| List-Archive: | <https://cygwin.com/pipermail/cygwin/> |
| List-Post: | <mailto:cygwin AT cygwin DOT com> |
| List-Help: | <mailto:cygwin-request AT cygwin DOT com?subject=help> |
| List-Subscribe: | <https://cygwin.com/mailman/listinfo/cygwin>, |
| <mailto:cygwin-request AT cygwin DOT com?subject=subscribe> | |
| From: | Takashi Yano via Cygwin <cygwin AT cygwin DOT com> |
| Reply-To: | Takashi Yano <takashi DOT yano AT nifty DOT ne DOT jp> |
| 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 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 DOT yano AT nifty DOT ne DOT 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
| webmaster | delorie software privacy |
| Copyright © 2019 by DJ Delorie | Updated Jul 2019 |