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.

Signed-off-by: Adam Yi <ayi@janestreet.com>
Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
(cherry picked from commit 436a604b7dc741fc76b5a6704c6cd8bb178518e7)

diff --git a/NEWS b/NEWS
index 545ede4f558e84a870a6a3ba6398498a9b1453ba..01f05ce75e70b053fe4d3aea01a4890614b16277 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -140,6 +140,7 @@ The following bugs are resolved with this release:
   [29953] x86: Check minimum/maximum of non_temporal_threshold
   [30053] time: strftime %s returns -1 after 2038 on 32 bits systems
   [30151] gshadow: Matching sgetsgent, sgetsgent_r ERANGE handling
+  [30163] posix: Fix system blocks SIGCHLD erroneously
   [30305] x86_64: Fix asm constraints in feraiseexcept
 
 \f

diff --git a/stdlib/tst-system.c b/stdlib/tst-system.c
index 178808e048a9b3bc4668ad6001e1ec60f6d2b397..d1413d7c6a4cb82c9b66142a3016ab92a71137c8 100644 (file)
--- a/stdlib/tst-system.c
+++ b/stdlib/tst-system.c
@@ -26,6 +26,7 @@
 #include <support/check.h>
 #include <support/temp_file.h>
 #include <support/support.h>
+#include <support/xthread.h>
 #include <support/xunistd.h>
 
 static char *tmpdir;
@@ -72,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)
 {
@@ -155,6 +170,17 @@ do_test (void)
     xchmod (_PATH_BSHELL, st.st_mode);
   }
 
+  {
+    pthread_t long_sleep_thread = xpthread_create (NULL,
+                                                   sleep_and_check_sigchld,
+                                                   &(double) { 0.2 });
+    pthread_t short_sleep_thread = xpthread_create (NULL,
+                                                    sleep_and_check_sigchld,
+                                                    &(double) { 0.1 });
+    xpthread_join (short_sleep_thread);
+    xpthread_join (long_sleep_thread);
+  }
+
   TEST_COMPARE (system (""), 0);
 
   return 0;

diff --git a/support/Makefile b/support/Makefile
index d008a20a0e2afa0f5dfa80993f9ff8ff50d7efaa..6f4b7e4b45094496f20091147bc0c679fbaed636 100644 (file)
--- 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 \

diff --git a/support/dtotimespec-time64.c b/support/dtotimespec-time64.c
new file mode 100644 (file)
index 0000000..b3d5e35
--- /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

diff --git a/support/dtotimespec.c b/support/dtotimespec.c
new file mode 100644 (file)
index 0000000..cde5b4d
--- /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 (sec <= TYPE_MINIMUM (time_t))
+    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);
+    }
+}

diff --git a/support/shell-container.c b/support/shell-container.c
index b2a4324dc7444be5a9034f4a39a399467c16dd4e..6fe925dc49075848dc0d00be9646225168615d45 100644 (file)
--- 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,32 @@ kill_func (char **argv)
   return 0;
 }
 
+/* Emulate the "/bin/sleep" command.  No suffix support.  Options are
+   ignored.  */
+static int
+sleep_func (char **argv)
+{
+  if (argv[0] == NULL)
+    {
+      fprintf (stderr, "sleep: missing operand\n");
+      return 1;
+    }
+  char *endptr = NULL;
+  double sec = strtod (argv[0], &endptr);
+  if (endptr == argv[0] || errno == ERANGE || sec < 0)
+    {
+      fprintf (stderr, "sleep: invalid time interval '%s'\n", argv[0]);
+      return 1;
+    }
+  struct timespec ts = dtotimespec (sec);
+  if (nanosleep (&ts, NULL) < 0)
+    {
+      fprintf (stderr, "sleep: failed to nanosleep: %s\n", strerror (errno));
+      return 1;
+    }
+  return 0;
+}
+
 /* This is a list of all the built-in commands we understand.  */
 static struct {
   const char *name;
@@ -181,6 +208,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 0478aef51fdcc5ae265e402f9f3dddbecff9a202..843a90d60675f37ddd6617f95001c5f81ac26572 100644 (file)
--- 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 48668fb392e67d1a84bb8ffef480e4237b7b4645..b9676abb1c6f35eb942faf3e8d2a48d05f8aa88f 100644 (file)
--- 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);