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

  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(-)
  

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
  

On 24/02/23 15:00, Adam Yi wrote:
> Looks like some spaces/tabs got mixed up in the email. Please use the
> attachment instead.

Next time just send a v2, it is better to keep track with you patchwork
instance [1].

[1] https://patchwork.sourceware.org/project/glibc/list/

> 
> 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.

Patch looks ok, I would recommend a v3 to enhance the testing a bit.

>> ---
>>  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;

Do we really need to sleep for 2 seconds here? It adds more latency on
testcase run.

>> 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]);

I think it would be better add support for split second, similar to
coreutils.  Something like:

#include <support/check.h>
#include <support/timespec.h>

/* I shamelessly copied it from gnulib, move it to support/dtotimespec.c.  */
struct timespec
dtotimespec (double sec)
{
  if (! (TYPE_MINIMUM (time_t) < sec))
    return make_timespec (TYPE_MINIMUM (time_t), 0);
  else if (! (sec < 1.0 + TYPE_MAXIMUM (time_t)))
    return make_timespec (TYPE_MAXIMUM (time_t), TIMESPEC_HZ - 1);
  else
    {
      time_t s = sec;
      double frac = TIMESPEC_HZ * (sec - s);
      long ns = frac;
      ns += ns < frac;
      s += ns / TIMESPEC_HZ;
      ns %= TIMESPEC_HZ;

      if (ns < 0)
        {
          s--;
          ns += TIMESPEC_HZ;
        }

      return make_timespec (s, ns);
    }
}

static int
sleep_func (char **argv)
{
   TEST_VERIFY_EXIT  (argv[0] != NULL);
   char *endptr = NULL;
   double sec = strtod (argv[0], &endptr);
   TEST_VERIFY_EXIT (endptr != argv[0] && errno != ERANGE);
   /* No suffix support.  */
   TEST_VERIFY_EXIT (sec >= 0.0);
   
   struct timespec ts = dtotimespec (sec);
   /* We don't expect any signal here.  */
   TEST_VERIFY_EXIT (nanosleep (&ts, NULL) == 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);

Ok, thanks for spotting it.

>> --
>> 2.31.1
  

On Sat, Feb 25, 2023 at 4:18 AM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 24/02/23 15:00, Adam Yi wrote:
> > Looks like some spaces/tabs got mixed up in the email. Please use the
> > attachment instead.
>
> Next time just send a v2, it is better to keep track with you patchwork
> instance [1].
>
> [1] https://patchwork.sourceware.org/project/glibc/list/

Got it, thanks!
>
> >
> > 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.
>
> Patch looks ok, I would recommend a v3 to enhance the testing a bit.

Thanks for the suggestion! I've sent over a v3 to add split second support.

>
> >> ---
> >>  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;
>
> Do we really need to sleep for 2 seconds here? It adds more latency on
> testcase run.
>
> >> 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]);
>
> I think it would be better add support for split second, similar to
> coreutils.  Something like:
>
> #include <support/check.h>
> #include <support/timespec.h>
>
> /* I shamelessly copied it from gnulib, move it to support/dtotimespec.c.  */
> struct timespec
> dtotimespec (double sec)
> {
>   if (! (TYPE_MINIMUM (time_t) < sec))
>     return make_timespec (TYPE_MINIMUM (time_t), 0);
>   else if (! (sec < 1.0 + TYPE_MAXIMUM (time_t)))
>     return make_timespec (TYPE_MAXIMUM (time_t), TIMESPEC_HZ - 1);
>   else
>     {
>       time_t s = sec;
>       double frac = TIMESPEC_HZ * (sec - s);
>       long ns = frac;
>       ns += ns < frac;
>       s += ns / TIMESPEC_HZ;
>       ns %= TIMESPEC_HZ;
>
>       if (ns < 0)
>         {
>           s--;
>           ns += TIMESPEC_HZ;
>         }
>
>       return make_timespec (s, ns);
>     }
> }
>
> static int
> sleep_func (char **argv)
> {
>    TEST_VERIFY_EXIT  (argv[0] != NULL);
>    char *endptr = NULL;
>    double sec = strtod (argv[0], &endptr);
>    TEST_VERIFY_EXIT (endptr != argv[0] && errno != ERANGE);
>    /* No suffix support.  */
>    TEST_VERIFY_EXIT (sec >= 0.0);
>
>    struct timespec ts = dtotimespec (sec);
>    /* We don't expect any signal here.  */
>    TEST_VERIFY_EXIT (nanosleep (&ts, NULL) == 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);
>
> Ok, thanks for spotting it.
>
> >> --
> >> 2.31.1