From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.sergiodj.net (mail.sergiodj.net [195.201.110.160]) by sourceware.org (Postfix) with ESMTPS id B5AFA3858C01 for ; Wed, 23 Aug 2023 14:10:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B5AFA3858C01 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=sergiodj.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=sergiodj.net DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=sergiodj.net; s=20160602; t=1692799850; bh=5EtqKpQE18NhqLF01wUMixzpmeH0qvyBdaUnjv7pYnY=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=rheFfMaZmoYuhlEtDRddiH1NHA56anl3VWcpnBlpC5qElIuJLdOTIM2+SXOixNB5T wv2L8O/6CWBQzGCdJ6w7Dz4cJIxuks3tvlSlfyg+DNcnjP6FN/Mt+ZTfxEt66d/Jua UdDnfd0hf/HndbbWSoDmbgGnVineYvrpX6rsI2/o= Received: from localhost (24-212-170-20.cable.teksavvy.com [24.212.170.20]) by mail.sergiodj.net (Postfix) with ESMTPSA id 18D92A602AE; Wed, 23 Aug 2023 10:10:49 -0400 (EDT) From: Sergio Durigan Junior To: Andreas Schwab Cc: Sergio Durigan Junior via Libc-alpha Subject: Re: [PATCH] sysdeps: Clear O_CREAT|O_ACCMODE when trying again on sem_open In-Reply-To: (Andreas Schwab's message of "Wed, 23 Aug 2023 09:40:54 +0200") References: <20230823042129.3955131-1-sergiodj@sergiodj.net> X-URL: http://blog.sergiodj.net Date: Wed, 23 Aug 2023 10:10:48 -0400 Message-ID: <87h6oprgrb.fsf@sergiodj.net> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Wednesday, August 23 2023, Andreas Schwab wrote: > On Aug 23 2023, Sergio Durigan Junior via Libc-alpha wrote: > >> The fix here is to actually make sure that the O_CREAT|O_ACCMODE flags >> are clear after we enter "try_again". > > That doesn't match what the patch does. It only adds flags to > open_flags, and does not remove any. You're correct. Simon also spotted the same problem. Here's an updated version of the patch. -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible https://sergiodj.net/ >From cb1a17590878955bcf6d7c2821ca95da3896608c Mon Sep 17 00:00:00 2001 From: Sergio Durigan Junior Date: Wed, 23 Aug 2023 00:10:44 -0400 Subject: [PATCH] sysdeps: Clear O_CREAT|O_ACCMODE when trying again on sem_open When invoking sem_open with O_CREAT as one of its flags, we'll end up in the second part of sem_open's "if ((oflag & O_CREAT) == 0 || (oflag & O_EXCL) == 0)", which means that we don't expect the semaphore file to exist. In that part, open_flags is initialized as "O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC" and there's an attempt to open(2) the file, which will likely fail because it won't exist. After that first (expected) failure, some cleanup is done and we go back to the label "try_again", which lives in the first part of the aforementioned "if". The problem is that, in that part of the code, we expect the semaphore file to exist, and as such O_CREAT (this time the flag we pass to open(2)) needs to be cleaned from open_flags, otherwise we'll see another failure (this time unexpected) when trying to open the file, which will lead the call to sem_open to fail as well. This can cause very strange bugs, especially with OpenMPI, which makes extensive use of semaphores. The fix here is to actually make sure that the O_CREAT|O_ACCMODE flags are clear after we enter "try_again". See also: https://bugs.launchpad.net/ubuntu/+source/h5py/+bug/2031912 Signed-off-by: Sergio Durigan Junior Co-Authored-By: Simon Chopin Fixes: 533deafbdf189f5fbb280c28562dd43ace2f4b0f ("Use O_CLOEXEC in more places (BZ #15722)") --- sysdeps/pthread/sem_open.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/sysdeps/pthread/sem_open.c b/sysdeps/pthread/sem_open.c index e5db929d20..529c286541 100644 --- a/sysdeps/pthread/sem_open.c +++ b/sysdeps/pthread/sem_open.c @@ -36,7 +36,6 @@ sem_t * __sem_open (const char *name, int oflag, ...) { int fd; - int open_flags; sem_t *result; /* Check that shared futexes are supported. */ @@ -65,10 +64,9 @@ __sem_open (const char *name, int oflag, ...) /* If the semaphore object has to exist simply open it. */ if ((oflag & O_CREAT) == 0 || (oflag & O_EXCL) == 0) { - open_flags = O_RDWR | O_NOFOLLOW | O_CLOEXEC; - open_flags |= (oflag & ~(O_CREAT|O_ACCMODE)); try_again: - fd = __open (dirname.name, open_flags); + fd = __open (dirname.name, + (oflag & ~(O_CREAT|O_ACCMODE)) | O_NOFOLLOW | O_RDWR | O_CLOEXEC); if (fd == -1) { @@ -135,8 +133,7 @@ __sem_open (const char *name, int oflag, ...) } /* Open the file. Make sure we do not overwrite anything. */ - open_flags = O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC; - fd = __open (tmpfname, open_flags, mode); + fd = __open (tmpfname, O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, mode); if (fd == -1) { if (errno == EEXIST) -- 2.39.2