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

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

On 24/02/23 12:27, Adam Yi 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.

Thanks for working on this, just some more minor comments below.

> ---
>  stdlib/tst-system.c          | 28 ++++++++++++++++++++
>  support/Makefile             |  2 ++
>  support/dtotimespec-time64.c | 27 +++++++++++++++++++
>  support/dtotimespec.c        | 50 ++++++++++++++++++++++++++++++++++++
>  support/shell-container.c    | 13 ++++++++++
>  support/timespec.h           |  4 +++
>  sysdeps/posix/system.c       |  6 ++---
>  7 files changed, 127 insertions(+), 3 deletions(-)
>  create mode 100644 support/dtotimespec-time64.c
>  create mode 100644 support/dtotimespec.c
> 
> diff --git a/stdlib/tst-system.c b/stdlib/tst-system.c
> index 634acfe264..736d2bf95b 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,20 @@ call_system (void *closure)
>      }
>  }
>  
> +static void *
> +sleep_and_check_sigchld (void *closure)
> +{
> +  double *seconds = (double *) closure;
> +  char cmd[namemax];
> +  sprintf (cmd, "sleep %lf" , *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 +170,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, &(double) { 0.2 }), 0);
> +    TEST_COMPARE (pthread_create (&short_sleep_thread, NULL,
> +                  sleep_and_check_sigchld, &(double) { 0.1 }), 0);
> +
> +    TEST_COMPARE (pthread_join (short_sleep_thread, NULL), 0);
> +    TEST_COMPARE (pthread_join (long_sleep_thread, NULL), 0);


Use xpthread_create and xpthread_join here:

  pthread_t short_sleep_thread = xpthread_create (NULL, 
	sleep_and_check_sigchld, &(double) { 0.2 });
  pthread_t long_sleep_thread = xpthread_create (NULL,
	sleep_and_check_sigchld, &(double) { 0.1 });
  xpthread_join (short_sleep_thread);
  xpthread_join (sleep_and_check_sigchld);

> +  }
> +
>    TEST_COMPARE (system (""), 0);
>  
>    return 0> diff --git a/support/Makefile b/support/Makefile
> index b29b7eb505..48cd74581d 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -32,6 +32,8 @@ libsupport-routines = \
>    check_hostent \
>    check_netent \
>    delayed_exit \
> +  dtotimespec \
> +  dtotimespec-time64 \
>    ignore_stderr \
>    next_to_fault \
>    oom_error \

Ok.

> diff --git a/support/dtotimespec-time64.c b/support/dtotimespec-time64.c
> new file mode 100644
> index 0000000000..b3d5e351e3
> --- /dev/null
> +++ b/support/dtotimespec-time64.c
> @@ -0,0 +1,27 @@
> +/* Convert double to timespec.  64-bit time support.
> +   Copyright (C) 2011-2023 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library and is also part of gnulib.
> +   Patches to this file should be submitted to both projects.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <time.h>
> +
> +#if __TIMESIZE != 64
> +# define timespec      __timespec64
> +# define time_t        __time64_t
> +# define dtotimespec   dtotimespec_time64
> +# include "dtotimespec.c"
> +#endif

Ok.

> diff --git a/support/dtotimespec.c b/support/dtotimespec.c
> new file mode 100644
> index 0000000000..860403b83c
> --- /dev/null
> +++ b/support/dtotimespec.c
> @@ -0,0 +1,50 @@
> +/* Convert double to timespec.
> +   Copyright (C) 2011-2023 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library and is also part of gnulib.
> +   Patches to this file should be submitted to both projects.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +/* Convert the double value SEC to a struct timespec.  Round toward
> +   positive infinity.  On overflow, return an extremal value.  */
> +
> +#include <support/timespec.h>
> +#include <intprops.h>
> +
> +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);
> +    }
> +}

Ok.

> diff --git a/support/shell-container.c b/support/shell-container.c
> index e9ac9b6d04..a2be314ad9 100644
> --- a/support/shell-container.c
> +++ b/support/shell-container.c
> @@ -39,6 +39,7 @@
>  #include <error.h>
>  
>  #include <support/support.h>
> +#include <support/timespec.h>
>  
>  /* Design considerations
>  
> @@ -171,6 +172,17 @@ kill_func (char **argv)
>    return 0;
>  }
>  
> +/* Emulate the "/bin/sleep" command.  No suffix support.  Options are
> +   ignored.  */
> +static int
> +sleep_func (char **argv)
> +{
> +  double sec = atof (argv[0]);

Add some parsing check here:

   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 and only positive values.  */
   TEST_VERIFY_EXIT (sec >= 0.0);

> +  struct timespec ts = dtotimespec (sec);
> +  nanosleep (&ts, NULL);
> +  return 0;
> +}
> +
>  /* This is a list of all the built-in commands we understand.  */
>  static struct {
>    const char *name;
> @@ -181,6 +193,7 @@ static struct {
>    { "cp", copy_func },
>    { "exit", exit_func },
>    { "kill", kill_func },
> +  { "sleep", sleep_func },
>    { NULL, NULL }
>  };
>  
> diff --git a/support/timespec.h b/support/timespec.h
> index 77b1e4e8d6..9559836d4c 100644
> --- a/support/timespec.h
> +++ b/support/timespec.h
> @@ -57,6 +57,8 @@ int support_timespec_check_in_range (struct timespec expected,
>  				     struct timespec observed,
>  				     double lower_bound, double upper_bound);
>  
> +struct timespec dtotimespec (double sec) __attribute__((const));
> +
>  #else
>  struct timespec __REDIRECT (timespec_add, (struct timespec, struct timespec),
>  			    timespec_add_time64);
> @@ -82,6 +84,8 @@ int __REDIRECT (support_timespec_check_in_range, (struct timespec expected,
>  						  double lower_bound,
>  						  double upper_bound),
>  		support_timespec_check_in_range_time64);
> +
> +struct timespec __REDIRECT (dtotimespec, (double sec), dtotimespec_time64);
>  #endif
>  
>  /* Check that the timespec on the left represents a time before the
> 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);