[v3,2/2] posix: Use posix_spawn on system

Message ID 20181025174103.31596-2-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • [v3,1/2] posix: Use posix_spawn on popen
Related show

Commit Message

Adhemerval Zanella Oct. 25, 2018, 5:41 p.m.
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

-- 
2.17.1

Comments

Adhemerval Zanella Nov. 27, 2018, 5:40 p.m. | #1
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

>
Florian Weimer Nov. 29, 2018, 5:37 p.m. | #2
* 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
Adhemerval Zanella Nov. 30, 2018, 3:11 p.m. | #3
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
Florian Weimer Nov. 30, 2018, 3:21 p.m. | #4
* 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
Adhemerval Zanella Nov. 30, 2018, 6:25 p.m. | #5
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
Florian Weimer Nov. 30, 2018, 6:34 p.m. | #6
* 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
Adhemerval Zanella Nov. 30, 2018, 7:32 p.m. | #7
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.
Florian Weimer Nov. 30, 2018, 7:35 p.m. | #8
* 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

Patch

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