DKIM-Filter:	OpenDKIM Filter v2.11.0 delorie.com 44VE2eYe2532004
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=N6KdKf1W
X-Recipient:	archive-cygwin AT delorie DOT com
DKIM-Filter:	OpenDKIM Filter v2.11.0 sourceware.org AFCCF385ED4A
DKIM-Signature:	v=1; a=rsa-sha256; c=relaxed/relaxed; d=cygwin.com;
	s=default; t=1717164159;
	bh=AtfCLlSlwlQ7ZI0plurdyLKnjejwGn3VmfP4nYcslg4=;
	h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe:
	List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:
	From;
	b=N6KdKf1WXPgKT4SjlZRtnk5Fn5fgtpgotB2MRNITesKqjpgqPVWA5MSQQHJOU9/iL
	mrjxdnw1UlukFQsAJhtOMoFuXye45pRTaC05ZjY3c/LJHG6BHMD8w0VTv3LqwhwCvC
	1WjRWY8UAlqQBlSmN5AcDFBRhMZJkyGM+hH74BDc=
X-Original-To:	cygwin AT cygwin DOT com
Delivered-To:	cygwin AT cygwin DOT com
DMARC-Filter:	OpenDMARC Filter v1.4.2 sourceware.org 5273D3858C60
ARC-Filter:	OpenARC Filter v1.0.0 sourceware.org 5273D3858C60
ARC-Seal:	i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1717164106; cv=pass;
	b=Mb9wBSpgWfrjM5+4iVCO14doBCtIk77FJMmIdEhIqHVkrJSUWzvQKla9gDK4xSuKBMjG/1RwtEaaQAMHu+4nL7kShWdeo0r6uxQ2Dh8lUyY0Cm/sfOmPYsF9SxmibcfyAn6uDeam2xn5UNmekwU9fJAq1AeoaRE6/+ne0uZ6Yx8=
ARC-Message-Signature:	i=2; a=rsa-sha256; d=sourceware.org; s=key;
	t=1717164106; c=relaxed/simple;
	bh=WB+cZX1vLup4YODHtdR3nEIDsZhARDUV/UyMGU44sMY=;
	h=DKIM-Signature:DKIM-Signature:From:To:Subject:Date:Message-ID:
	MIME-Version;
	b=rUZBq6UpS4gNeiv66ZCYky6ADTB9lPwbk3uLQ2dHACvLg6UlGq5oLcCeZsaw8JMpvTkhKNWaeGb9F5mh47ZuSAUrr/p+fSqaW7E29O0UVl7yaDkBEEn8d/rhggglzxXKPfMrJjzV/LevD4dbwgnLCvgqEq2Mymh5IlLWCGRgYwo=
ARC-Authentication-Results:	i=2; server2.sourceware.org
ARC-Seal:	i=1; a=rsa-sha256; t=1717164096; cv=none;
	d=strato.com; s=strato-dkim-0002;
	b=YxGAQtOsms4ru23GiDbbz1MejmBqqrNUilvPmhz9LUdwHl62UFsKamFk0TDoaoTVfK
	MxwuUqhIwZlY1Ejp4U0ispsNaeHNaoKwkBKJCINk6uGcutTU9ko1DJus4PCIkjzCsV3a
	816mNdpCS+rfDU2m9t8p8bD8p08wbzJt6y0sNwq13eDDhxMLC7jvu6Jp1BGvmtflnPD9
	c1oBUFivVGBkfI7YoZ+T1AbVflOTdCKx6r/Zy51qDPr1CWzJST1LTzz/RePvE98UoFdh
	fG+IAvK/RhmwupwJozbLD3e443aKP3OVDy8E34SAbnQ0JhqelUZhxcIGFwBwSfi1JGKt
	QD9w==
ARC-Message-Signature:	i=1; a=rsa-sha256; c=relaxed/relaxed; t=1717164096;
	s=strato-dkim-0002; d=strato.com;
	h=References:In-Reply-To:Message-ID:Date:Subject:To:From:Cc:Date:From:
	Subject:Sender;
	bh=+xyRmqClR925b5WNMnClYhEmxqvHLQuONHM3zM0HBpY=;
	b=TtihjUbAS9HV1cOOL+jmHZhQc6dDqRtlK7nUotj2unYs1qZ/PNFPNohOpvootNCRzp
	m+4CJYzilMiJlAxQNim5gqQ7tJ84ugL7++2TyAcBE+k/64L/G4JNPzINka8nYFfIUt1X
	qj7MdYzFx5ZFARrWpgrnnkY1fvNp9icFECtnb0Y91ZYLleHIsRGWJWL1c0n0R7CfqQXB
	cxegk9A5kZ9l84obtMBIQpXHTJR9aTqy5CBr2mqGXPOjj7ygvV34vfg8x1FeasoNrbYA
	+rPZo9TpiOg8gW0gkjD2mLNul9qExrrCP139u30nZjbwE/T2B7rAQ9jJaZSR/aQq3vXG
	/B+w==
ARC-Authentication-Results:	i=1; strato.com;
	arc=none;
	dkim=none
X-RZG-CLASS-ID:	mo00
X-RZG-AUTH:	":Ln4Re0+Ic/6oZXR1YgKryK8brlshOcZlIWs+iCP5vnk6shH0WWb0LN8XZoH94zq68+3cfpOT2PWVAMbaeIYvfnxGcywKjdvH"
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
X-Spam-Status:	No, score=-2.9 required=5.0 tests=BAYES_00, DKIM_SIGNED,
	DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_NUMSUBJECT, RCVD_IN_DNSWL_NONE,
	RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_NONE, TXREP,
	T_SCC_BODY_TEXT_LINE autolearn=no 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:	Bruno Haible via Cygwin <cygwin AT cygwin DOT com>
Reply-To:	Bruno Haible <bruno AT clisp DOT org>
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

- Raw text -