Re: [PATCH] sysdeps: sem_open: Clear O_CREAT when semaphore file is expected to exist [BZ #30789]

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 01/11/23 19:15, Sergio Durigan Junior wrote:
> 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.
> 
> Fix the bug by simplifying the logic when choosing open(2) flags and
> making sure O_CREAT is not set when the semaphore file is expected to
> exist.
> 
> A regression test for this issue would require a complex and cpu time
> consuming logic, since to trigger the wrong code path is not
> straightforward due the racy condition.  There is a somewhat reliable
> reproducer in the bug, but it requires using OpenMPI.
> 
> This resolves BZ #30789.
> 
> See also: https://bugs.launchpad.net/ubuntu/+source/h5py/+bug/2031912
> 
> Signed-off-by: Sergio Durigan Junior <sergiodj@sergiodj.net>
> Co-Authored-By: Simon Chopin <simon.chopin@canonical.com>
> Co-Authored-By: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
> Fixes: 533deafbdf189f5fbb280c28562dd43ace2f4b0f ("Use O_CLOEXEC in more places (BZ #15722)")

LGTM, thanks.  I will install this shortly.

> ---
>  sysdeps/pthread/sem_open.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/sysdeps/pthread/sem_open.c b/sysdeps/pthread/sem_open.c
> index e5db929d20..0e331a7445 100644
> --- a/sysdeps/pthread/sem_open.c
> +++ b/sysdeps/pthread/sem_open.c
> @@ -32,11 +32,12 @@
>  # define __unlink unlink
>  #endif
>  
> +#define SEM_OPEN_FLAGS (O_RDWR | O_NOFOLLOW | O_CLOEXEC)
> +
>  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 +66,8 @@ __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_EXCL) | SEM_OPEN_FLAGS);
>  
>        if (fd == -1)
>  	{
> @@ -135,8 +134,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_CREAT | O_EXCL | SEM_OPEN_FLAGS, mode);
>  	  if (fd == -1)
>  	    {
>  	      if (errno == EEXIST)