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

Message ID 68bde1a2-ba4d-b3b0-389a-bc70a6b23ed5@linaro.org
State Dropped
Headers

Commit Message

Adhemerval Zanella Nov. 30, 2018, 3:11 p.m. UTC
  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).

---
  

Comments

Florian Weimer Nov. 30, 2018, 3:21 p.m. UTC | #1
* Adhemerval Zanella:

> On 29/11/2018 15:37, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> +/* We have to and actually can handle cancelable system().  The big
>>> +   problem: we have to kill the child process if necessary.  To do
>>> +   this a cleanup handler has to be registered and it has to be able
>>> +   to find the PID of the child.  The main problem is to reliable have
>>> +   the PID when needed.  It is not necessary for the parent thread to
>>> +   return.  It might still be in the kernel when the cancellation
>>> +   request comes.  Therefore we have to use the clone() calls ability
>>> +   to have the kernel write the PID into the user-level variable.  */
>> 
>> This comment does not look relevant to me anymore.
>
> I think it still worth to mention glibc system aims to be thread-safe,
> which requires restore the signal dispositions for SIGINT and SIGQUIT 
> correctly and to deal with cancellation by terminating the child process.

> +/* This system implementation aims to be thread-safe, which requires restore
> +   the signal dispositions for SIGINT and SIGQUIT correctly and to deal with
> +   cancellation by terminating the child process.  */

I don't think you restore SIGINT and SIGQUIT correctly for concurrent
system calls.  This is what the ADD_REF code in the old version
attempted to do.

Thnaks,
Florian
  

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..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