Message ID | CAHwrs-DiqV0SsfHj+B6+PgUJ=6af5bR6+QrYXS0A+yjFoE0SXQ@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | posix: Fix system blocks SIGCHLD erroneously [BZ #30163] | expand |
Looks like some spaces/tabs got mixed up in the email. Please use the attachment instead. On Sat, Feb 25, 2023 at 1:31 AM Adam Yi <ayi@janestreet.com> wrote: > > Fix bug that SIGCHLD is erroneously blocked forever in the following > scenario: > > 1. Thread A calls system but hasn't returned yet > 2. Thread B calls another system but returns > > SIGCHLD would be blocked forever in thread B after its system() returns, > even after the system() in thread A returns. > > Although POSIX does not require, glibc system implementation aims to be > thread and cancellation safe. This bug was introduced in > 5fb7fc96350575c9adb1316833e48ca11553be49 when we moved reverting signal > mask to happen when the last concurrently running system returns, > despite that signal mask is per thread. This commit reverts this logic > and adds a test. > --- > stdlib/tst-system.c | 27 +++++++++++++++++++++++++++ > support/shell-container.c | 10 ++++++++++ > sysdeps/posix/system.c | 6 +++--- > 3 files changed, 40 insertions(+), 3 deletions(-) > > diff --git a/stdlib/tst-system.c b/stdlib/tst-system.c > index 634acfe264..be95c2de44 100644 > --- a/stdlib/tst-system.c > +++ b/stdlib/tst-system.c > @@ -20,6 +20,8 @@ > #include <string.h> > #include <signal.h> > #include <paths.h> > +#include <pthread.h> > +#include <inttypes.h> > > #include <support/capture_subprocess.h> > #include <support/check.h> > @@ -71,6 +73,19 @@ call_system (void *closure) > } > } > > +static void * > +sleep_and_check_sigchld (void *seconds) > +{ > + char cmd[namemax]; > + sprintf(cmd, "sleep %" PRIdPTR, (intptr_t) seconds); > + TEST_COMPARE (system (cmd), 0); > + > + sigset_t blocked = {0}; > + TEST_COMPARE (sigprocmask (SIG_BLOCK, NULL, &blocked), 0); > + TEST_COMPARE (sigismember (&blocked, SIGCHLD), 0); > + return NULL; > +} > + > static int > do_test (void) > { > @@ -154,6 +169,18 @@ do_test (void) > xchmod (_PATH_BSHELL, st.st_mode); > } > > + { > + pthread_t long_sleep_thread, short_sleep_thread; > + > + TEST_COMPARE (pthread_create (&long_sleep_thread, NULL, > + sleep_and_check_sigchld, (void *) 2), 0); > + TEST_COMPARE (pthread_create (&short_sleep_thread, NULL, > + sleep_and_check_sigchld, (void *) 1), 0); > + > + TEST_COMPARE (pthread_join (short_sleep_thread, NULL), 0); > + TEST_COMPARE (pthread_join (long_sleep_thread, NULL), 0); > + } > + > TEST_COMPARE (system (""), 0); > > return 0; > diff --git a/support/shell-container.c b/support/shell-container.c > index e9ac9b6d04..083426550b 100644 > --- a/support/shell-container.c > +++ b/support/shell-container.c > @@ -171,6 +171,15 @@ kill_func (char **argv) > return 0; > } > > +/* Emulate the "/bin/sleep" command. Options are ignored. */ > +static int > +sleep_func (char **argv) > +{ > + int secs = atoi (argv[0]); > + sleep (secs); > + return 0; > +} > + > /* This is a list of all the built-in commands we understand. */ > static struct { > const char *name; > @@ -181,6 +190,7 @@ static struct { > { "cp", copy_func }, > { "exit", exit_func }, > { "kill", kill_func }, > + { "sleep", sleep_func }, > { NULL, NULL } > }; > > diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c > index 2335a99184..d77720a625 100644 > --- a/sysdeps/posix/system.c > +++ b/sysdeps/posix/system.c > @@ -179,16 +179,16 @@ do_system (const char *line) > as if the shell had terminated using _exit(127). */ > status = W_EXITCODE (127, 0); > > + /* sigaction can not fail with SIGINT/SIGQUIT used with old > + disposition. Same applies for sigprocmask. */ > DO_LOCK (); > if (SUB_REF () == 0) > { > - /* sigaction can not fail with SIGINT/SIGQUIT used with old > - disposition. Same applies for sigprocmask. */ > __sigaction (SIGINT, &intr, NULL); > __sigaction (SIGQUIT, &quit, NULL); > - __sigprocmask (SIG_SETMASK, &omask, NULL); > } > DO_UNLOCK (); > + __sigprocmask (SIG_SETMASK, &omask, NULL); > > if (ret != 0) > __set_errno (ret); > -- > 2.31.1
On 24/02/23 15:00, Adam Yi wrote: > Looks like some spaces/tabs got mixed up in the email. Please use the > attachment instead. Next time just send a v2, it is better to keep track with you patchwork instance [1]. [1] https://patchwork.sourceware.org/project/glibc/list/ > > On Sat, Feb 25, 2023 at 1:31 AM Adam Yi <ayi@janestreet.com> wrote: >> >> Fix bug that SIGCHLD is erroneously blocked forever in the following >> scenario: >> >> 1. Thread A calls system but hasn't returned yet >> 2. Thread B calls another system but returns >> >> SIGCHLD would be blocked forever in thread B after its system() returns, >> even after the system() in thread A returns. >> >> Although POSIX does not require, glibc system implementation aims to be >> thread and cancellation safe. This bug was introduced in >> 5fb7fc96350575c9adb1316833e48ca11553be49 when we moved reverting signal >> mask to happen when the last concurrently running system returns, >> despite that signal mask is per thread. This commit reverts this logic >> and adds a test. Patch looks ok, I would recommend a v3 to enhance the testing a bit. >> --- >> stdlib/tst-system.c | 27 +++++++++++++++++++++++++++ >> support/shell-container.c | 10 ++++++++++ >> sysdeps/posix/system.c | 6 +++--- >> 3 files changed, 40 insertions(+), 3 deletions(-) >> >> diff --git a/stdlib/tst-system.c b/stdlib/tst-system.c >> index 634acfe264..be95c2de44 100644 >> --- a/stdlib/tst-system.c >> +++ b/stdlib/tst-system.c >> @@ -20,6 +20,8 @@ >> #include <string.h> >> #include <signal.h> >> #include <paths.h> >> +#include <pthread.h> >> +#include <inttypes.h> >> >> #include <support/capture_subprocess.h> >> #include <support/check.h> >> @@ -71,6 +73,19 @@ call_system (void *closure) >> } >> } >> >> +static void * >> +sleep_and_check_sigchld (void *seconds) >> +{ >> + char cmd[namemax]; >> + sprintf(cmd, "sleep %" PRIdPTR, (intptr_t) seconds); >> + TEST_COMPARE (system (cmd), 0); >> + >> + sigset_t blocked = {0}; >> + TEST_COMPARE (sigprocmask (SIG_BLOCK, NULL, &blocked), 0); >> + TEST_COMPARE (sigismember (&blocked, SIGCHLD), 0); >> + return NULL; >> +} >> + >> static int >> do_test (void) >> { >> @@ -154,6 +169,18 @@ do_test (void) >> xchmod (_PATH_BSHELL, st.st_mode); >> } >> >> + { >> + pthread_t long_sleep_thread, short_sleep_thread; >> + >> + TEST_COMPARE (pthread_create (&long_sleep_thread, NULL, >> + sleep_and_check_sigchld, (void *) 2), 0); >> + TEST_COMPARE (pthread_create (&short_sleep_thread, NULL, >> + sleep_and_check_sigchld, (void *) 1), 0); >> + >> + TEST_COMPARE (pthread_join (short_sleep_thread, NULL), 0); >> + TEST_COMPARE (pthread_join (long_sleep_thread, NULL), 0); >> + } >> + >> TEST_COMPARE (system (""), 0); >> >> return 0; Do we really need to sleep for 2 seconds here? It adds more latency on testcase run. >> diff --git a/support/shell-container.c b/support/shell-container.c >> index e9ac9b6d04..083426550b 100644 >> --- a/support/shell-container.c >> +++ b/support/shell-container.c >> @@ -171,6 +171,15 @@ kill_func (char **argv) >> return 0; >> } >> >> +/* Emulate the "/bin/sleep" command. Options are ignored. */ >> +static int >> +sleep_func (char **argv) >> +{ >> + int secs = atoi (argv[0]); I think it would be better add support for split second, similar to coreutils. Something like: #include <support/check.h> #include <support/timespec.h> /* I shamelessly copied it from gnulib, move it to support/dtotimespec.c. */ struct timespec dtotimespec (double sec) { if (! (TYPE_MINIMUM (time_t) < sec)) return make_timespec (TYPE_MINIMUM (time_t), 0); else if (! (sec < 1.0 + TYPE_MAXIMUM (time_t))) return make_timespec (TYPE_MAXIMUM (time_t), TIMESPEC_HZ - 1); else { time_t s = sec; double frac = TIMESPEC_HZ * (sec - s); long ns = frac; ns += ns < frac; s += ns / TIMESPEC_HZ; ns %= TIMESPEC_HZ; if (ns < 0) { s--; ns += TIMESPEC_HZ; } return make_timespec (s, ns); } } static int sleep_func (char **argv) { TEST_VERIFY_EXIT (argv[0] != NULL); char *endptr = NULL; double sec = strtod (argv[0], &endptr); TEST_VERIFY_EXIT (endptr != argv[0] && errno != ERANGE); /* No suffix support. */ TEST_VERIFY_EXIT (sec >= 0.0); struct timespec ts = dtotimespec (sec); /* We don't expect any signal here. */ TEST_VERIFY_EXIT (nanosleep (&ts, NULL) == 0); } >> + sleep (secs); >> + return 0; >> +} >> + >> /* This is a list of all the built-in commands we understand. */ >> static struct { >> const char *name; >> @@ -181,6 +190,7 @@ static struct { >> { "cp", copy_func }, >> { "exit", exit_func }, >> { "kill", kill_func }, >> + { "sleep", sleep_func }, >> { NULL, NULL } >> }; >> >> diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c >> index 2335a99184..d77720a625 100644 >> --- a/sysdeps/posix/system.c >> +++ b/sysdeps/posix/system.c >> @@ -179,16 +179,16 @@ do_system (const char *line) >> as if the shell had terminated using _exit(127). */ >> status = W_EXITCODE (127, 0); >> >> + /* sigaction can not fail with SIGINT/SIGQUIT used with old >> + disposition. Same applies for sigprocmask. */ >> DO_LOCK (); >> if (SUB_REF () == 0) >> { >> - /* sigaction can not fail with SIGINT/SIGQUIT used with old >> - disposition. Same applies for sigprocmask. */ >> __sigaction (SIGINT, &intr, NULL); >> __sigaction (SIGQUIT, &quit, NULL); >> - __sigprocmask (SIG_SETMASK, &omask, NULL); >> } >> DO_UNLOCK (); >> + __sigprocmask (SIG_SETMASK, &omask, NULL); >> >> if (ret != 0) >> __set_errno (ret); Ok, thanks for spotting it. >> -- >> 2.31.1
On Sat, Feb 25, 2023 at 4:18 AM Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote: > > > > On 24/02/23 15:00, Adam Yi wrote: > > Looks like some spaces/tabs got mixed up in the email. Please use the > > attachment instead. > > Next time just send a v2, it is better to keep track with you patchwork > instance [1]. > > [1] https://patchwork.sourceware.org/project/glibc/list/ Got it, thanks! > > > > > On Sat, Feb 25, 2023 at 1:31 AM Adam Yi <ayi@janestreet.com> wrote: > >> > >> Fix bug that SIGCHLD is erroneously blocked forever in the following > >> scenario: > >> > >> 1. Thread A calls system but hasn't returned yet > >> 2. Thread B calls another system but returns > >> > >> SIGCHLD would be blocked forever in thread B after its system() returns, > >> even after the system() in thread A returns. > >> > >> Although POSIX does not require, glibc system implementation aims to be > >> thread and cancellation safe. This bug was introduced in > >> 5fb7fc96350575c9adb1316833e48ca11553be49 when we moved reverting signal > >> mask to happen when the last concurrently running system returns, > >> despite that signal mask is per thread. This commit reverts this logic > >> and adds a test. > > Patch looks ok, I would recommend a v3 to enhance the testing a bit. Thanks for the suggestion! I've sent over a v3 to add split second support. > > >> --- > >> stdlib/tst-system.c | 27 +++++++++++++++++++++++++++ > >> support/shell-container.c | 10 ++++++++++ > >> sysdeps/posix/system.c | 6 +++--- > >> 3 files changed, 40 insertions(+), 3 deletions(-) > >> > >> diff --git a/stdlib/tst-system.c b/stdlib/tst-system.c > >> index 634acfe264..be95c2de44 100644 > >> --- a/stdlib/tst-system.c > >> +++ b/stdlib/tst-system.c > >> @@ -20,6 +20,8 @@ > >> #include <string.h> > >> #include <signal.h> > >> #include <paths.h> > >> +#include <pthread.h> > >> +#include <inttypes.h> > >> > >> #include <support/capture_subprocess.h> > >> #include <support/check.h> > >> @@ -71,6 +73,19 @@ call_system (void *closure) > >> } > >> } > >> > >> +static void * > >> +sleep_and_check_sigchld (void *seconds) > >> +{ > >> + char cmd[namemax]; > >> + sprintf(cmd, "sleep %" PRIdPTR, (intptr_t) seconds); > >> + TEST_COMPARE (system (cmd), 0); > >> + > >> + sigset_t blocked = {0}; > >> + TEST_COMPARE (sigprocmask (SIG_BLOCK, NULL, &blocked), 0); > >> + TEST_COMPARE (sigismember (&blocked, SIGCHLD), 0); > >> + return NULL; > >> +} > >> + > >> static int > >> do_test (void) > >> { > >> @@ -154,6 +169,18 @@ do_test (void) > >> xchmod (_PATH_BSHELL, st.st_mode); > >> } > >> > >> + { > >> + pthread_t long_sleep_thread, short_sleep_thread; > >> + > >> + TEST_COMPARE (pthread_create (&long_sleep_thread, NULL, > >> + sleep_and_check_sigchld, (void *) 2), 0); > >> + TEST_COMPARE (pthread_create (&short_sleep_thread, NULL, > >> + sleep_and_check_sigchld, (void *) 1), 0); > >> + > >> + TEST_COMPARE (pthread_join (short_sleep_thread, NULL), 0); > >> + TEST_COMPARE (pthread_join (long_sleep_thread, NULL), 0); > >> + } > >> + > >> TEST_COMPARE (system (""), 0); > >> > >> return 0; > > Do we really need to sleep for 2 seconds here? It adds more latency on > testcase run. > > >> diff --git a/support/shell-container.c b/support/shell-container.c > >> index e9ac9b6d04..083426550b 100644 > >> --- a/support/shell-container.c > >> +++ b/support/shell-container.c > >> @@ -171,6 +171,15 @@ kill_func (char **argv) > >> return 0; > >> } > >> > >> +/* Emulate the "/bin/sleep" command. Options are ignored. */ > >> +static int > >> +sleep_func (char **argv) > >> +{ > >> + int secs = atoi (argv[0]); > > I think it would be better add support for split second, similar to > coreutils. Something like: > > #include <support/check.h> > #include <support/timespec.h> > > /* I shamelessly copied it from gnulib, move it to support/dtotimespec.c. */ > struct timespec > dtotimespec (double sec) > { > if (! (TYPE_MINIMUM (time_t) < sec)) > return make_timespec (TYPE_MINIMUM (time_t), 0); > else if (! (sec < 1.0 + TYPE_MAXIMUM (time_t))) > return make_timespec (TYPE_MAXIMUM (time_t), TIMESPEC_HZ - 1); > else > { > time_t s = sec; > double frac = TIMESPEC_HZ * (sec - s); > long ns = frac; > ns += ns < frac; > s += ns / TIMESPEC_HZ; > ns %= TIMESPEC_HZ; > > if (ns < 0) > { > s--; > ns += TIMESPEC_HZ; > } > > return make_timespec (s, ns); > } > } > > static int > sleep_func (char **argv) > { > TEST_VERIFY_EXIT (argv[0] != NULL); > char *endptr = NULL; > double sec = strtod (argv[0], &endptr); > TEST_VERIFY_EXIT (endptr != argv[0] && errno != ERANGE); > /* No suffix support. */ > TEST_VERIFY_EXIT (sec >= 0.0); > > struct timespec ts = dtotimespec (sec); > /* We don't expect any signal here. */ > TEST_VERIFY_EXIT (nanosleep (&ts, NULL) == 0); > } > > >> + sleep (secs); > >> + return 0; > >> +} > >> + > >> /* This is a list of all the built-in commands we understand. */ > >> static struct { > >> const char *name; > >> @@ -181,6 +190,7 @@ static struct { > >> { "cp", copy_func }, > >> { "exit", exit_func }, > >> { "kill", kill_func }, > >> + { "sleep", sleep_func }, > >> { NULL, NULL } > >> }; > >> > >> diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c > >> index 2335a99184..d77720a625 100644 > >> --- a/sysdeps/posix/system.c > >> +++ b/sysdeps/posix/system.c > >> @@ -179,16 +179,16 @@ do_system (const char *line) > >> as if the shell had terminated using _exit(127). */ > >> status = W_EXITCODE (127, 0); > >> > >> + /* sigaction can not fail with SIGINT/SIGQUIT used with old > >> + disposition. Same applies for sigprocmask. */ > >> DO_LOCK (); > >> if (SUB_REF () == 0) > >> { > >> - /* sigaction can not fail with SIGINT/SIGQUIT used with old > >> - disposition. Same applies for sigprocmask. */ > >> __sigaction (SIGINT, &intr, NULL); > >> __sigaction (SIGQUIT, &quit, NULL); > >> - __sigprocmask (SIG_SETMASK, &omask, NULL); > >> } > >> DO_UNLOCK (); > >> + __sigprocmask (SIG_SETMASK, &omask, NULL); > >> > >> if (ret != 0) > >> __set_errno (ret); > > Ok, thanks for spotting it. > > >> -- > >> 2.31.1
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);