[Bug libc/30163] New: system() erroneously block SIGCHLD forever when called concurrently

[Bug libc/30163] New: system() erroneously block SIGCHLD forever when called concurrently

[ayi at janestreet dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30163

            Bug ID: 30163
           Summary: system() erroneously block SIGCHLD forever when called
                    concurrently
           Product: glibc
           Version: 2.29
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: libc
          Assignee: unassigned at sourceware dot org
          Reporter: ayi at janestreet dot com
                CC: drepper.fsp at gmail dot com
  Target Milestone: ---

Created attachment 14718
  --> https://sourceware.org/bugzilla/attachment.cgi?id=14718&action=edit
Reproduction code

Description
-----------

Although POSIX does not require, glibc system implementation aims to be
thread and cancellation safe.

However, I found that commit 5fb7fc96350575c9adb1316833e48ca11553be49
introduced a concurrency bug. Specifically, in the following scenario with two
threads in the same process:

1. Thread A calls system but hasn't returned yet
2. Thread B calls another system but returns

I observed that SIGCHLD would be blocked forever in thread B after its system()
returns, even after the system() in thread A returns (though I believe it
should unblock it after system() in thread B returns).

Analysis
--------

We always block SIGCHLD and store original signal mask in omask before calling
posix_spawn:

```
// ...
 __sigemptyset (&sa.sa_mask);    
 // ...
 __sigaddset (&sa.sa_mask, SIGCHLD);                                            
 /* sigprocmask can not fail with SIG_BLOCK used with valid input               
    arguments.  */                                                              
 __sigprocmask (SIG_BLOCK, &sa.sa_mask, &omask); 
 // ...
 __posix_spawnattr_setsigmask (&spawn_attr, &omask);
```

However, when it comes to restoring to omask in parent process, the code
previously looks like this:

```

  DO_LOCK ();
  if ((SUB_REF () == 0
       && (__sigaction (SIGINT, &intr, (struct sigaction *) NULL)
       | __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL)) != 0)
      || __sigprocmask (SIG_SETMASK, &omask, (sigset_t *) NULL) != 0)
    {
#ifndef _LIBC
      /* glibc cannot be used on systems without waitpid.  */
      if (errno == ENOSYS)
    __set_errno (save);
      else
#endif
    status = -1;
    }
  DO_UNLOCK ();
```

The aforementioned buggy commit changed it to this:

```

DO_LOCK ();
if (SUB_REF () == 0)
  {
    /* sigaction can not fail with SIGINT/SIGQUIT used with old
   disposition.  Same applies for sigprocmask.  */
    __sigaction (SIGINT, &intr, NULL);
    __sigaction (SIGQUIT, &quit, NULL);
    __sigprocmask (SIG_SETMASK, &omask, NULL);
  }
DO_UNLOCK ();
```

Previously we would unconditionally revert mask back to omask but only call
sigaction if it's the last one (since that's shared for the whole process); now
we also only revert if it's the last ongoing system, despite that signal mask
is per-thread.

Fix
---

I believe we should move the last `__sigprocmask` out of this if statement. I
already have a patch ready and will send it over later today.

Reproduction
------------
I've attached a simple C reproduction program to this bug. It doesn't print
anything on releases/2.28/master (before introducing the bug) but prints
"SIGCHLD is erroneously blocked" on releases/2.29/master and on latest master
branch.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30163] system() erroneously block SIGCHLD forever when called concurrently

[ayi at janestreet dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30163

Adam Yi <ayi at janestreet dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ayi at janestreet dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30163] system() erroneously block SIGCHLD forever when called concurrently

[ayi at janestreet dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30163

--- Comment #1 from Adam Yi <ayi at janestreet dot com> ---
Created attachment 14720
  --> https://sourceware.org/bugzilla/attachment.cgi?id=14720&action=edit
Proposed patch

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30163] system() erroneously block SIGCHLD forever when called concurrently

[ayi at janestreet dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30163

Adam Yi <ayi at janestreet dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |adhemerval.zanella at linaro dot o
                   |                            |rg

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30163] system() erroneously block SIGCHLD forever when called concurrently

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30163

--- Comment #2 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
Can you send the patch to libc-alpha? All development is done there, along with
patch reviews and discussions.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30163] system() erroneously block SIGCHLD forever when called concurrently

[ayi at janestreet dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30163

--- Comment #3 from Adam Yi <ayi at janestreet dot com> ---
(In reply to Adhemerval Zanella from comment #2)
> Can you send the patch to libc-alpha? All development is done there, along
> with patch reviews and discussions.

Will do, thanks!

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30163] system() erroneously block SIGCHLD forever when called concurrently

[ayi at janestreet dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=30163

Adam Yi <ayi at janestreet dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #4 from Adam Yi <ayi at janestreet dot com> ---
Fixed by 436a604b7dc741fc76b5a6704c6cd8bb178518e7

-- 
You are receiving this mail because:
You are on the CC list for the bug.