Don't block SIGCHILD when system() is called concurrently

Resolves: #2176707

Note: Upstream patch used nanosleep() to support fractional seconds
in the "shell" sleep command.  This would have required a long
dependency chain of time_t related changes to be brought in.  Instead,
I modified the test to use integer sleep values and integer sleep().
I confirmed that the modified test still detected the bug without
the patch, and passes with it.

Future tests that use the test-container sleep shell command, and
require fractional sleep periods, will either need to be modified
similarly or deal with the nanosleep patch set.
This commit is contained in:
DJ Delorie 2023-07-07 18:44:30 -04:00
parent a9c42b37f5
commit 54aeb61503
3 changed files with 177 additions and 1 deletions

144
glibc-rh2176707-1.patch Normal file
View File

@ -0,0 +1,144 @@
From 436a604b7dc741fc76b5a6704c6cd8bb178518e7 Mon Sep 17 00:00:00 2001
From: Adam Yi <ayi@janestreet.com>
Date: Tue, 7 Mar 2023 07:30:02 -0500
Subject: posix: Fix system blocks SIGCHLD erroneously [BZ #30163]
Fix bug that SIGCHLD is erroneously blocked forever in the following
scenario:
1. Thread A calls system but hasn't returned yet
2. Thread B calls another system but returns
SIGCHLD would be blocked forever in thread B after its system() returns,
even after the system() in thread A returns.
Although POSIX does not require, glibc system implementation aims to be
thread and cancellation safe. This bug was introduced in
5fb7fc96350575c9adb1316833e48ca11553be49 when we moved reverting signal
mask to happen when the last concurrently running system returns,
despite that signal mask is per thread. This commit reverts this logic
and adds a test.
Signed-off-by: Adam Yi <ayi@janestreet.com>
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
[DJ: Edited to use integer sleep() instead of nanosleep() dependency rabbit hole]
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) { 2 });
+ pthread_t short_sleep_thread = xpthread_create (NULL,
+ sleep_and_check_sigchld,
+ &(double) { 1 });
+ xpthread_join (short_sleep_thread);
+ xpthread_join (long_sleep_thread);
+ }
+
TEST_COMPARE (system (""), 0);
return 0;
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
@@ -169,6 +170,31 @@ 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;
+ long sec = strtol (argv[0], &endptr, 0);
+ if (endptr == argv[0] || errno == ERANGE || sec < 0)
+ {
+ fprintf (stderr, "sleep: invalid time interval '%s'\n", argv[0]);
+ return 1;
+ }
+ if (sleep (sec) < 0)
+ {
+ fprintf (stderr, "sleep: failed to nanosleep\n");
+ 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/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);

27
glibc-rh2176707-2.patch Normal file
View File

@ -0,0 +1,27 @@
From d03094649d39949a30513bf3ffb03a28fecbccd8 Mon Sep 17 00:00:00 2001
From: Adam Yi <ayi@janestreet.com>
Date: Wed, 8 Mar 2023 03:11:47 -0500
Subject: hurd: fix build of tst-system.c
We made tst-system.c depend on pthread, but that requires linking with
$(shared-thread-library). It does not fail under Linux because the
variable expands to nothing under Linux, but it fails for Hurd.
I tested verified via cross-compiling that "make check" now works
for Hurd.
Signed-off-by: Adam Yi <ayi@janestreet.com>
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
[DJ: Edited for RHEL 8]
diff -rup a/stdlib/Makefile b/stdlib/Makefile
--- a/stdlib/Makefile 2023-07-07 00:44:55.810981644 -0400
+++ b/stdlib/Makefile 2023-07-07 00:46:47.541411091 -0400
@@ -102,6 +102,7 @@ LDLIBS-test-atexit-race = $(shared-threa
LDLIBS-test-at_quick_exit-race = $(shared-thread-library)
LDLIBS-test-cxa_atexit-race = $(shared-thread-library)
LDLIBS-test-on_exit-race = $(shared-thread-library)
+LDLIBS-tst-system = $(shared-thread-library)
LDLIBS-test-dlclose-exit-race = $(shared-thread-library) $(libdl)
LDFLAGS-test-dlclose-exit-race = $(LDFLAGS-rdynamic)

View File

@ -1,6 +1,6 @@
%define glibcsrcdir glibc-2.28
%define glibcversion 2.28
%define glibcrelease 229%{?dist}
%define glibcrelease 230%{?dist}
# Pre-release tarballs are pulled in from git using a command that is
# effectively:
#
@ -1038,6 +1038,8 @@ Patch845: glibc-rh2180155-1.patch
Patch846: glibc-rh2180155-2.patch
Patch847: glibc-rh2180155-3.patch
Patch848: glibc-rh2213909.patch
Patch849: glibc-rh2176707-1.patch
Patch850: glibc-rh2176707-2.patch
##############################################################################
# Continued list of core "glibc" package information:
@ -2868,6 +2870,9 @@ fi
%files -f compat-libpthread-nonshared.filelist -n compat-libpthread-nonshared
%changelog
* Fri Jul 7 2023 DJ Delorie <dj@redhat.com> - 2.28-230
- Don't block SIGCHILD when system() is called concurrently (#2176707)
* Mon Jul 3 2023 DJ Delorie <dj@redhat.com> - 2.28-229
- resolv_conf: release lock on allocation failure (#2213909)