DKIM-Filter:	OpenDKIM Filter v2.11.0 delorie.com 44UAFH0L2054029
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=OZrWDoIt
X-Recipient:	archive-cygwin AT delorie DOT com
DKIM-Filter:	OpenDKIM Filter v2.11.0 sourceware.org 71C2B3850200
DKIM-Signature:	v=1; a=rsa-sha256; c=relaxed/relaxed; d=cygwin.com;
	s=default; t=1717064115;
	bh=QQnDPTXPHYFDy/fygRQq5apru3gcYurLA67VxDCqsjU=;
	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=OZrWDoItphdJgWlC+VL/4CPh3vVeejkMMoUS/M+l2ympRBJXT3xSDY9RdmCRk3XJX
	njLnkJkSN49DCKH7seeGGa9uQwp0TdzP7rVw0A0jEi+65EuwbYn2pU9W3mI+L9lN5X
	RLkUn0ZA8apDUEWy7cD04Vfu87czR8pvh7J91YXE=
X-Original-To:	cygwin AT cygwin DOT com
Delivered-To:	cygwin AT cygwin DOT com
DMARC-Filter:	OpenDMARC Filter v1.4.2 sourceware.org E70963858416
ARC-Filter:	OpenARC Filter v1.0.0 sourceware.org E70963858416
ARC-Seal:	i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1717064060; cv=pass;
	b=nwHOTqDPu2t0VECMcxGgjdluDYCLCu7Gcg2fIiTWDTuVKWvo/jG1lvzKhGV/fs2IEym2dsZ6FGkzQjmYzhReNuHAwbiVNzHHhe0l6YaMdsI17139OvkDi9Z5TxqfzXKjXbvEGMXDisTu/u5N8mM7rN/fSQ93qs4ujtR0GJs9UvM=
ARC-Message-Signature:	i=2; a=rsa-sha256; d=sourceware.org; s=key;
	t=1717064060; c=relaxed/simple;
	bh=6hSIkv9dhYV81UZU0E8TkX49qoJ71iNkirHsdL6vIeE=;
	h=DKIM-Signature:DKIM-Signature:From:To:Subject:Date:Message-ID:
	MIME-Version;
	b=Gnn5aRj3+fQI+M0/3753yGtpnraMvWcYLHln7eGNsz+2+g6AbQJWJY9aDWY9HYQMsUOAy+lIZCGRUVQ4PXeM2p4lXV3DgxK0TKpY2nkyhFxNfpMrUIZ4F1TUfjcMEK0nTW4nf406eBmcFt25kBONrjYTjOniabNILR0UkwpD8ks=
ARC-Authentication-Results:	i=2; server2.sourceware.org
ARC-Seal:	i=1; a=rsa-sha256; t=1717064051; cv=none;
	d=strato.com; s=strato-dkim-0002;
	b=XdR4kWqELvziZ+sIHqeG0Q/7YKl/Q5zVORbFbvZRhIPT/mTgOKcoVd3kI1a5GkvjjC
	sSYcBaVX7MCO+RfznIBmLa5PHA5jkOOdT6nmnGlZKeW1fiooEFJ0l4ymBLyYI/roA5ZK
	MwVH6/OehLXDCqtr8D90aXlkbkm+VZqawO/c8/k932xYcip1/iBU3nLB5O5dvFeZ35xd
	CMbcH7X5VbKb+XpLdGobwHsu1x9ua6wsXhutNZl8MvjP3kutuNvwlNwYm3a4gZ4TJzlU
	dLR6E7sy4We63HKDH4/wfn5YTs2t/JuFM6Wu7XEE/qQ5uH8Bc+vzzcGaVO7VxA1gENP+
	9vww==
