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

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

[Adam Yi]

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          | 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);
+  }
+
   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 \
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
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);
+    }
+}
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]);
+  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);
-- 
2.31.1

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

[Adam Yi]

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

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

[Adam Yi]

[-- Attachment #1: Type: text/plain, Size: 4092 bytes --]

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

[-- Attachment #2: 0001-posix-Fix-system-blocks-SIGCHLD-erroneously-BZ-30163.patch --]
[-- Type: text/x-patch, Size: 4001 bytes --]

From 67c4ce9e908bd9a1fd801eced8410661e27c0a3d Mon Sep 17 00:00:00 2001
From: Adam Yi <ayi@janestreet.com>
Date: Fri, 24 Feb 2023 10:27:41 -0500
Subject: [PATCH] posix: Fix system blocks SIGCHLD erroneously [BZ #30163]
To: libc-alpha@sourceware.org
Cc: adhemerval.zanella@linaro.org,
    i@adamyi.com

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

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

[Adhemerval Zanella Netto]

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

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

[Adam Yi]

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

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

[Adhemerval Zanella Netto]

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

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

[Adam Yi]

[-- Attachment #1: Type: text/plain, Size: 11708 bytes --]

Thanks for reviewing!

On Fri, Mar 3, 2023 at 12:42 AM Adhemerval Zanella Netto <
adhemerval.zanella@linaro.org> wrote:

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

Will do. Thanks!

>
> > +  }
> > +
> >    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);
>

I'm not sure if TEST_VERIFY_EXIT is the correct thing to do here, since
it's the mocked /bin/sh,
not directly part of the test. It's weird that providing invalid argument
to `sleep` would crash the shell.
Would it make more sense to print an error to stderr and return 1? FWIW, I
didn't add parsing check
here because I saw that other functions here do not have any sanity checks.
exit and kill are just
calling atoi without checking for errors and they don't check return values
either. There are some
checks in copy_func but it doesn't even check that there are two arguments
before referencing them.



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

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

[Adhemerval Zanella Netto]

On 03/03/23 02:11, Adam Yi wrote:
> Thanks for reviewing!

> 
>     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);
> 
> 
> I'm not sure if TEST_VERIFY_EXIT is the correct thing to do here, since it's the mocked /bin/sh,
> not directly part of the test. It's weird that providing invalid argument to `sleep` would crash the shell.
> Would it make more sense to print an error to stderr and return 1? FWIW, I didn't add parsing check
> here because I saw that other functions here do not have any sanity checks. exit and kill are just
> calling atoi without checking for errors and they don't check return values either. There are some
> checks in copy_func but it doesn't even check that there are two arguments before referencing them.

Printing an error to stderr is fine, and indeed it follow better
the mock shell way. 

I want to add some sanity check to avoid issuing invalid argument to sleep 
that returns immediately or setup an unexpected timer, which might be tricky 
to debug (specially in the container environment).  Failing early with a message 
catch possible tests issues.

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

[Adam Yi]

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          | 26 +++++++++++++++++++
 support/Makefile             |  2 ++
 support/dtotimespec-time64.c | 27 +++++++++++++++++++
 support/dtotimespec.c        | 50 ++++++++++++++++++++++++++++++++++++
 support/shell-container.c    | 28 ++++++++++++++++++++
 support/timespec.h           |  4 +++
 sysdeps/posix/system.c       |  6 ++---
 7 files changed, 140 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..47a0afe6bf 100644
--- a/stdlib/tst-system.c
+++ b/stdlib/tst-system.c
@@ -25,6 +25,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;
@@ -71,6 +72,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 +169,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 d52c472755..05b31159ea 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 \
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
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);
+    }
+}
diff --git a/support/shell-container.c b/support/shell-container.c
index ffa3378b5e..b1f9e793c1 100644
--- a/support/shell-container.c
+++ b/support/shell-container.c
@@ -37,6 +37,7 @@
 #include <error.h>
 
 #include <support/support.h>
