Re: [PATCH v3 2/2] posix: Use posix_spawn on system

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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
>