ARC-Message-Signature:	i=1; a=rsa-sha256; c=relaxed/relaxed; t=1717064051;
	s=strato-dkim-0002; d=strato.com;
	h=References:In-Reply-To:Message-ID:Date:Subject:To:From:Cc:Date:From:
	Subject:Sender;
	bh=eozHNVpnMV15u2F70IdzWsmlcf5275bBewugfYkezl0=;
	b=l2Unk9srRnCH5FbpLgDzw7Lh+OZVxe22bEQeArxWXd0J6qVCBGsmrGLAcWOmctyBX0
	g6BwVqc98rzYI0eKvkSK+GHS/fk6mVdTwjmxHkgR1f/rEPQK7SVKf02FgAU8h6zwXJFg
	4e7ia62yjReL3p3m318OtmWqfkyOW47x3kykxvNRrFTuRc8iJYN1l1eDmdehExEm3n8n
	JddWQsl1YjoeLZ/+bhYmhAk0t3TVmZweKeUp5GpFY8owBhzjb1TbpJBamRCgVRWk6yXC
	ad6qNKNKDa4sNW7XCzUaCRuLbYclfko5I/ilmIoRE+A+5+ShtRbuTs4OK6SYsAiR3Iqf
	ikrg==
ARC-Authentication-Results:	i=1; strato.com;
	arc=none;
	dkim=none
X-RZG-CLASS-ID:	mo01
X-RZG-AUTH:	":Ln4Re0+Ic/6oZXR1YgKryK8brlshOcZlIWs+iCP5vnk6shH0WWb0LN8XZoH94zq68+3cfpOfjqSRCBA3DxwVMPzYVsfrcNDf"
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:	Thu, 30 May 2024 12:14:10 +0200
Message-ID:	<5613635.1WZ037k8cV@nimes>
In-Reply-To:	<20240530050538.53724-1-takashi.yano@nifty.ne.jp>
References:	<20240530050538 DOT 53724-1-takashi DOT yano AT nifty DOT ne DOT jp>
MIME-Version:	1.0
X-Spam-Status:	No, score=-3.3 required=5.0 tests=BAYES_00, DKIM_SIGNED,
	DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_NUMSUBJECT, RCVD_IN_DNSWL_LOW,
	RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_NONE, 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:	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>

Takashi Yano wrote in cygwin-patches:
>  int
>  pthread::once (pthread_once_t *once_control, void (*init_routine) (void))
>  {
> -  // already done ?
> -  if (once_control->state)
> +  /* Sign bit of once_control->state is used as done flag */
> +  if (once_control->state & INT_MIN)
>      return 0;
>  
> +  /* 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);
> -  /* Here we must set a cancellation handler to unlock the mutex if needed */
> -  /* but a cancellation handler is not the right thing. We need this in the thread
> -   *cleanup routine. Assumption: a thread can only be in one pthread_once routine
> -   *at a time. Stote a mutex_t *in the pthread_structure. if that's non null unlock
> -   *on pthread_exit ();
> -   */

Sorry, in a unified diff form this is unreadable. One needs to look at the
entire function. A context diff would have been better. So:

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;

  /* 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 ();
      once_control->state |= INT_MIN;
    }
  pthread_mutex_unlock (&once_control->mutex);
  if (InterlockedDecrement (&once_control->state) == INT_MIN)
    pthread_mutex_destroy (&once_control->mutex);
  return 0;
}

1) The overall logic seems right (using bit 31 of the state word as a
'done' bit), and the last thread that used the mutex destroys it.

2) However, the 'state' field is not volatile, and therefore the memory
model does not guarantee that an assignment

     once_control->state |= INT_MIN;

done in one thread has an effect on the values seen by other threads
that do

     if (once_control->state & INT_MIN)

As long as there is no synchronization between the two CPUs that execute
the two threads, one CPU may indefinitely see the old value of
once_control->state, while the other CPU's new value is not being
"broadcasted" to all CPUs.

3) Also, as noted by Noel Grandin, there is a race: If one thread does

     once_control->state |= INT_MIN;

while another thread does

     InterlockedIncrement (&once_control->state);
or
     InterlockedDecrement (&once_control->state)

the result can be that the increment or decrement is nullified. If it's
an increment that gets nullified, the pthread_mutex_destroy occurs too
early, with fatal consequences. If it's a decrement that get nullified,
the pthread_mutex_destroy never happens, and there is therefore a resource
leak.

4) Does the test program that I posted in
<https://cygwin.com/pipermail/cygwin/2024-May/255987.html> now pass?
Notes:
  - You should set
      #define ENABLE_DEBUGGING 0
    because with the debugging output, it nearly always succeeds.
  - You should run it 10 times in a row, not just once. It may well
    succeed 9 out of 10 times and fail 1 out of 10 times.

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 -