| Message ID | 20181025174103.31596-2-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] posix: Use posix_spawn on popen | expand |
Ping. On 25/10/2018 14:41, Adhemerval Zanella wrote: > This patch uses posix_spawn on system implementation. On Linux this has > the advantage of much lower memory consumption (usually 32 Kb minimum for > the mmap stack area). > > Although POSIX does not require, glibc system implementation aims to be > thread and cancellation safe. The cancellation code is moved to generic > implementation and enabled iff SIGCANCEL is defined (similar on how the > cancellation handler is enabled on nptl-init.c). > > The _LIBC_REENTRANT related code is removed and the signal handler reset > when cancellation happens is handled all by the cleanup handler (system > is already a AS-unsafe function). > > Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu, > arm-linux-gnueabihf, and powerpc64le-linux-gnu. > > * sysdeps/unix/sysv/linux/spawni.c (__spawni_child): Use > __sigismember instead of sigismember. > * sysdeps/posix/system.c [SIGCANCEL] (cancel_handler_args, > cancel_handler): New definitions. > (CLEANUP_HANDLER, CLEANUP_RESET): Likewise. > (DO_LOCK, DO_UNLOCK, INIT_LOCK, ADD_REF, SUB_REF): Remove. > (do_system): Use posix_spawn instead of fork and execl and remove > reentracy code. > * sysdeps/generic/not-errno.h (__kill_noerrno): New prototype. > * sysdeps/unix/sysv/linux/not-errno.h (__kill_noerrno): Likewise. > * sysdeps/unix/sysv/linux/ia64/system.c: Remove file. > * sysdeps/unix/sysv/linux/s390/system.c: Likewise. > * sysdeps/unix/sysv/linux/sparc/system.c: Likewise. > * sysdeps/unix/sysv/linux/system.c: Likewise. > --- > ChangeLog | 15 ++ > sysdeps/generic/not-errno.h | 2 + > sysdeps/posix/system.c | 201 +++++++++++-------------- > sysdeps/unix/sysv/linux/ia64/system.c | 30 ---- > sysdeps/unix/sysv/linux/not-errno.h | 14 ++ > sysdeps/unix/sysv/linux/s390/system.c | 29 ---- > sysdeps/unix/sysv/linux/sparc/system.c | 29 ---- > sysdeps/unix/sysv/linux/spawni.c | 4 +- > sysdeps/unix/sysv/linux/system.c | 76 ---------- > 9 files changed, 119 insertions(+), 281 deletions(-) > delete mode 100644 sysdeps/unix/sysv/linux/ia64/system.c > delete mode 100644 sysdeps/unix/sysv/linux/s390/system.c > delete mode 100644 sysdeps/unix/sysv/linux/sparc/system.c > delete mode 100644 sysdeps/unix/sysv/linux/system.c > > diff --git a/sysdeps/generic/not-errno.h b/sysdeps/generic/not-errno.h > index 93617a3266..0fd66b5c5e 100644 > --- a/sysdeps/generic/not-errno.h > +++ b/sysdeps/generic/not-errno.h > @@ -17,3 +17,5 @@ > <http://www.gnu.org/licenses/>. */ > > extern __typeof (__access) __access_noerrno attribute_hidden; > + > +extern __typeof (__kill) __kill_noerrno attribute_hidden; > diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c > index d7594436ed..c762f15995 100644 > --- a/sysdeps/posix/system.c > +++ b/sysdeps/posix/system.c > @@ -17,36 +17,63 @@ > > #include <errno.h> > #include <signal.h> > -#include <stddef.h> > #include <stdlib.h> > #include <unistd.h> > +#include <sigsetops.h> > +#include <spawn.h> > +#include <pthread.h> > #include <sys/types.h> > #include <sys/wait.h> > -#include <libc-lock.h> > -#include <sysdep-cancel.h> > -#include <sigsetops.h> > +#include <stdio.h> > > +#include <libc-lock.h> > +#include <not-errno.h> > +#include <internal-signals.h> > > #define SHELL_PATH "/bin/sh" /* Path of the shell. */ > #define SHELL_NAME "sh" /* Name to give it. */ > > +/* We have to and actually can handle cancelable system(). The big > + problem: we have to kill the child process if necessary. To do > + this a cleanup handler has to be registered and it has to be able > + to find the PID of the child. The main problem is to reliable have > + the PID when needed. It is not necessary for the parent thread to > + return. It might still be in the kernel when the cancellation > + request comes. Therefore we have to use the clone() calls ability > + to have the kernel write the PID into the user-level variable. */ > > -#ifdef _LIBC_REENTRANT > -static struct sigaction intr, quit; > -static int sa_refcntr; > -__libc_lock_define_initialized (static, lock); > +#ifdef SIGCANCEL > + > +struct cancel_handler_args > +{ > + struct sigaction *quit; > + struct sigaction *intr; > + pid_t pid; > +}; > + > +static void > +cancel_handler (void *arg) > +{ > + struct cancel_handler_args *args = (struct cancel_handler_args *) (arg); > > -# define DO_LOCK() __libc_lock_lock (lock) > -# define DO_UNLOCK() __libc_lock_unlock (lock) > -# define INIT_LOCK() ({ __libc_lock_init (lock); sa_refcntr = 0; }) > -# define ADD_REF() sa_refcntr++ > -# define SUB_REF() --sa_refcntr > + __kill_noerrno (args->pid, SIGKILL); > + > + TEMP_FAILURE_RETRY (__waitpid (args->pid, NULL, 0)); > + > + __sigaction (SIGQUIT, args->quit, NULL); > + __sigaction (SIGINT, args->intr, NULL); > +} > +# define CLEANUP_HANDLER(q, i, p) \ > + __libc_cleanup_region_start (1, cancel_handler, \ > + &((struct cancel_handler_args) { \ > + .quit = &(q), \ > + .intr = &(i), \ > + .pid = (p) })) > +# define CLEANUP_RESET() \ > + __libc_cleanup_region_end (0) > #else > -# define DO_LOCK() > -# define DO_UNLOCK() > -# define INIT_LOCK() > -# define ADD_REF() 0 > -# define SUB_REF() 0 > +# define CLEANUP_HANDLER(q, i, p) > +# define CLEANUP_RESET() > #endif > > > @@ -54,122 +81,66 @@ __libc_lock_define_initialized (static, lock); > static int > do_system (const char *line) > { > - int status, save; > + int status; > pid_t pid; > struct sigaction sa; > -#ifndef _LIBC_REENTRANT > struct sigaction intr, quit; > -#endif > sigset_t omask; > + sigset_t reset; > > sa.sa_handler = SIG_IGN; > sa.sa_flags = 0; > __sigemptyset (&sa.sa_mask); > > - DO_LOCK (); > - if (ADD_REF () == 0) > - { > - if (__sigaction (SIGINT, &sa, &intr) < 0) > - { > - (void) SUB_REF (); > - goto out; > - } > - if (__sigaction (SIGQUIT, &sa, &quit) < 0) > - { > - save = errno; > - (void) SUB_REF (); > - goto out_restore_sigint; > - } > - } > - DO_UNLOCK (); > + /* sigaction can not fail with SIGINT/SIGQUIT used with SIG_IGN. */ > + __sigaction (SIGINT, &sa, &intr); > + __sigaction (SIGQUIT, &sa, &quit); > > - /* We reuse the bitmap in the 'sa' structure. */ > __sigaddset (&sa.sa_mask, SIGCHLD); > - save = errno; > - if (__sigprocmask (SIG_BLOCK, &sa.sa_mask, &omask) < 0) > - { > -#ifndef _LIBC > - if (errno == ENOSYS) > - __set_errno (save); > - else > -#endif > - { > - DO_LOCK (); > - if (SUB_REF () == 0) > - { > - save = errno; > - (void) __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL); > - out_restore_sigint: > - (void) __sigaction (SIGINT, &intr, (struct sigaction *) NULL); > - __set_errno (save); > - } > - out: > - DO_UNLOCK (); > - return -1; > - } > - } > - > -#ifdef CLEANUP_HANDLER > - CLEANUP_HANDLER; > -#endif > - > -#ifdef FORK > - pid = FORK (); > -#else > - pid = __fork (); > -#endif > - if (pid == (pid_t) 0) > - { > - /* Child side. */ > - const char *new_argv[4]; > - new_argv[0] = SHELL_NAME; > - new_argv[1] = "-c"; > - new_argv[2] = line; > - new_argv[3] = NULL; > - > - /* Restore the signals. */ > - (void) __sigaction (SIGINT, &intr, (struct sigaction *) NULL); > - (void) __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL); > - (void) __sigprocmask (SIG_SETMASK, &omask, (sigset_t *) NULL); > - INIT_LOCK (); > - > - /* Exec the shell. */ > - (void) __execve (SHELL_PATH, (char *const *) new_argv, __environ); > - _exit (127); > - } > - else if (pid < (pid_t) 0) > - /* The fork failed. */ > - status = -1; > - else > - /* Parent side. */ > + /* sigprocmask can not fail with SIG_BLOCK used with valid input > + arguments. */ > + __sigprocmask (SIG_BLOCK, &sa.sa_mask, &omask); > + > + __sigemptyset (&reset); > + if (intr.sa_handler != SIG_IGN) > + __sigaddset(&reset, SIGINT); > + if (quit.sa_handler != SIG_IGN) > + __sigaddset(&reset, SIGQUIT); > + > + posix_spawnattr_t spawn_attr; > + /* None of the posix_spawnattr_* function returns an error, including > + posix_spawnattr_setflags for the follow specific usage (using valid > + flags). */ > + __posix_spawnattr_init (&spawn_attr); > + __posix_spawnattr_setsigmask (&spawn_attr, &omask); > + __posix_spawnattr_setsigdefault (&spawn_attr, &reset); > + __posix_spawnattr_setflags (&spawn_attr, > + POSIX_SPAWN_SETSIGDEF | POSIX_SPAWN_SETSIGMASK); > + > + status = __posix_spawn (&pid, SHELL_PATH, 0, &spawn_attr, > + (char *const[]){ (char*) SHELL_NAME, > + (char*) "-c", > + (char *) line, NULL }, > + __environ); > + __posix_spawnattr_destroy (&spawn_attr); > + > + if (status == 0) > { > + /* Cancellation results in cleanup handlers running as exceptions in > + the block where they were installed, so it is safe to reference > + stack variable allocate in the broader scope. */ > + CLEANUP_HANDLER (quit, intr, pid); > /* Note the system() is a cancellation point. But since we call > waitpid() which itself is a cancellation point we do not > have to do anything here. */ > if (TEMP_FAILURE_RETRY (__waitpid (pid, &status, 0)) != pid) > status = -1; > + CLEANUP_RESET (); > } > > -#ifdef CLEANUP_HANDLER > - CLEANUP_RESET; > -#endif > - > - save = errno; > - DO_LOCK (); > - if ((SUB_REF () == 0 > - && (__sigaction (SIGINT, &intr, (struct sigaction *) NULL) > - | __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL)) != 0) > - || __sigprocmask (SIG_SETMASK, &omask, (sigset_t *) NULL) != 0) > - { > -#ifndef _LIBC > - /* glibc cannot be used on systems without waitpid. */ > - if (errno == ENOSYS) > - __set_errno (save); > - else > -#endif > - status = -1; > - } > - DO_UNLOCK (); > + __sigaction (SIGINT, &intr, NULL); > + __sigaction (SIGQUIT, &quit, NULL); > + __sigprocmask (SIG_SETMASK, &omask, NULL); > > return status; > } > diff --git a/sysdeps/unix/sysv/linux/ia64/system.c b/sysdeps/unix/sysv/linux/ia64/system.c > deleted file mode 100644 > index d09fefefe6..0000000000 > --- a/sysdeps/unix/sysv/linux/ia64/system.c > +++ /dev/null > @@ -1,30 +0,0 @@ > -/* Copyright (C) 2002-2018 Free Software Foundation, Inc. > - This file is part of the GNU C Library. > - > - 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 > - <http://www.gnu.org/licenses/>. */ > - > -/* We have to and actually can handle cancelable system(). The big > - problem: we have to kill the child process if necessary. To do > - this a cleanup handler has to be registered and is has to be able > - to find the PID of the child. The main problem is to reliable have > - the PID when needed. It is not necessary for the parent thread to > - return. It might still be in the kernel when the cancellation > - request comes. Therefore we have to use the clone() calls ability > - to have the kernel write the PID into the user-level variable. */ > -#define FORK() \ > - INLINE_SYSCALL (clone2, 6, CLONE_PARENT_SETTID | SIGCHLD, NULL, 0, \ > - &pid, NULL, NULL) > - > -#include <sysdeps/unix/sysv/linux/system.c> > diff --git a/sysdeps/unix/sysv/linux/not-errno.h b/sysdeps/unix/sysv/linux/not-errno.h > index 106ba5c72e..b2f72cfb3d 100644 > --- a/sysdeps/unix/sysv/linux/not-errno.h > +++ b/sysdeps/unix/sysv/linux/not-errno.h > @@ -16,6 +16,9 @@ > License along with the GNU C Library; if not, see > <http://www.gnu.org/licenses/>. */ > > +#include <sysdep.h> > +#include <fcntl.h> > + > /* This function is used on maybe_enable_malloc_check (elf/dl-tunables.c) > and to avoid having to build/use multiple versions if stack protection > in enabled it is defined as inline. */ > @@ -33,3 +36,14 @@ __access_noerrno (const char *pathname, int mode) > return INTERNAL_SYSCALL_ERRNO (res, err); > return 0; > } > + > +static inline int > +__kill_noerrno (pid_t pid, int sig) > +{ > + int res; > + INTERNAL_SYSCALL_DECL (err); > + res = INTERNAL_SYSCALL_CALL (kill, err, pid, sig); > + if (INTERNAL_SYSCALL_ERROR_P (res, err)) > + return INTERNAL_SYSCALL_ERRNO (res, err); > + return 0; > +} > diff --git a/sysdeps/unix/sysv/linux/s390/system.c b/sysdeps/unix/sysv/linux/s390/system.c > deleted file mode 100644 > index d8ef461334..0000000000 > --- a/sysdeps/unix/sysv/linux/s390/system.c > +++ /dev/null > @@ -1,29 +0,0 @@ > -/* Copyright (C) 2003-2018 Free Software Foundation, Inc. > - This file is part of the GNU C Library. > - > - 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 > - <http://www.gnu.org/licenses/>. */ > - > -/* We have to and actually can handle cancelable system(). The big > - problem: we have to kill the child process if necessary. To do > - this a cleanup handler has to be registered and is has to be able > - to find the PID of the child. The main problem is to reliable have > - the PID when needed. It is not necessary for the parent thread to > - return. It might still be in the kernel when the cancellation > - request comes. Therefore we have to use the clone() calls ability > - to have the kernel write the PID into the user-level variable. */ > -#define FORK() \ > - INLINE_SYSCALL (clone, 3, 0, CLONE_PARENT_SETTID | SIGCHLD, &pid) > - > -#include "../system.c" > diff --git a/sysdeps/unix/sysv/linux/sparc/system.c b/sysdeps/unix/sysv/linux/sparc/system.c > deleted file mode 100644 > index 1f65c83399..0000000000 > --- a/sysdeps/unix/sysv/linux/sparc/system.c > +++ /dev/null > @@ -1,29 +0,0 @@ > -/* Copyright (C) 2003-2018 Free Software Foundation, Inc. > - This file is part of the GNU C Library. > - > - 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 > - <http://www.gnu.org/licenses/>. */ > - > -/* We have to and actually can handle cancelable system(). The big > - problem: we have to kill the child process if necessary. To do > - this a cleanup handler has to be registered and is has to be able > - to find the PID of the child. The main problem is to reliable have > - the PID when needed. It is not necessary for the parent thread to > - return. It might still be in the kernel when the cancellation > - request comes. Therefore we have to use the clone() calls ability > - to have the kernel write the PID into the user-level variable. */ > -#define FORK() \ > - INLINE_CLONE_SYSCALL (CLONE_PARENT_SETTID | SIGCHLD, 0, &pid, NULL, NULL) > - > -#include "../system.c" > diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c > index 85239cedbf..6a8bd2ed2e 100644 > --- a/sysdeps/unix/sysv/linux/spawni.c > +++ b/sysdeps/unix/sysv/linux/spawni.c > @@ -138,11 +138,11 @@ __spawni_child (void *arguments) > for (int sig = 1; sig < _NSIG; ++sig) > { > if ((attr->__flags & POSIX_SPAWN_SETSIGDEF) > - && sigismember (&attr->__sd, sig)) > + && __sigismember (&attr->__sd, sig)) > { > sa.sa_handler = SIG_DFL; > } > - else if (sigismember (&hset, sig)) > + else if (__sigismember (&hset, sig)) > { > if (__is_internal_signal (sig)) > sa.sa_handler = SIG_IGN; > diff --git a/sysdeps/unix/sysv/linux/system.c b/sysdeps/unix/sysv/linux/system.c > deleted file mode 100644 > index 7cc68a1528..0000000000 > --- a/sysdeps/unix/sysv/linux/system.c > +++ /dev/null > @@ -1,76 +0,0 @@ > -/* Copyright (C) 2002-2018 Free Software Foundation, Inc. > - This file is part of the GNU C Library. > - > - 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 > - <http://www.gnu.org/licenses/>. */ > - > -#include <sched.h> > -#include <signal.h> > -#include <string.h> /* For the real memset prototype. */ > -#include <sysdep.h> > -#include <unistd.h> > -#include <sys/wait.h> > -#include <libc-lock.h> > - > -/* We have to and actually can handle cancelable system(). The big > - problem: we have to kill the child process if necessary. To do > - this a cleanup handler has to be registered and is has to be able > - to find the PID of the child. The main problem is to reliable have > - the PID when needed. It is not necessary for the parent thread to > - return. It might still be in the kernel when the cancellation > - request comes. Therefore we have to use the clone() calls ability > - to have the kernel write the PID into the user-level variable. */ > -#ifndef FORK > -# define FORK() \ > - INLINE_SYSCALL (clone, 3, CLONE_PARENT_SETTID | SIGCHLD, 0, &pid) > -#endif > - > -#ifdef _LIBC_REENTRANT > -static void cancel_handler (void *arg); > - > -# define CLEANUP_HANDLER \ > - __libc_cleanup_region_start (1, cancel_handler, &pid) > - > -# define CLEANUP_RESET \ > - __libc_cleanup_region_end (0) > -#endif > - > - > -/* Linux has waitpid(), so override the generic unix version. */ > -#include <sysdeps/posix/system.c> > - > - > -#ifdef _LIBC_REENTRANT > -/* The cancellation handler. */ > -static void > -cancel_handler (void *arg) > -{ > - pid_t child = *(pid_t *) arg; > - > - INTERNAL_SYSCALL_DECL (err); > - INTERNAL_SYSCALL (kill, err, 2, child, SIGKILL); > - > - TEMP_FAILURE_RETRY (__waitpid (child, NULL, 0)); > - > - DO_LOCK (); > - > - if (SUB_REF () == 0) > - { > - (void) __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL); > - (void) __sigaction (SIGINT, &intr, (struct sigaction *) NULL); > - } > - > - DO_UNLOCK (); > -} > -#endif >
* Adhemerval Zanella: > +/* We have to and actually can handle cancelable system(). The big > + problem: we have to kill the child process if necessary. To do > + this a cleanup handler has to be registered and it has to be able > + to find the PID of the child. The main problem is to reliable have > + the PID when needed. It is not necessary for the parent thread to > + return. It might still be in the kernel when the cancellation > + request comes. Therefore we have to use the clone() calls ability > + to have the kernel write the PID into the user-level variable. */ This comment does not look relevant to me anymore. > -#ifdef _LIBC_REENTRANT > -static struct sigaction intr, quit; > -static int sa_refcntr; > -__libc_lock_define_initialized (static, lock); > +#ifdef SIGCANCEL > + > +struct cancel_handler_args > +{ > + struct sigaction *quit; > + struct sigaction *intr; > + pid_t pid; Trailing whitespace on the last line. > +# define CLEANUP_HANDLER(q, i, p) \ > + __libc_cleanup_region_start (1, cancel_handler, \ > + &((struct cancel_handler_args) { \ > + .quit = &(q), \ > + .intr = &(i), \ > + .pid = (p) })) > +# define CLEANUP_RESET() \ > + __libc_cleanup_region_end (0) I don't see the value in those macros, they just hide what is going on. You should also use a named local variable instead of the compound literal. I think it is acceptable to duplicate the actual __waitpid call for the cancellation and non-cancellation cases. Rest looks okay to me. I think we should seriously consider not messing with signals if __libc_multiple_threads, but that is a separate change. Thanks, Florian
On 29/11/2018 15:37, Florian Weimer wrote: > * Adhemerval Zanella: > >> +/* We have to and actually can handle cancelable system(). The big >> + problem: we have to kill the child process if necessary. To do >> + this a cleanup handler has to be registered and it has to be able >> + to find the PID of the child. The main problem is to reliable have >> + the PID when needed. It is not necessary for the parent thread to >> + return. It might still be in the kernel when the cancellation >> + request comes. Therefore we have to use the clone() calls ability >> + to have the kernel write the PID into the user-level variable. */ > > This comment does not look relevant to me anymore. I think it still worth to mention glibc system aims to be thread-safe, which requires restore the signal dispositions for SIGINT and SIGQUIT correctly and to deal with cancellation by terminating the child process. > >> -#ifdef _LIBC_REENTRANT >> -static struct sigaction intr, quit; >> -static int sa_refcntr; >> -__libc_lock_define_initialized (static, lock); >> +#ifdef SIGCANCEL >> + >> +struct cancel_handler_args >> +{ >> + struct sigaction *quit; >> + struct sigaction *intr; >> + pid_t pid; > > Trailing whitespace on the last line. Ack. > >> +# define CLEANUP_HANDLER(q, i, p) \ >> + __libc_cleanup_region_start (1, cancel_handler, \ >> + &((struct cancel_handler_args) { \ >> + .quit = &(q), \ >> + .intr = &(i), \ >> + .pid = (p) })) >> +# define CLEANUP_RESET() \ >> + __libc_cleanup_region_end (0) > > I don't see the value in those macros, they just hide what is going on. > You should also use a named local variable instead of the compound > literal. I think it is acceptable to duplicate the actual __waitpid > call for the cancellation and non-cancellation cases. I don't have a strong preference about the macros, I remove them and used a named local variable. For cleanup handler, I only still only use it if SIGCANCEL is defined because it would require implement __kill_noerrno for hurd (and it won't be used anyway). > > Rest looks okay to me. > > I think we should seriously consider not messing with signals if > __libc_multiple_threads, but that is a separate change. In this case I think we would need to provide a compat system symbol with this new semantic, but system is really problematic (specially in multithreaded environments) so I am not sure if it worth the trouble. Below the updated patch (which I intend to commit). --- diff --git a/sysdeps/generic/not-errno.h b/sysdeps/generic/not-errno.h index 93617a3266..0fd66b5c5e 100644 --- a/sysdeps/generic/not-errno.h +++ b/sysdeps/generic/not-errno.h @@ -17,3 +17,5 @@ <http://www.gnu.org/licenses/>. */ extern __typeof (__access) __access_noerrno attribute_hidden; + +extern __typeof (__kill) __kill_noerrno attribute_hidden; diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c index d7594436ed..6d3f1bba20 100644 --- a/sysdeps/posix/system.c +++ b/sysdeps/posix/system.c @@ -17,159 +17,121 @@ #include <errno.h> #include <signal.h> -#include <stddef.h> #include <stdlib.h> #include <unistd.h> +#include <sigsetops.h> +#include <spawn.h> +#include <pthread.h> #include <sys/types.h> #include <sys/wait.h> -#include <libc-lock.h> -#include <sysdep-cancel.h> -#include <sigsetops.h> +#include <stdio.h> +#include <libc-lock.h> +#include <not-errno.h> +#include <internal-signals.h> #define SHELL_PATH "/bin/sh" /* Path of the shell. */ #define SHELL_NAME "sh" /* Name to give it. */ +#ifdef SIGCANCEL +struct cancel_handler_args +{ + struct sigaction *quit; + struct sigaction *intr; + pid_t pid; +}; -#ifdef _LIBC_REENTRANT -static struct sigaction intr, quit; -static int sa_refcntr; -__libc_lock_define_initialized (static, lock); - -# define DO_LOCK() __libc_lock_lock (lock) -# define DO_UNLOCK() __libc_lock_unlock (lock) -# define INIT_LOCK() ({ __libc_lock_init (lock); sa_refcntr = 0; }) -# define ADD_REF() sa_refcntr++ -# define SUB_REF() --sa_refcntr -#else -# define DO_LOCK() -# define DO_UNLOCK() -# define INIT_LOCK() -# define ADD_REF() 0 -# define SUB_REF() 0 -#endif +/* This system implementation aims to be thread-safe, which requires restore + the signal dispositions for SIGINT and SIGQUIT correctly and to deal with + cancellation by terminating the child process. */ +static void +cancel_handler (void *arg) +{ + struct cancel_handler_args *args = (struct cancel_handler_args *) (arg); + __kill_noerrno (args->pid, SIGKILL); + + TEMP_FAILURE_RETRY (__waitpid (args->pid, NULL, 0)); + + __sigaction (SIGQUIT, args->quit, NULL); + __sigaction (SIGINT, args->intr, NULL); +} +#endif /* Execute LINE as a shell command, returning its status. */ static int do_system (const char *line) { - int status, save; + int status; pid_t pid; struct sigaction sa; -#ifndef _LIBC_REENTRANT struct sigaction intr, quit; -#endif sigset_t omask; + sigset_t reset; sa.sa_handler = SIG_IGN; sa.sa_flags = 0; __sigemptyset (&sa.sa_mask); - DO_LOCK (); - if (ADD_REF () == 0) - { - if (__sigaction (SIGINT, &sa, &intr) < 0) - { - (void) SUB_REF (); - goto out; - } - if (__sigaction (SIGQUIT, &sa, &quit) < 0) - { - save = errno; - (void) SUB_REF (); - goto out_restore_sigint; - } - } - DO_UNLOCK (); + /* sigaction can not fail with SIGINT/SIGQUIT used with SIG_IGN. */ + __sigaction (SIGINT, &sa, &intr); + __sigaction (SIGQUIT, &sa, &quit); - /* We reuse the bitmap in the 'sa' structure. */ __sigaddset (&sa.sa_mask, SIGCHLD); - save = errno; - if (__sigprocmask (SIG_BLOCK, &sa.sa_mask, &omask) < 0) + /* sigprocmask can not fail with SIG_BLOCK used with valid input + arguments. */ + __sigprocmask (SIG_BLOCK, &sa.sa_mask, &omask); + + __sigemptyset (&reset); + if (intr.sa_handler != SIG_IGN) + __sigaddset(&reset, SIGINT); + if (quit.sa_handler != SIG_IGN) + __sigaddset(&reset, SIGQUIT); + + posix_spawnattr_t spawn_attr; + /* None of the posix_spawnattr_* function returns an error, including + posix_spawnattr_setflags for the follow specific usage (using valid + flags). */ + __posix_spawnattr_init (&spawn_attr); + __posix_spawnattr_setsigmask (&spawn_attr, &omask); + __posix_spawnattr_setsigdefault (&spawn_attr, &reset); + __posix_spawnattr_setflags (&spawn_attr, + POSIX_SPAWN_SETSIGDEF | POSIX_SPAWN_SETSIGMASK); + + status = __posix_spawn (&pid, SHELL_PATH, 0, &spawn_attr, + (char *const[]){ (char*) SHELL_NAME, + (char*) "-c", + (char *) line, NULL }, + __environ); + __posix_spawnattr_destroy (&spawn_attr); + + if (status == 0) { -#ifndef _LIBC - if (errno == ENOSYS) - __set_errno (save); - else + /* Cancellation results in cleanup handlers running as exceptions in + the block where they were installed, so it is safe to reference + stack variable allocate in the broader scope. */ +#ifdef SIGCANCEL + struct cancel_handler_args cancel_args = + { + .quit = &quit, + .intr = &intr, + .pid = pid + }; + __libc_cleanup_region_start (1, cancel_handler, &cancel_args); #endif - { - DO_LOCK (); - if (SUB_REF () == 0) - { - save = errno; - (void) __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL); - out_restore_sigint: - (void) __sigaction (SIGINT, &intr, (struct sigaction *) NULL); - __set_errno (save); - } - out: - DO_UNLOCK (); - return -1; - } - } - -#ifdef CLEANUP_HANDLER - CLEANUP_HANDLER; -#endif - -#ifdef FORK - pid = FORK (); -#else - pid = __fork (); -#endif - if (pid == (pid_t) 0) - { - /* Child side. */ - const char *new_argv[4]; - new_argv[0] = SHELL_NAME; - new_argv[1] = "-c"; - new_argv[2] = line; - new_argv[3] = NULL; - - /* Restore the signals. */ - (void) __sigaction (SIGINT, &intr, (struct sigaction *) NULL); - (void) __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL); - (void) __sigprocmask (SIG_SETMASK, &omask, (sigset_t *) NULL); - INIT_LOCK (); - - /* Exec the shell. */ - (void) __execve (SHELL_PATH, (char *const *) new_argv, __environ); - _exit (127); - } - else if (pid < (pid_t) 0) - /* The fork failed. */ - status = -1; - else - /* Parent side. */ - { /* Note the system() is a cancellation point. But since we call waitpid() which itself is a cancellation point we do not have to do anything here. */ if (TEMP_FAILURE_RETRY (__waitpid (pid, &status, 0)) != pid) status = -1; - } - -#ifdef CLEANUP_HANDLER - CLEANUP_RESET; +#ifdef SIGCANCEL + __libc_cleanup_region_end (0); #endif - - save = errno; - DO_LOCK (); - if ((SUB_REF () == 0 - && (__sigaction (SIGINT, &intr, (struct sigaction *) NULL) - | __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL)) != 0) - || __sigprocmask (SIG_SETMASK, &omask, (sigset_t *) NULL) != 0) - { -#ifndef _LIBC - /* glibc cannot be used on systems without waitpid. */ - if (errno == ENOSYS) - __set_errno (save); - else -#endif - status = -1; } - DO_UNLOCK (); + + __sigaction (SIGINT, &intr, NULL); + __sigaction (SIGQUIT, &quit, NULL); + __sigprocmask (SIG_SETMASK, &omask, NULL); return status; } diff --git a/sysdeps/unix/sysv/linux/ia64/system.c b/sysdeps/unix/sysv/linux/ia64/system.c deleted file mode 100644 index d09fefefe6..0000000000 --- a/sysdeps/unix/sysv/linux/ia64/system.c +++ /dev/null @@ -1,30 +0,0 @@ -/* Copyright (C) 2002-2018 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - 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 - <http://www.gnu.org/licenses/>. */ - -/* We have to and actually can handle cancelable system(). The big - problem: we have to kill the child process if necessary. To do - this a cleanup handler has to be registered and is has to be able - to find the PID of the child. The main problem is to reliable have - the PID when needed. It is not necessary for the parent thread to - return. It might still be in the kernel when the cancellation - request comes. Therefore we have to use the clone() calls ability - to have the kernel write the PID into the user-level variable. */ -#define FORK() \ - INLINE_SYSCALL (clone2, 6, CLONE_PARENT_SETTID | SIGCHLD, NULL, 0, \ - &pid, NULL, NULL) - -#include <sysdeps/unix/sysv/linux/system.c> diff --git a/sysdeps/unix/sysv/linux/not-errno.h b/sysdeps/unix/sysv/linux/not-errno.h index 106ba5c72e..b2f72cfb3d 100644 --- a/sysdeps/unix/sysv/linux/not-errno.h +++ b/sysdeps/unix/sysv/linux/not-errno.h @@ -16,6 +16,9 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#include <sysdep.h> +#include <fcntl.h> + /* This function is used on maybe_enable_malloc_check (elf/dl-tunables.c) and to avoid having to build/use multiple versions if stack protection in enabled it is defined as inline. */ @@ -33,3 +36,14 @@ __access_noerrno (const char *pathname, int mode) return INTERNAL_SYSCALL_ERRNO (res, err); return 0; } + +static inline int +__kill_noerrno (pid_t pid, int sig) +{ + int res; + INTERNAL_SYSCALL_DECL (err); + res = INTERNAL_SYSCALL_CALL (kill, err, pid, sig); + if (INTERNAL_SYSCALL_ERROR_P (res, err)) + return INTERNAL_SYSCALL_ERRNO (res, err); + return 0; +} diff --git a/sysdeps/unix/sysv/linux/s390/system.c b/sysdeps/unix/sysv/linux/s390/system.c deleted file mode 100644 index d8ef461334..0000000000 --- a/sysdeps/unix/sysv/linux/s390/system.c +++ /dev/null @@ -1,29 +0,0 @@ -/* Copyright (C) 2003-2018 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - 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 - <http://www.gnu.org/licenses/>. */ - -/* We have to and actually can handle cancelable system(). The big - problem: we have to kill the child process if necessary. To do - this a cleanup handler has to be registered and is has to be able - to find the PID of the child. The main problem is to reliable have - the PID when needed. It is not necessary for the parent thread to - return. It might still be in the kernel when the cancellation - request comes. Therefore we have to use the clone() calls ability - to have the kernel write the PID into the user-level variable. */ -#define FORK() \ - INLINE_SYSCALL (clone, 3, 0, CLONE_PARENT_SETTID | SIGCHLD, &pid) - -#include "../system.c" diff --git a/sysdeps/unix/sysv/linux/sparc/system.c b/sysdeps/unix/sysv/linux/sparc/system.c deleted file mode 100644 index 1f65c83399..0000000000 --- a/sysdeps/unix/sysv/linux/sparc/system.c +++ /dev/null @@ -1,29 +0,0 @@ -/* Copyright (C) 2003-2018 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - 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 - <http://www.gnu.org/licenses/>. */ - -/* We have to and actually can handle cancelable system(). The big - problem: we have to kill the child process if necessary. To do - this a cleanup handler has to be registered and is has to be able - to find the PID of the child. The main problem is to reliable have - the PID when needed. It is not necessary for the parent thread to - return. It might still be in the kernel when the cancellation - request comes. Therefore we have to use the clone() calls ability - to have the kernel write the PID into the user-level variable. */ -#define FORK() \ - INLINE_CLONE_SYSCALL (CLONE_PARENT_SETTID | SIGCHLD, 0, &pid, NULL, NULL) - -#include "../system.c" diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c index 9f3a137b5c..dbb6cdd5f0 100644 --- a/sysdeps/unix/sysv/linux/spawni.c +++ b/sysdeps/unix/sysv/linux/spawni.c @@ -138,11 +138,11 @@ __spawni_child (void *arguments) for (int sig = 1; sig < _NSIG; ++sig) { if ((attr->__flags & POSIX_SPAWN_SETSIGDEF) - && sigismember (&attr->__sd, sig)) + && __sigismember (&attr->__sd, sig)) { sa.sa_handler = SIG_DFL; } - else if (sigismember (&hset, sig)) + else if (__sigismember (&hset, sig)) { if (__is_internal_signal (sig)) sa.sa_handler = SIG_IGN; diff --git a/sysdeps/unix/sysv/linux/system.c b/sysdeps/unix/sysv/linux/system.c deleted file mode 100644 index 7cc68a1528..0000000000 --- a/sysdeps/unix/sysv/linux/system.c +++ /dev/null @@ -1,76 +0,0 @@ -/* Copyright (C) 2002-2018 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - 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 - <http://www.gnu.org/licenses/>. */ - -#include <sched.h> -#include <signal.h> -#include <string.h> /* For the real memset prototype. */ -#include <sysdep.h> -#include <unistd.h> -#include <sys/wait.h> -#include <libc-lock.h> - -/* We have to and actually can handle cancelable system(). The big - problem: we have to kill the child process if necessary. To do - this a cleanup handler has to be registered and is has to be able - to find the PID of the child. The main problem is to reliable have - the PID when needed. It is not necessary for the parent thread to - return. It might still be in the kernel when the cancellation - request comes. Therefore we have to use the clone() calls ability - to have the kernel write the PID into the user-level variable. */ -#ifndef FORK -# define FORK() \ - INLINE_SYSCALL (clone, 3, CLONE_PARENT_SETTID | SIGCHLD, 0, &pid) -#endif - -#ifdef _LIBC_REENTRANT -static void cancel_handler (void *arg); - -# define CLEANUP_HANDLER \ - __libc_cleanup_region_start (1, cancel_handler, &pid) - -# define CLEANUP_RESET \ - __libc_cleanup_region_end (0) -#endif - - -/* Linux has waitpid(), so override the generic unix version. */ -#include <sysdeps/posix/system.c> - - -#ifdef _LIBC_REENTRANT -/* The cancellation handler. */ -static void -cancel_handler (void *arg) -{ - pid_t child = *(pid_t *) arg; - - INTERNAL_SYSCALL_DECL (err); - INTERNAL_SYSCALL (kill, err, 2, child, SIGKILL); - - TEMP_FAILURE_RETRY (__waitpid (child, NULL, 0)); - - DO_LOCK (); - - if (SUB_REF () == 0) - { - (void) __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL); - (void) __sigaction (SIGINT, &intr, (struct sigaction *) NULL); - } - - DO_UNLOCK (); -} -#endif -- 2.17.1
* Adhemerval Zanella: > On 29/11/2018 15:37, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> +/* We have to and actually can handle cancelable system(). The big >>> + problem: we have to kill the child process if necessary. To do >>> + this a cleanup handler has to be registered and it has to be able >>> + to find the PID of the child. The main problem is to reliable have >>> + the PID when needed. It is not necessary for the parent thread to >>> + return. It might still be in the kernel when the cancellation >>> + request comes. Therefore we have to use the clone() calls ability >>> + to have the kernel write the PID into the user-level variable. */ >> >> This comment does not look relevant to me anymore. > > I think it still worth to mention glibc system aims to be thread-safe, > which requires restore the signal dispositions for SIGINT and SIGQUIT > correctly and to deal with cancellation by terminating the child process. > +/* This system implementation aims to be thread-safe, which requires restore > + the signal dispositions for SIGINT and SIGQUIT correctly and to deal with > + cancellation by terminating the child process. */ I don't think you restore SIGINT and SIGQUIT correctly for concurrent system calls. This is what the ADD_REF code in the old version attempted to do. Thnaks, Florian
On 30/11/2018 13:21, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 29/11/2018 15:37, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> +/* We have to and actually can handle cancelable system(). The big >>>> + problem: we have to kill the child process if necessary. To do >>>> + this a cleanup handler has to be registered and it has to be able >>>> + to find the PID of the child. The main problem is to reliable have >>>> + the PID when needed. It is not necessary for the parent thread to >>>> + return. It might still be in the kernel when the cancellation >>>> + request comes. Therefore we have to use the clone() calls ability >>>> + to have the kernel write the PID into the user-level variable. */ >>> >>> This comment does not look relevant to me anymore. >> >> I think it still worth to mention glibc system aims to be thread-safe, >> which requires restore the signal dispositions for SIGINT and SIGQUIT >> correctly and to deal with cancellation by terminating the child process. > >> +/* This system implementation aims to be thread-safe, which requires restore >> + the signal dispositions for SIGINT and SIGQUIT correctly and to deal with >> + cancellation by terminating the child process. */ > > I don't think you restore SIGINT and SIGQUIT correctly for concurrent > system calls. This is what the ADD_REF code in the old version > attempted to do. It is not strictly incorrect, although Linux sigaction is not really thread-safe (due the copy in/out kernel sigaction structure). And it is indeed not optional, and I agree that relying on this benign data race behaviour is not correct. Below it is an updated patch with ref counter reinstated. --- diff --git a/sysdeps/generic/not-errno.h b/sysdeps/generic/not-errno.h index 93617a3266..0fd66b5c5e 100644 --- a/sysdeps/generic/not-errno.h +++ b/sysdeps/generic/not-errno.h @@ -17,3 +17,5 @@ <http://www.gnu.org/licenses/>. */ extern __typeof (__access) __access_noerrno attribute_hidden; + +extern __typeof (__kill) __kill_noerrno attribute_hidden; diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c index d7594436ed..76e67373dc 100644 --- a/sysdeps/posix/system.c +++ b/sysdeps/posix/system.c @@ -17,20 +17,35 @@ #include <errno.h> #include <signal.h> -#include <stddef.h> #include <stdlib.h> #include <unistd.h> +#include <sigsetops.h> +#include <spawn.h> +#include <pthread.h> #include <sys/types.h> #include <sys/wait.h> -#include <libc-lock.h> -#include <sysdep-cancel.h> -#include <sigsetops.h> +#include <stdio.h> +#include <libc-lock.h> +#include <not-errno.h> +#include <internal-signals.h> #define SHELL_PATH "/bin/sh" /* Path of the shell. */ #define SHELL_NAME "sh" /* Name to give it. */ +/* This system implementation aims to be thread-safe, which requires to + restore the signal dispositions for SIGINT and SIGQUIT correctly and to + deal with cancellation by terminating the child process. + + The signal disposition restoration on the single-thread case is + straighfoward. For multithreaded case, a reference-counter with a lock + is used, so the first thread will set the SIGINT/SIGQUIT dispositions and + last thread will restore them. + + Cancellation handling is done with thread cancellation clean-up handlers + on waitpid call. */ + #ifdef _LIBC_REENTRANT static struct sigaction intr, quit; static int sa_refcntr; @@ -50,17 +65,45 @@ __libc_lock_define_initialized (static, lock); #endif +#if defined(_LIBC_REENTRANT) && defined(SIGCANCEL) +struct cancel_handler_args +{ + struct sigaction *quit; + struct sigaction *intr; + pid_t pid; +}; + +static void +cancel_handler (void *arg) +{ + struct cancel_handler_args *args = (struct cancel_handler_args *) (arg); + + __kill_noerrno (args->pid, SIGKILL); + + TEMP_FAILURE_RETRY (__waitpid (args->pid, NULL, 0)); + + DO_LOCK (); + if (SUB_REF () == 0) + { + __sigaction (SIGQUIT, args->quit, NULL); + __sigaction (SIGINT, args->intr, NULL); + } + DO_UNLOCK (); +} +#endif + /* Execute LINE as a shell command, returning its status. */ static int do_system (const char *line) { - int status, save; + int status; pid_t pid; struct sigaction sa; #ifndef _LIBC_REENTRANT struct sigaction intr, quit; #endif sigset_t omask; + sigset_t reset; sa.sa_handler = SIG_IGN; sa.sa_flags = 0; @@ -69,105 +112,72 @@ do_system (const char *line) DO_LOCK (); if (ADD_REF () == 0) { - if (__sigaction (SIGINT, &sa, &intr) < 0) - { - (void) SUB_REF (); - goto out; - } - if (__sigaction (SIGQUIT, &sa, &quit) < 0) - { - save = errno; - (void) SUB_REF (); - goto out_restore_sigint; - } + /* sigaction can not fail with SIGINT/SIGQUIT used with SIG_IGN. */ + __sigaction (SIGINT, &sa, &intr); + __sigaction (SIGQUIT, &sa, &quit); } DO_UNLOCK (); - /* We reuse the bitmap in the 'sa' structure. */ __sigaddset (&sa.sa_mask, SIGCHLD); - save = errno; - if (__sigprocmask (SIG_BLOCK, &sa.sa_mask, &omask) < 0) + /* sigprocmask can not fail with SIG_BLOCK used with valid input + arguments. */ + __sigprocmask (SIG_BLOCK, &sa.sa_mask, &omask); + + __sigemptyset (&reset); + if (intr.sa_handler != SIG_IGN) + __sigaddset(&reset, SIGINT); + if (quit.sa_handler != SIG_IGN) + __sigaddset(&reset, SIGQUIT); + + posix_spawnattr_t spawn_attr; + /* None of the posix_spawnattr_* function returns an error, including + posix_spawnattr_setflags for the follow specific usage (using valid + flags). */ + __posix_spawnattr_init (&spawn_attr); + __posix_spawnattr_setsigmask (&spawn_attr, &omask); + __posix_spawnattr_setsigdefault (&spawn_attr, &reset); + __posix_spawnattr_setflags (&spawn_attr, + POSIX_SPAWN_SETSIGDEF | POSIX_SPAWN_SETSIGMASK); + + status = __posix_spawn (&pid, SHELL_PATH, 0, &spawn_attr, + (char *const[]){ (char*) SHELL_NAME, + (char*) "-c", + (char *) line, NULL }, + __environ); + __posix_spawnattr_destroy (&spawn_attr); + + if (status == 0) { -#ifndef _LIBC - if (errno == ENOSYS) - __set_errno (save); - else -#endif - { - DO_LOCK (); - if (SUB_REF () == 0) - { - save = errno; - (void) __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL); - out_restore_sigint: - (void) __sigaction (SIGINT, &intr, (struct sigaction *) NULL); - __set_errno (save); - } - out: - DO_UNLOCK (); - return -1; - } - } - -#ifdef CLEANUP_HANDLER - CLEANUP_HANDLER; -#endif - -#ifdef FORK - pid = FORK (); -#else - pid = __fork (); + /* Cancellation results in cleanup handlers running as exceptions in + the block where they were installed, so it is safe to reference + stack variable allocate in the broader scope. */ +#if defined(_LIBC_REENTRANT) && defined(SIGCANCEL) + struct cancel_handler_args cancel_args = + { + .quit = &quit, + .intr = &intr, + .pid = pid + }; + __libc_cleanup_region_start (1, cancel_handler, &cancel_args); #endif - if (pid == (pid_t) 0) - { - /* Child side. */ - const char *new_argv[4]; - new_argv[0] = SHELL_NAME; - new_argv[1] = "-c"; - new_argv[2] = line; - new_argv[3] = NULL; - - /* Restore the signals. */ - (void) __sigaction (SIGINT, &intr, (struct sigaction *) NULL); - (void) __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL); - (void) __sigprocmask (SIG_SETMASK, &omask, (sigset_t *) NULL); - INIT_LOCK (); - - /* Exec the shell. */ - (void) __execve (SHELL_PATH, (char *const *) new_argv, __environ); - _exit (127); - } - else if (pid < (pid_t) 0) - /* The fork failed. */ - status = -1; - else - /* Parent side. */ - { /* Note the system() is a cancellation point. But since we call waitpid() which itself is a cancellation point we do not have to do anything here. */ if (TEMP_FAILURE_RETRY (__waitpid (pid, &status, 0)) != pid) status = -1; - } - -#ifdef CLEANUP_HANDLER - CLEANUP_RESET; +#if defined(_LIBC_REENTRANT) && defined(SIGCANCEL) + __libc_cleanup_region_end (0); #endif + } - save = errno; DO_LOCK (); - if ((SUB_REF () == 0 - && (__sigaction (SIGINT, &intr, (struct sigaction *) NULL) - | __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL)) != 0) - || __sigprocmask (SIG_SETMASK, &omask, (sigset_t *) NULL) != 0) + if (SUB_REF () == 0) { -#ifndef _LIBC - /* glibc cannot be used on systems without waitpid. */ - if (errno == ENOSYS) - __set_errno (save); - else -#endif - status = -1; + /* 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 (); diff --git a/sysdeps/unix/sysv/linux/ia64/system.c b/sysdeps/unix/sysv/linux/ia64/system.c deleted file mode 100644 index d09fefefe6..0000000000 --- a/sysdeps/unix/sysv/linux/ia64/system.c +++ /dev/null @@ -1,30 +0,0 @@ -/* Copyright (C) 2002-2018 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - 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 - <http://www.gnu.org/licenses/>. */ - -/* We have to and actually can handle cancelable system(). The big - problem: we have to kill the child process if necessary. To do - this a cleanup handler has to be registered and is has to be able - to find the PID of the child. The main problem is to reliable have - the PID when needed. It is not necessary for the parent thread to - return. It might still be in the kernel when the cancellation - request comes. Therefore we have to use the clone() calls ability - to have the kernel write the PID into the user-level variable. */ -#define FORK() \ - INLINE_SYSCALL (clone2, 6, CLONE_PARENT_SETTID | SIGCHLD, NULL, 0, \ - &pid, NULL, NULL) - -#include <sysdeps/unix/sysv/linux/system.c> diff --git a/sysdeps/unix/sysv/linux/not-errno.h b/sysdeps/unix/sysv/linux/not-errno.h index 106ba5c72e..b2f72cfb3d 100644 --- a/sysdeps/unix/sysv/linux/not-errno.h +++ b/sysdeps/unix/sysv/linux/not-errno.h @@ -16,6 +16,9 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#include <sysdep.h> +#include <fcntl.h> + /* This function is used on maybe_enable_malloc_check (elf/dl-tunables.c) and to avoid having to build/use multiple versions if stack protection in enabled it is defined as inline. */ @@ -33,3 +36,14 @@ __access_noerrno (const char *pathname, int mode) return INTERNAL_SYSCALL_ERRNO (res, err); return 0; } + +static inline int +__kill_noerrno (pid_t pid, int sig) +{ + int res; + INTERNAL_SYSCALL_DECL (err); + res = INTERNAL_SYSCALL_CALL (kill, err, pid, sig); + if (INTERNAL_SYSCALL_ERROR_P (res, err)) + return INTERNAL_SYSCALL_ERRNO (res, err); + return 0; +} diff --git a/sysdeps/unix/sysv/linux/s390/system.c b/sysdeps/unix/sysv/linux/s390/system.c deleted file mode 100644 index d8ef461334..0000000000 --- a/sysdeps/unix/sysv/linux/s390/system.c +++ /dev/null @@ -1,29 +0,0 @@ -/* Copyright (C) 2003-2018 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - 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 - <http://www.gnu.org/licenses/>. */ - -/* We have to and actually can handle cancelable system(). The big - problem: we have to kill the child process if necessary. To do - this a cleanup handler has to be registered and is has to be able - to find the PID of the child. The main problem is to reliable have - the PID when needed. It is not necessary for the parent thread to - return. It might still be in the kernel when the cancellation - request comes. Therefore we have to use the clone() calls ability - to have the kernel write the PID into the user-level variable. */ -#define FORK() \ - INLINE_SYSCALL (clone, 3, 0, CLONE_PARENT_SETTID | SIGCHLD, &pid) - -#include "../system.c" diff --git a/sysdeps/unix/sysv/linux/sparc/system.c b/sysdeps/unix/sysv/linux/sparc/system.c deleted file mode 100644 index 1f65c83399..0000000000 --- a/sysdeps/unix/sysv/linux/sparc/system.c +++ /dev/null @@ -1,29 +0,0 @@ -/* Copyright (C) 2003-2018 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - 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 - <http://www.gnu.org/licenses/>. */ - -/* We have to and actually can handle cancelable system(). The big - problem: we have to kill the child process if necessary. To do - this a cleanup handler has to be registered and is has to be able - to find the PID of the child. The main problem is to reliable have - the PID when needed. It is not necessary for the parent thread to - return. It might still be in the kernel when the cancellation - request comes. Therefore we have to use the clone() calls ability - to have the kernel write the PID into the user-level variable. */ -#define FORK() \ - INLINE_CLONE_SYSCALL (CLONE_PARENT_SETTID | SIGCHLD, 0, &pid, NULL, NULL) - -#include "../system.c" diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c index 9f3a137b5c..dbb6cdd5f0 100644 --- a/sysdeps/unix/sysv/linux/spawni.c +++ b/sysdeps/unix/sysv/linux/spawni.c @@ -138,11 +138,11 @@ __spawni_child (void *arguments) for (int sig = 1; sig < _NSIG; ++sig) { if ((attr->__flags & POSIX_SPAWN_SETSIGDEF) - && sigismember (&attr->__sd, sig)) + && __sigismember (&attr->__sd, sig)) { sa.sa_handler = SIG_DFL; } - else if (sigismember (&hset, sig)) + else if (__sigismember (&hset, sig)) { if (__is_internal_signal (sig)) sa.sa_handler = SIG_IGN; diff --git a/sysdeps/unix/sysv/linux/system.c b/sysdeps/unix/sysv/linux/system.c deleted file mode 100644 index 7cc68a1528..0000000000 --- a/sysdeps/unix/sysv/linux/system.c +++ /dev/null @@ -1,76 +0,0 @@ -/* Copyright (C) 2002-2018 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - 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 - <http://www.gnu.org/licenses/>. */ - -#include <sched.h> -#include <signal.h> -#include <string.h> /* For the real memset prototype. */ -#include <sysdep.h> -#include <unistd.h> -#include <sys/wait.h> -#include <libc-lock.h> - -/* We have to and actually can handle cancelable system(). The big - problem: we have to kill the child process if necessary. To do - this a cleanup handler has to be registered and is has to be able - to find the PID of the child. The main problem is to reliable have - the PID when needed. It is not necessary for the parent thread to - return. It might still be in the kernel when the cancellation - request comes. Therefore we have to use the clone() calls ability - to have the kernel write the PID into the user-level variable. */ -#ifndef FORK -# define FORK() \ - INLINE_SYSCALL (clone, 3, CLONE_PARENT_SETTID | SIGCHLD, 0, &pid) -#endif - -#ifdef _LIBC_REENTRANT -static void cancel_handler (void *arg); - -# define CLEANUP_HANDLER \ - __libc_cleanup_region_start (1, cancel_handler, &pid) - -# define CLEANUP_RESET \ - __libc_cleanup_region_end (0) -#endif - - -/* Linux has waitpid(), so override the generic unix version. */ -#include <sysdeps/posix/system.c> - - -#ifdef _LIBC_REENTRANT -/* The cancellation handler. */ -static void -cancel_handler (void *arg) -{ - pid_t child = *(pid_t *) arg; - - INTERNAL_SYSCALL_DECL (err); - INTERNAL_SYSCALL (kill, err, 2, child, SIGKILL); - - TEMP_FAILURE_RETRY (__waitpid (child, NULL, 0)); - - DO_LOCK (); - - if (SUB_REF () == 0) - { - (void) __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL); - (void) __sigaction (SIGINT, &intr, (struct sigaction *) NULL); - } - - DO_UNLOCK (); -} -#endif -- 2.17.1
* Adhemerval Zanella: > On 30/11/2018 13:21, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> On 29/11/2018 15:37, Florian Weimer wrote: >>>> * Adhemerval Zanella: >>>> >>>>> +/* We have to and actually can handle cancelable system(). The big >>>>> + problem: we have to kill the child process if necessary. To do >>>>> + this a cleanup handler has to be registered and it has to be able >>>>> + to find the PID of the child. The main problem is to reliable have >>>>> + the PID when needed. It is not necessary for the parent thread to >>>>> + return. It might still be in the kernel when the cancellation >>>>> + request comes. Therefore we have to use the clone() calls ability >>>>> + to have the kernel write the PID into the user-level variable. */ >>>> >>>> This comment does not look relevant to me anymore. >>> >>> I think it still worth to mention glibc system aims to be thread-safe, >>> which requires restore the signal dispositions for SIGINT and SIGQUIT >>> correctly and to deal with cancellation by terminating the child process. >> >>> +/* This system implementation aims to be thread-safe, which requires restore >>> + the signal dispositions for SIGINT and SIGQUIT correctly and to deal with >>> + cancellation by terminating the child process. */ >> >> I don't think you restore SIGINT and SIGQUIT correctly for concurrent >> system calls. This is what the ADD_REF code in the old version >> attempted to do. > > It is not strictly incorrect, although Linux sigaction is not really > thread-safe (due the copy in/out kernel sigaction structure). And it > is indeed not optional, and I agree that relying on this benign data race > behaviour is not correct. Below it is an updated patch with ref counter > reinstated. It's not about the data race, it is about the higher-level race condition. The problem is that the first thread to enter system and capture the original signal state may not be the last to leave system and restore things. > +static void > +cancel_handler (void *arg) > +{ > + struct cancel_handler_args *args = (struct cancel_handler_args *) (arg); > + > + __kill_noerrno (args->pid, SIGKILL); > + > + TEMP_FAILURE_RETRY (__waitpid (args->pid, NULL, 0)); One last question (I promise): Should this be the nocancel variant? Thanks, Florian
On 30/11/2018 16:34, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 30/11/2018 13:21, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> On 29/11/2018 15:37, Florian Weimer wrote: >>>>> * Adhemerval Zanella: >>>>> >>>>>> +/* We have to and actually can handle cancelable system(). The big >>>>>> + problem: we have to kill the child process if necessary. To do >>>>>> + this a cleanup handler has to be registered and it has to be able >>>>>> + to find the PID of the child. The main problem is to reliable have >>>>>> + the PID when needed. It is not necessary for the parent thread to >>>>>> + return. It might still be in the kernel when the cancellation >>>>>> + request comes. Therefore we have to use the clone() calls ability >>>>>> + to have the kernel write the PID into the user-level variable. */ >>>>> >>>>> This comment does not look relevant to me anymore. >>>> >>>> I think it still worth to mention glibc system aims to be thread-safe, >>>> which requires restore the signal dispositions for SIGINT and SIGQUIT >>>> correctly and to deal with cancellation by terminating the child process. >>> >>>> +/* This system implementation aims to be thread-safe, which requires restore >>>> + the signal dispositions for SIGINT and SIGQUIT correctly and to deal with >>>> + cancellation by terminating the child process. */ >>> >>> I don't think you restore SIGINT and SIGQUIT correctly for concurrent >>> system calls. This is what the ADD_REF code in the old version >>> attempted to do. >> >> It is not strictly incorrect, although Linux sigaction is not really >> thread-safe (due the copy in/out kernel sigaction structure). And it >> is indeed not optional, and I agree that relying on this benign data race >> behaviour is not correct. Below it is an updated patch with ref counter >> reinstated. > > It's not about the data race, it is about the higher-level race > condition. The problem is that the first thread to enter system and > capture the original signal state may not be the last to leave system > and restore things. Even with current implementation the program would need to coordinate sigaction with system, so I am not sure which would be best option (try to get some sanity on concurrent system or let the program handle it). > >> +static void >> +cancel_handler (void *arg) >> +{ >> + struct cancel_handler_args *args = (struct cancel_handler_args *) (arg); >> + >> + __kill_noerrno (args->pid, SIGKILL); >> + >> + TEMP_FAILURE_RETRY (__waitpid (args->pid, NULL, 0)); > > One last question (I promise): Should this be the nocancel variant? The cancel_handler will be called from within the sigcancel_handler (through unwind mechanism) and SIGCANCEL is not installed SA_NODEFER, so I can't see how another cancellation can act. However for consistency using __waitpid_nocancel does make sense. I changed it locally.
* Adhemerval Zanella: > On 30/11/2018 16:34, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> On 30/11/2018 13:21, Florian Weimer wrote: >>>> * Adhemerval Zanella: >>>> >>>>> On 29/11/2018 15:37, Florian Weimer wrote: >>>>>> * Adhemerval Zanella: >>>>>> >>>>>>> +/* We have to and actually can handle cancelable system(). The big >>>>>>> + problem: we have to kill the child process if necessary. To do >>>>>>> + this a cleanup handler has to be registered and it has to be able >>>>>>> + to find the PID of the child. The main problem is to reliable have >>>>>>> + the PID when needed. It is not necessary for the parent thread to >>>>>>> + return. It might still be in the kernel when the cancellation >>>>>>> + request comes. Therefore we have to use the clone() calls ability >>>>>>> + to have the kernel write the PID into the user-level variable. */ >>>>>> >>>>>> This comment does not look relevant to me anymore. >>>>> >>>>> I think it still worth to mention glibc system aims to be thread-safe, >>>>> which requires restore the signal dispositions for SIGINT and SIGQUIT >>>>> correctly and to deal with cancellation by terminating the child process. >>>> >>>>> +/* This system implementation aims to be thread-safe, which requires restore >>>>> + the signal dispositions for SIGINT and SIGQUIT correctly and to deal with >>>>> + cancellation by terminating the child process. */ >>>> >>>> I don't think you restore SIGINT and SIGQUIT correctly for concurrent >>>> system calls. This is what the ADD_REF code in the old version >>>> attempted to do. >>> >>> It is not strictly incorrect, although Linux sigaction is not really >>> thread-safe (due the copy in/out kernel sigaction structure). And it >>> is indeed not optional, and I agree that relying on this benign data race >>> behaviour is not correct. Below it is an updated patch with ref counter >>> reinstated. >> >> It's not about the data race, it is about the higher-level race >> condition. The problem is that the first thread to enter system and >> capture the original signal state may not be the last to leave system >> and restore things. > > Even with current implementation the program would need to coordinate > sigaction with system, so I am not sure which would be best option > (try to get some sanity on concurrent system or let the program > handle it). The common case where sigaction is called once at process start would be okay, though. >>> +static void >>> +cancel_handler (void *arg) >>> +{ >>> + struct cancel_handler_args *args = (struct cancel_handler_args *) (arg); >>> + >>> + __kill_noerrno (args->pid, SIGKILL); >>> + >>> + TEMP_FAILURE_RETRY (__waitpid (args->pid, NULL, 0)); >> >> One last question (I promise): Should this be the nocancel variant? > > The cancel_handler will be called from within the sigcancel_handler > (through unwind mechanism) and SIGCANCEL is not installed SA_NODEFER, > so I can't see how another cancellation can act. However for consistency > using __waitpid_nocancel does make sense. I changed it locally. Thanks! I don't have any further comments on your patch. It is probably okay to commit. Florian
diff --git a/sysdeps/generic/not-errno.h b/sysdeps/generic/not-errno.h index 93617a3266..0fd66b5c5e 100644 --- a/sysdeps/generic/not-errno.h +++ b/sysdeps/generic/not-errno.h @@ -17,3 +17,5 @@ <http://www.gnu.org/licenses/>. */ extern __typeof (__access) __access_noerrno attribute_hidden; + +extern __typeof (__kill) __kill_noerrno attribute_hidden; diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c index d7594436ed..c762f15995 100644 --- a/sysdeps/posix/system.c +++ b/sysdeps/posix/system.c @@ -17,36 +17,63 @@ #include <errno.h> #include <signal.h> -#include <stddef.h> #include <stdlib.h> #include <unistd.h> +#include <sigsetops.h> +#include <spawn.h> +#include <pthread.h> #include <sys/types.h> #include <sys/wait.h> -#include <libc-lock.h> -#include <sysdep-cancel.h> -#include <sigsetops.h> +#include <stdio.h> +#include <libc-lock.h> +#include <not-errno.h> +#include <internal-signals.h> #define SHELL_PATH "/bin/sh" /* Path of the shell. */ #define SHELL_NAME "sh" /* Name to give it. */ +/* We have to and actually can handle cancelable system(). The big + problem: we have to kill the child process if necessary. To do + this a cleanup handler has to be registered and it has to be able + to find the PID of the child. The main problem is to reliable have + the PID when needed. It is not necessary for the parent thread to + return. It might still be in the kernel when the cancellation + request comes. Therefore we have to use the clone() calls ability + to have the kernel write the PID into the user-level variable. */ -#ifdef _LIBC_REENTRANT -static struct sigaction intr, quit; -static int sa_refcntr; -__libc_lock_define_initialized (static, lock); +#ifdef SIGCANCEL + +struct cancel_handler_args +{ + struct sigaction *quit; + struct sigaction *intr; + pid_t pid; +}; + +static void +cancel_handler (void *arg) +{ + struct cancel_handler_args *args = (struct cancel_handler_args *) (arg); -# define DO_LOCK() __libc_lock_lock (lock) -# define DO_UNLOCK() __libc_lock_unlock (lock) -# define INIT_LOCK() ({ __libc_lock_init (lock); sa_refcntr = 0; }) -# define ADD_REF() sa_refcntr++ -# define SUB_REF() --sa_refcntr + __kill_noerrno (args->pid, SIGKILL); + + TEMP_FAILURE_RETRY (__waitpid (args->pid, NULL, 0)); + + __sigaction (SIGQUIT, args->quit, NULL); + __sigaction (SIGINT, args->intr, NULL); +} +# define CLEANUP_HANDLER(q, i, p) \ + __libc_cleanup_region_start (1, cancel_handler, \ + &((struct cancel_handler_args) { \ + .quit = &(q), \ + .intr = &(i), \ + .pid = (p) })) +# define CLEANUP_RESET() \ + __libc_cleanup_region_end (0) #else -# define DO_LOCK() -# define DO_UNLOCK() -# define INIT_LOCK() -# define ADD_REF() 0 -# define SUB_REF() 0 +# define CLEANUP_HANDLER(q, i, p) +# define CLEANUP_RESET() #endif @@ -54,122 +81,66 @@ __libc_lock_define_initialized (static, lock); static int do_system (const char *line) { - int status, save; + int status; pid_t pid; struct sigaction sa; -#ifndef _LIBC_REENTRANT struct sigaction intr, quit; -#endif sigset_t omask; + sigset_t reset; sa.sa_handler = SIG_IGN; sa.sa_flags = 0; __sigemptyset (&sa.sa_mask); - DO_LOCK (); - if (ADD_REF () == 0) - { - if (__sigaction (SIGINT, &sa, &intr) < 0) - { - (void) SUB_REF (); - goto out; - } - if (__sigaction (SIGQUIT, &sa, &quit) < 0) - { - save = errno; - (void) SUB_REF (); - goto out_restore_sigint; - } - } - DO_UNLOCK (); + /* sigaction can not fail with SIGINT/SIGQUIT used with SIG_IGN. */ + __sigaction (SIGINT, &sa, &intr); + __sigaction (SIGQUIT, &sa, &quit); - /* We reuse the bitmap in the 'sa' structure. */ __sigaddset (&sa.sa_mask, SIGCHLD); - save = errno; - if (__sigprocmask (SIG_BLOCK, &sa.sa_mask, &omask) < 0) - { -#ifndef _LIBC - if (errno == ENOSYS) - __set_errno (save); - else -#endif - { - DO_LOCK (); - if (SUB_REF () == 0) - { - save = errno; - (void) __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL); - out_restore_sigint: - (void) __sigaction (SIGINT, &intr, (struct sigaction *) NULL); - __set_errno (save); - } - out: - DO_UNLOCK (); - return -1; - } - } - -#ifdef CLEANUP_HANDLER - CLEANUP_HANDLER; -#endif - -#ifdef FORK - pid = FORK (); -#else - pid = __fork (); -#endif - if (pid == (pid_t) 0) - { - /* Child side. */ - const char *new_argv[4]; - new_argv[0] = SHELL_NAME; - new_argv[1] = "-c"; - new_argv[2] = line; - new_argv[3] = NULL; - - /* Restore the signals. */ - (void) __sigaction (SIGINT, &intr, (struct sigaction *) NULL); - (void) __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL); - (void) __sigprocmask (SIG_SETMASK, &omask, (sigset_t *) NULL); - INIT_LOCK (); - - /* Exec the shell. */ - (void) __execve (SHELL_PATH, (char *const *) new_argv, __environ); - _exit (127); - } - else if (pid < (pid_t) 0) - /* The fork failed. */ - status = -1; - else - /* Parent side. */ + /* sigprocmask can not fail with SIG_BLOCK used with valid input + arguments. */ + __sigprocmask (SIG_BLOCK, &sa.sa_mask, &omask); + + __sigemptyset (&reset); + if (intr.sa_handler != SIG_IGN) + __sigaddset(&reset, SIGINT); + if (quit.sa_handler != SIG_IGN) + __sigaddset(&reset, SIGQUIT); + + posix_spawnattr_t spawn_attr; + /* None of the posix_spawnattr_* function returns an error, including + posix_spawnattr_setflags for the follow specific usage (using valid + flags). */ + __posix_spawnattr_init (&spawn_attr); + __posix_spawnattr_setsigmask (&spawn_attr, &omask); + __posix_spawnattr_setsigdefault (&spawn_attr, &reset); + __posix_spawnattr_setflags (&spawn_attr, + POSIX_SPAWN_SETSIGDEF | POSIX_SPAWN_SETSIGMASK); + + status = __posix_spawn (&pid, SHELL_PATH, 0, &spawn_attr, + (char *const[]){ (char*) SHELL_NAME, + (char*) "-c", + (char *) line, NULL }, + __environ); + __posix_spawnattr_destroy (&spawn_attr); + + if (status == 0) { + /* Cancellation results in cleanup handlers running as exceptions in + the block where they were installed, so it is safe to reference + stack variable allocate in the broader scope. */ + CLEANUP_HANDLER (quit, intr, pid); /* Note the system() is a cancellation point. But since we call waitpid() which itself is a cancellation point we do not have to do anything here. */ if (TEMP_FAILURE_RETRY (__waitpid (pid, &status, 0)) != pid) status = -1; + CLEANUP_RESET (); } -#ifdef CLEANUP_HANDLER - CLEANUP_RESET; -#endif - - save = errno; - DO_LOCK (); - if ((SUB_REF () == 0 - && (__sigaction (SIGINT, &intr, (struct sigaction *) NULL) - | __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL)) != 0) - || __sigprocmask (SIG_SETMASK, &omask, (sigset_t *) NULL) != 0) - { -#ifndef _LIBC - /* glibc cannot be used on systems without waitpid. */ - if (errno == ENOSYS) - __set_errno (save); - else -#endif - status = -1; - } - DO_UNLOCK (); + __sigaction (SIGINT, &intr, NULL); + __sigaction (SIGQUIT, &quit, NULL); + __sigprocmask (SIG_SETMASK, &omask, NULL); return status; } diff --git a/sysdeps/unix/sysv/linux/ia64/system.c b/sysdeps/unix/sysv/linux/ia64/system.c deleted file mode 100644 index d09fefefe6..0000000000 --- a/sysdeps/unix/sysv/linux/ia64/system.c +++ /dev/null @@ -1,30 +0,0 @@ -/* Copyright (C) 2002-2018 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - 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 - <http://www.gnu.org/licenses/>. */ - -/* We have to and actually can handle cancelable system(). The big - problem: we have to kill the child process if necessary. To do - this a cleanup handler has to be registered and is has to be able - to find the PID of the child. The main problem is to reliable have - the PID when needed. It is not necessary for the parent thread to - return. It might still be in the kernel when the cancellation - request comes. Therefore we have to use the clone() calls ability - to have the kernel write the PID into the user-level variable. */ -#define FORK() \ - INLINE_SYSCALL (clone2, 6, CLONE_PARENT_SETTID | SIGCHLD, NULL, 0, \ - &pid, NULL, NULL) - -#include <sysdeps/unix/sysv/linux/system.c> diff --git a/sysdeps/unix/sysv/linux/not-errno.h b/sysdeps/unix/sysv/linux/not-errno.h index 106ba5c72e..b2f72cfb3d 100644 --- a/sysdeps/unix/sysv/linux/not-errno.h +++ b/sysdeps/unix/sysv/linux/not-errno.h @@ -16,6 +16,9 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#include <sysdep.h> +#include <fcntl.h> + /* This function is used on maybe_enable_malloc_check (elf/dl-tunables.c) and to avoid having to build/use multiple versions if stack protection in enabled it is defined as inline. */ @@ -33,3 +36,14 @@ __access_noerrno (const char *pathname, int mode) return INTERNAL_SYSCALL_ERRNO (res, err); return 0; } + +static inline int +__kill_noerrno (pid_t pid, int sig) +{ + int res; + INTERNAL_SYSCALL_DECL (err); + res = INTERNAL_SYSCALL_CALL (kill, err, pid, sig); + if (INTERNAL_SYSCALL_ERROR_P (res, err)) + return INTERNAL_SYSCALL_ERRNO (res, err); + return 0; +} diff --git a/sysdeps/unix/sysv/linux/s390/system.c b/sysdeps/unix/sysv/linux/s390/system.c deleted file mode 100644 index d8ef461334..0000000000 --- a/sysdeps/unix/sysv/linux/s390/system.c +++ /dev/null @@ -1,29 +0,0 @@ -/* Copyright (C) 2003-2018 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - 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 - <http://www.gnu.org/licenses/>. */ - -/* We have to and actually can handle cancelable system(). The big - problem: we have to kill the child process if necessary. To do - this a cleanup handler has to be registered and is has to be able - to find the PID of the child. The main problem is to reliable have - the PID when needed. It is not necessary for the parent thread to - return. It might still be in the kernel when the cancellation - request comes. Therefore we have to use the clone() calls ability - to have the kernel write the PID into the user-level variable. */ -#define FORK() \ - INLINE_SYSCALL (clone, 3, 0, CLONE_PARENT_SETTID | SIGCHLD, &pid) - -#include "../system.c" diff --git a/sysdeps/unix/sysv/linux/sparc/system.c b/sysdeps/unix/sysv/linux/sparc/system.c deleted file mode 100644 index 1f65c83399..0000000000 --- a/sysdeps/unix/sysv/linux/sparc/system.c +++ /dev/null @@ -1,29 +0,0 @@ -/* Copyright (C) 2003-2018 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - 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 - <http://www.gnu.org/licenses/>. */ - -/* We have to and actually can handle cancelable system(). The big - problem: we have to kill the child process if necessary. To do - this a cleanup handler has to be registered and is has to be able - to find the PID of the child. The main problem is to reliable have - the PID when needed. It is not necessary for the parent thread to - return. It might still be in the kernel when the cancellation - request comes. Therefore we have to use the clone() calls ability - to have the kernel write the PID into the user-level variable. */ -#define FORK() \ - INLINE_CLONE_SYSCALL (CLONE_PARENT_SETTID | SIGCHLD, 0, &pid, NULL, NULL) - -#include "../system.c" diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c index 85239cedbf..6a8bd2ed2e 100644 --- a/sysdeps/unix/sysv/linux/spawni.c +++ b/sysdeps/unix/sysv/linux/spawni.c @@ -138,11 +138,11 @@ __spawni_child (void *arguments) for (int sig = 1; sig < _NSIG; ++sig) { if ((attr->__flags & POSIX_SPAWN_SETSIGDEF) - && sigismember (&attr->__sd, sig)) + && __sigismember (&attr->__sd, sig)) { sa.sa_handler = SIG_DFL; } - else if (sigismember (&hset, sig)) + else if (__sigismember (&hset, sig)) { if (__is_internal_signal (sig)) sa.sa_handler = SIG_IGN; diff --git a/sysdeps/unix/sysv/linux/system.c b/sysdeps/unix/sysv/linux/system.c deleted file mode 100644 index 7cc68a1528..0000000000 --- a/sysdeps/unix/sysv/linux/system.c +++ /dev/null @@ -1,76 +0,0 @@ -/* Copyright (C) 2002-2018 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - 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 - <http://www.gnu.org/licenses/>. */ - -#include <sched.h> -#include <signal.h> -#include <string.h> /* For the real memset prototype. */ -#include <sysdep.h> -#include <unistd.h> -#include <sys/wait.h> -#include <libc-lock.h> - -/* We have to and actually can handle cancelable system(). The big - problem: we have to kill the child process if necessary. To do - this a cleanup handler has to be registered and is has to be able - to find the PID of the child. The main problem is to reliable have - the PID when needed. It is not necessary for the parent thread to - return. It might still be in the kernel when the cancellation - request comes. Therefore we have to use the clone() calls ability - to have the kernel write the PID into the user-level variable. */ -#ifndef FORK -# define FORK() \ - INLINE_SYSCALL (clone, 3, CLONE_PARENT_SETTID | SIGCHLD, 0, &pid) -#endif - -#ifdef _LIBC_REENTRANT -static void cancel_handler (void *arg); - -# define CLEANUP_HANDLER \ - __libc_cleanup_region_start (1, cancel_handler, &pid) - -# define CLEANUP_RESET \ - __libc_cleanup_region_end (0) -#endif - - -/* Linux has waitpid(), so override the generic unix version. */ -#include <sysdeps/posix/system.c> - - -#ifdef _LIBC_REENTRANT -/* The cancellation handler. */ -static void -cancel_handler (void *arg) -{ - pid_t child = *(pid_t *) arg; - - INTERNAL_SYSCALL_DECL (err); - INTERNAL_SYSCALL (kill, err, 2, child, SIGKILL); - - TEMP_FAILURE_RETRY (__waitpid (child, NULL, 0)); - - DO_LOCK (); - - if (SUB_REF () == 0) - { - (void) __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL); - (void) __sigaction (SIGINT, &intr, (struct sigaction *) NULL); - } - - DO_UNLOCK (); -} -#endif