| Message ID | E1pZMYf-007ZrK-1X@igm-qws-u21165a.delacy.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] posix: Fix system blocks SIGCHLD erroneously [BZ #30163] | expand |
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);
* 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
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 > >
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);