+#include <support/timespec.h>
 
 /* Design considerations
 
@@ -169,6 +170,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;
@@ -179,6 +206,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);
-- 
2.31.1

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

[Andreas Schwab]

On Mär 05 2023, Adam Yi via Libc-alpha wrote:

> +  if (! (TYPE_MINIMUM (time_t) < sec))
     if (sec <= TYPE_MINIMUM (time_t))

> +  else if (! (sec < 1.0 + TYPE_MAXIMUM (time_t)))
     else if (sec >= 1.0 + TYPE_MAXIMUM (time_t))

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Adam Yi]

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          | 26 +++++++++++++++++++
 support/Makefile             |  2 ++
 support/dtotimespec-time64.c | 27 +++++++++++++++++++
 support/dtotimespec.c        | 50 ++++++++++++++++++++++++++++++++++++
 support/shell-container.c    | 28 ++++++++++++++++++++
 support/timespec.h           |  4 +++
 sysdeps/posix/system.c       |  6 ++---
 7 files changed, 140 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..47a0afe6bf 100644
--- a/stdlib/tst-system.c
+++ b/stdlib/tst-system.c
@@ -25,6 +25,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;
@@ -71,6 +72,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 +169,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 d52c472755..05b31159ea 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 \
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
diff --git a/support/dtotimespec.c b/support/dtotimespec.c
new file mode 100644
index 0000000000..cde5b4d74c
--- /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 ffa3378b5e..b1f9e793c1 100644
--- a/support/shell-container.c
+++ b/support/shell-container.c
@@ -37,6 +37,7 @@
 #include <error.h>
 
 #include <support/support.h>
+#include <support/timespec.h>
 
 /* Design considerations
 
@@ -169,6 +170,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;
@@ -179,6 +206,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);
-- 
2.31.1

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

[Adhemerval Zanella Netto]

On 06/03/23 22:52, 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.

LGTM, thanks.  I *think* you might need to resend it with "Signed-off-by:" 
to mark this as a contribution under DCO.  I don't have access to the 
FSF records, Florian might help me (sorry to not have it checked earlier).

> ---
>  stdlib/tst-system.c          | 26 +++++++++++++++++++
>  support/Makefile             |  2 ++
>  support/dtotimespec-time64.c | 27 +++++++++++++++++++
>  support/dtotimespec.c        | 50 ++++++++++++++++++++++++++++++++++++
>  support/shell-container.c    | 28 ++++++++++++++++++++
>  support/timespec.h           |  4 +++
>  sysdeps/posix/system.c       |  6 ++---
>  7 files changed, 140 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..47a0afe6bf 100644
> --- a/stdlib/tst-system.c
> +++ b/stdlib/tst-system.c
> @@ -25,6 +25,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;
> @@ -71,6 +72,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 +169,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 d52c472755..05b31159ea 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 \
> 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
> diff --git a/support/dtotimespec.c b/support/dtotimespec.c
> new file mode 100644
> index 0000000000..cde5b4d74c
> --- /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 ffa3378b5e..b1f9e793c1 100644
> --- a/support/shell-container.c
> +++ b/support/shell-container.c
> @@ -37,6 +37,7 @@
>  #include <error.h>
>  
>  #include <support/support.h>
> +#include <support/timespec.h>
>  
>  /* Design considerations
>  
> @@ -169,6 +170,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;
> @@ -179,6 +206,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);

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

[Adam Yi]

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>
---
 stdlib/tst-system.c          | 26 +++++++++++++++++++
 support/Makefile             |  2 ++
 support/dtotimespec-time64.c | 27 +++++++++++++++++++
 support/dtotimespec.c        | 50 ++++++++++++++++++++++++++++++++++++
 support/shell-container.c    | 28 ++++++++++++++++++++
 support/timespec.h           |  4 +++
 sysdeps/posix/system.c       |  6 ++---
 7 files changed, 140 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..47a0afe6bf 100644
--- a/stdlib/tst-system.c
+++ b/stdlib/tst-system.c
@@ -25,6 +25,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;
@@ -71,6 +72,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 +169,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 d52c472755..05b31159ea 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 \
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
diff --git a/support/dtotimespec.c b/support/dtotimespec.c
new file mode 100644
index 0000000000..cde5b4d74c
--- /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 ffa3378b5e..b1f9e793c1 100644
--- a/support/shell-container.c
+++ b/support/shell-container.c
@@ -37,6 +37,7 @@
 #include <error.h>
 
 #include <support/support.h>
+#include <support/timespec.h>
 
 /* Design considerations
 
@@ -169,6 +170,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;
@@ -179,6 +206,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);
-- 
2.31.1

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

[Florian Weimer]

* Adhemerval Zanella Netto:

> On 06/03/23 22:52, 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.
>
> LGTM, thanks.  I *think* you might need to resend it with "Signed-off-by:" 
> to mark this as a contribution under DCO.  I don't have access to the 
> FSF records, Florian might help me (sorry to not have it checked
> earlier).

Sorry, I don't have direct access to FSF records.  I'm told that people
hang out on OFTC IRC who have, though.

Thanks,
Florian

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

[Adam Yi]

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

I haven't signed any FSF paperwork. I'll resend the patch with DCO signoff.
Thanks!

On Tue, Mar 7, 2023 at 8:33 PM Florian Weimer <fweimer@redhat.com> wrote:

> * Adhemerval Zanella Netto:
>
> > On 06/03/23 22:52, 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.
> >
> > LGTM, thanks.  I *think* you might need to resend it with
> "Signed-off-by:"
> > to mark this as a contribution under DCO.  I don't have access to the
> > FSF records, Florian might help me (sorry to not have it checked
> > earlier).
>
> Sorry, I don't have direct access to FSF records.  I'm told that people
> hang out on OFTC IRC who have, though.
>
> Thanks,
> Florian
>
>

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

[Adhemerval Zanella Netto]

On 07/03/23 09:30, 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.
> 
> Signed-off-by: Adam Yi <ayi@janestreet.com>

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  stdlib/tst-system.c          | 26 +++++++++++++++++++
>  support/Makefile             |  2 ++
>  support/dtotimespec-time64.c | 27 +++++++++++++++++++
>  support/dtotimespec.c        | 50 ++++++++++++++++++++++++++++++++++++
>  support/shell-container.c    | 28 ++++++++++++++++++++
>  support/timespec.h           |  4 +++
>  sysdeps/posix/system.c       |  6 ++---
>  7 files changed, 140 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..47a0afe6bf 100644
> --- a/stdlib/tst-system.c
> +++ b/stdlib/tst-system.c
> @@ -25,6 +25,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;
> @@ -71,6 +72,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 +169,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 d52c472755..05b31159ea 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 \
> 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
> diff --git a/support/dtotimespec.c b/support/dtotimespec.c
> new file mode 100644
> index 0000000000..cde5b4d74c
> --- /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 ffa3378b5e..b1f9e793c1 100644
> --- a/support/shell-container.c
> +++ b/support/shell-container.c
> @@ -37,6 +37,7 @@
>  #include <error.h>
>  
>  #include <support/support.h>
> +#include <support/timespec.h>
>  
>  /* Design considerations
>  
> @@ -169,6 +170,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;
> @@ -179,6 +206,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);

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

[Joseph Myers]

This breaks the testsuite build for Hurd (tst-system needs to link with 
$(shared-thread-library), I think).

/scratch/jmyers/glibc-bot/install/compilers/i686-gnu/lib/gcc/i686-glibc-gnu/12.2.1/../../../../i686-glibc-gnu/bin/ld: /scratch/jmyers/glibc-bot/build/glibcs/i686-gnu/glibc/support/libsupport_nonshared.a(xpthread_create.oS): in function `xpthread_create':
/scratch/jmyers/glibc-bot/src/glibc/support/xpthread_create.c:26: undefined reference to `pthread_create'
/scratch/jmyers/glibc-bot/install/compilers/i686-gnu/lib/gcc/i686-glibc-gnu/12.2.1/../../../../i686-glibc-gnu/bin/ld: /scratch/jmyers/glibc-bot/build/glibcs/i686-gnu/glibc/support/libsupport_nonshared.a(xpthread_join.oS): in function `xpthread_join':
/scratch/jmyers/glibc-bot/src/glibc/support/xpthread_join.c:25: undefined reference to `pthread_join'
collect2: error: ld returned 1 exit status
make[3]: *** [../Rules:238: /scratch/jmyers/glibc-bot/build/glibcs/i686-gnu/glibc/stdlib/tst-system] Error 1

https://sourceware.org/pipermail/libc-testresults/2023q1/010979.html

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Adam Yi]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

Thanks for reporting this! Confirmed that linking with
$(shared-thread-library) fixes the problem. I'll send a patch.

On Wed, Mar 8, 2023 at 6:50 AM Joseph Myers <joseph@codesourcery.com> wrote:

> This breaks the testsuite build for Hurd (tst-system needs to link with
> $(shared-thread-library), I think).
>
> /scratch/jmyers/glibc-bot/install/compilers/i686-gnu/lib/gcc/i686-glibc-gnu/12.2.1/../../../../i686-glibc-gnu/bin/ld:
> /scratch/jmyers/glibc-bot/build/glibcs/i686-gnu/glibc/support/libsupport_nonshared.a(xpthread_create.oS):
> in function `xpthread_create':
> /scratch/jmyers/glibc-bot/src/glibc/support/xpthread_create.c:26:
> undefined reference to `pthread_create'
> /scratch/jmyers/glibc-bot/install/compilers/i686-gnu/lib/gcc/i686-glibc-gnu/12.2.1/../../../../i686-glibc-gnu/bin/ld:
> /scratch/jmyers/glibc-bot/build/glibcs/i686-gnu/glibc/support/libsupport_nonshared.a(xpthread_join.oS):
> in function `xpthread_join':
> /scratch/jmyers/glibc-bot/src/glibc/support/xpthread_join.c:25: undefined
> reference to `pthread_join'
> collect2: error: ld returned 1 exit status
> make[3]: *** [../Rules:238:
> /scratch/jmyers/glibc-bot/build/glibcs/i686-gnu/glibc/stdlib/tst-system]
> Error 1
>
> https://sourceware.org/pipermail/libc-testresults/2023q1/010979.html
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
>