[PATCH] posix: Fix system blocks SIGCHLD erroneously [BZ #30163]
Adam Yi
ayi@janestreet.com
Wed Mar 1 02:36:04 GMT 2023
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
More information about the Libc-alpha
mailing list