[PATCH] posix: Fix system blocks SIGCHLD erroneously [BZ #30163]

[Adam Yi]

Looks like some spaces/tabs got mixed up in the email. Please use the
attachment instead.

On Sat, Feb 25, 2023 at 1:31 AM Adam Yi <ayi@janestreet.com> wrote:
>
> Fix bug that SIGCHLD is erroneously blocked forever in the following
> scenario:
>
> 1. Thread A calls system but hasn't returned yet
> 2. Thread B calls another system but returns
>
> SIGCHLD would be blocked forever in thread B after its system() returns,
> even after the system() in thread A returns.
>
> Although POSIX does not require, glibc system implementation aims to be
> thread and cancellation safe. This bug was introduced in
> 5fb7fc96350575c9adb1316833e48ca11553be49 when we moved reverting signal
> mask to happen when the last concurrently running system returns,
> despite that signal mask is per thread. This commit reverts this logic
> and adds a test.
> ---
>  stdlib/tst-system.c       | 27 +++++++++++++++++++++++++++
>  support/shell-container.c | 10 ++++++++++
>  sysdeps/posix/system.c    |  6 +++---
>  3 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/stdlib/tst-system.c b/stdlib/tst-system.c
> index 634acfe264..be95c2de44 100644
> --- a/stdlib/tst-system.c
> +++ b/stdlib/tst-system.c
> @@ -20,6 +20,8 @@
>  #include <string.h>
>  #include <signal.h>
>  #include <paths.h>
> +#include <pthread.h>
> +#include <inttypes.h>
>
>  #include <support/capture_subprocess.h>
>  #include <support/check.h>
> @@ -71,6 +73,19 @@ call_system (void *closure)
>      }
>  }
>
> +static void *
> +sleep_and_check_sigchld (void *seconds)
> +{
> +  char cmd[namemax];
> +  sprintf(cmd, "sleep %" PRIdPTR, (intptr_t) seconds);
> +  TEST_COMPARE (system (cmd), 0);
> +
> +  sigset_t blocked = {0};
> +  TEST_COMPARE (sigprocmask (SIG_BLOCK, NULL, &blocked), 0);
> +  TEST_COMPARE (sigismember (&blocked, SIGCHLD), 0);
> +  return NULL;
> +}
> +
>  static int
>  do_test (void)
>  {
> @@ -154,6 +169,18 @@ do_test (void)
>      xchmod (_PATH_BSHELL, st.st_mode);
>    }
>
> +  {
> +    pthread_t long_sleep_thread, short_sleep_thread;
> +
> +    TEST_COMPARE (pthread_create (&long_sleep_thread, NULL,
> +                  sleep_and_check_sigchld, (void *) 2), 0);
> +    TEST_COMPARE (pthread_create (&short_sleep_thread, NULL,
> +                  sleep_and_check_sigchld, (void *) 1), 0);
> +
> +    TEST_COMPARE (pthread_join (short_sleep_thread, NULL), 0);
> +    TEST_COMPARE (pthread_join (long_sleep_thread, NULL), 0);
> +  }
> +
>    TEST_COMPARE (system (""), 0);
>
>    return 0;
> diff --git a/support/shell-container.c b/support/shell-container.c
> index e9ac9b6d04..083426550b 100644
> --- a/support/shell-container.c
> +++ b/support/shell-container.c
> @@ -171,6 +171,15 @@ kill_func (char **argv)
>    return 0;
>  }
>
> +/* Emulate the "/bin/sleep" command.  Options are ignored.  */
> +static int
> +sleep_func (char **argv)
> +{
> +  int secs = atoi (argv[0]);
> +  sleep (secs);
> +  return 0;
> +}
> +
>  /* This is a list of all the built-in commands we understand.  */
>  static struct {
>    const char *name;
> @@ -181,6 +190,7 @@ static struct {
>    { "cp", copy_func },
>    { "exit", exit_func },
>    { "kill", kill_func },
> +  { "sleep", sleep_func },
>    { NULL, NULL }
>  };
>
> diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c
> index 2335a99184..d77720a625 100644
> --- a/sysdeps/posix/system.c
> +++ b/sysdeps/posix/system.c
> @@ -179,16 +179,16 @@ do_system (const char *line)
>        as if the shell had terminated using _exit(127).  */
>     status = W_EXITCODE (127, 0);
>
> +  /* sigaction can not fail with SIGINT/SIGQUIT used with old
> +     disposition.  Same applies for sigprocmask.  */
>    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 ();
> +  __sigprocmask (SIG_SETMASK, &omask, NULL);
>
>    if (ret != 0)
>      __set_errno (ret);
> --
> 2.31.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-posix-Fix-system-blocks-SIGCHLD-erroneously-BZ-30163.patch
Type: text/x-patch
Size: 4001 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20230225/354ec1fd/attachment.bin>