[PATCH 2/3] posix: Use posix_spawn on popen

[PATCH 2/3] posix: Use posix_spawn on popen

[Adhemerval Zanella]

This patch uses posix_spawn on popen instead of fork and execl.  On Linux
this has the advantage of much lower memory consumption (usually 32 Kb
minimum for the mmap stack area).

Checked on x86_64-linux-gnu and i686-linux-gnu.

	* libio/iopopen.c (_IO_new_proc_open): use posix_spawn instead of
	fork and execl.
---
 ChangeLog       |  3 ++
 libio/iopopen.c | 97 +++++++++++++++++++++++++++++--------------------
 2 files changed, 61 insertions(+), 39 deletions(-)

diff --git a/libio/iopopen.c b/libio/iopopen.c
index 2eff45b4c8..3cce2e5596 100644
--- a/libio/iopopen.c
+++ b/libio/iopopen.c
@@ -34,7 +34,8 @@
 #include <not-cancel.h>
 #include <sys/types.h>
 #include <sys/wait.h>
-#include <kernel-features.h>
+#include <spawn.h>
+#include <paths.h>
 
 struct _IO_proc_file
 {
@@ -63,9 +64,8 @@ FILE *
 _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
 {
   int read_or_write;
-  int parent_end, child_end;
   int pipe_fds[2];
-  pid_t child_pid;
+  int op;
 
   int do_read = 0;
   int do_write = 0;
@@ -108,59 +108,78 @@ _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
 
   if (do_read)
     {
-      parent_end = pipe_fds[0];
-      child_end = pipe_fds[1];
+      op = 0;
       read_or_write = _IO_NO_WRITES;
     }
   else
     {
-      parent_end = pipe_fds[1];
-      child_end = pipe_fds[0];
+      op = 1;
       read_or_write = _IO_NO_READS;
     }
 
-  ((_IO_proc_file *) fp)->pid = child_pid = __fork ();
-  if (child_pid == 0)
-    {
-      int child_std_end = do_read ? 1 : 0;
-      struct _IO_proc_file *p;
-
-      if (child_end != child_std_end)
-	__dup2 (child_end, child_std_end);
-      else
-	/* The descriptor is already the one we will use.  But it must
-	   not be marked close-on-exec.  Undo the effects.  */
-	__fcntl (child_end, F_SETFD, 0);
-      /* POSIX.2:  "popen() shall ensure that any streams from previous
-         popen() calls that remain open in the parent process are closed
-	 in the new child process." */
-      for (p = proc_file_chain; p; p = p->next)
-	{
-	  int fd = _IO_fileno ((FILE *) p);
+  {
+    posix_spawn_file_actions_t fa;
+    /* posix_spawn_file_actions_init does not fail.  */
+    __posix_spawn_file_actions_init (&fa);
 
-	  /* If any stream from previous popen() calls has fileno
-	     child_std_end, it has been already closed by the dup2 syscall
-	     above.  */
-	  if (fd != child_std_end)
-	    __close_nocancel (fd);
-	}
+    /* The descriptor is already in the one the child will use.  In this case
+       it must be moved to another one, otherwise there is no safe way to
+       remove the close-on-exec flag in the child without creating a FD leak
+       race in the parent.  */
+    if (pipe_fds[1 - op] == 1 - op)
+      {
+	int tmp = __fcntl (1 - op, F_DUPFD_CLOEXEC, 0);
+	if (tmp < 0)
+	  goto spawn_failure;
+	__close_nocancel (pipe_fds[1 - op]);
+	pipe_fds[1 - op] = tmp;
+      }
 
-      execl ("/bin/sh", "sh", "-c", command, (char *) 0);
-      _exit (127);
-    }
-  __close_nocancel (child_end);
-  if (child_pid < 0)
+    if (__posix_spawn_file_actions_adddup2 (&fa, pipe_fds[1 - op], 1 - op)
+	!= 0)
+      goto spawn_failure;
+
+    /* POSIX.2: "popen() shall ensure that any streams from previous popen()
+       calls that remain open in the parent process are closed in the new
+       child process." */
+    for (struct _IO_proc_file *p = proc_file_chain; p; p = p->next)
+      {
+	int fd = _IO_fileno ((FILE *) p);
+
+	/* If any stream from previous popen() calls has fileno
+	   child_send, it has been already closed by the dup2 syscall
+	   above.  */
+	if (fd != 1 - op
+	    && __posix_spawn_file_actions_addclose (&fa, fd) != 0)
+	  goto spawn_failure;
+      }
+
+    if (__posix_spawn (&((_IO_proc_file *) fp)->pid, _PATH_BSHELL, &fa, 0,
+		     (char *const[]){ (char*) "sh", (char*) "-c",
+		     (char *) command, NULL }, __environ) != 0)
+      {
+      spawn_failure:
+	__posix_spawn_file_actions_destroy (&fa);
+	__close_nocancel (pipe_fds[1 - op]);
+	__set_errno (ENOMEM);
+	return NULL;
+      }
+
+    __posix_spawn_file_actions_destroy (&fa);
+  }
+  __close_nocancel (pipe_fds[1 - op]);
+  if (((_IO_proc_file *) fp)->pid < 0)
     {
-      __close_nocancel (parent_end);
+      __close_nocancel (pipe_fds[op]);
       return NULL;
     }
 
   if (!do_cloexec)
     /* Undo the effects of the pipe2 call which set the
        close-on-exec flag.  */
-    __fcntl (parent_end, F_SETFD, 0);
+    __fcntl (pipe_fds[op], F_SETFD, 0);
 
-  _IO_fileno (fp) = parent_end;
+  _IO_fileno (fp) = pipe_fds[op];
 
   /* Link into proc_file_chain. */
 #ifdef _IO_MTSAFE_IO
-- 
2.17.1

[PATCH 1/3] posix: Add internal symbols for posix_spawn interface

[Adhemerval Zanella]

This patch adds internal hidden definition for mostly of the posix_spawn
function so it can be used internally on both popen and system
implementations.

Checked on x86_64-linux-gnu.

	* include/spawn.h (__posix_spawn, posix_spawn_file_actions_addclose,
	__posix_spawn_file_actions_adddup2, __posix_spawn_file_actions_destroy,
	__posix_spawn_file_actions_init, __posix_spawnattr_init,
	__posix_spawnattr_destroy, __posix_spawnattr_setflags,
	__posix_spawnattr_setsigdefault, __posix_spawnattr_setsigmask): New
	prototype.
	* posix/spawn.c (__posix_spawn): Add libc_hidden_def.
	* posix/spawn_faction_addclose.c
	(__posix_spawn_file_actions_addclose): Add hidden definition.
	* posix/spawn_faction_adddup2.c
	(__posix_spawn_file_actions_adddup2): Likewise.
	* posix/spawn_faction_destroy.c
	(__posix_spawn_file_actions_destroy): Likewise.
	* posix/spawn_faction_init.c (__posix_spawn_file_actions_init):
	Likewise.
	* posix/spawnattr_destroy.c (__posix_spawnattr_destroy): Likewise.
	* posix/spawnattr_init.c (__posix_spawnattr_init): Likewise.
	* posix/spawnattr_setdefault.c (__posix_spawnattr_setsigdefault):
	Likewise.
	* posix/spawnattr_setflags.c (__posix_spawnattr_setflags): Likewise.
	* posix/spawnattr_setsigmask.c (__posix_spawnattr_setsigmask):
	Likewise.
---
 ChangeLog                      | 25 ++++++++++++++++++++++++
 include/spawn.h                | 35 ++++++++++++++++++++++++++++++++++
 posix/spawn.c                  |  1 +
 posix/spawn_faction_addclose.c |  6 ++++--
 posix/spawn_faction_adddup2.c  |  6 ++++--
 posix/spawn_faction_destroy.c  |  4 +++-
 posix/spawn_faction_init.c     |  3 ++-
 posix/spawnattr_destroy.c      |  3 ++-
 posix/spawnattr_init.c         |  3 ++-
 posix/spawnattr_setdefault.c   |  5 +++--
 posix/spawnattr_setflags.c     |  3 ++-
 posix/spawnattr_setsigmask.c   |  3 ++-
 12 files changed, 85 insertions(+), 12 deletions(-)

diff --git a/include/spawn.h b/include/spawn.h
index a6c7a8adc3..7fdd965bd7 100644
--- a/include/spawn.h
+++ b/include/spawn.h
@@ -1 +1,36 @@
+#ifndef _SPAWN_H
 #include <posix/spawn.h>
+
+# ifndef _ISOMAC
+__typeof (posix_spawn) __posix_spawn;
+libc_hidden_proto (__posix_spawn)
+
+__typeof (posix_spawn_file_actions_addclose)
+  __posix_spawn_file_actions_addclose attribute_hidden;
+
+__typeof (posix_spawn_file_actions_adddup2)
+  __posix_spawn_file_actions_adddup2 attribute_hidden;
+
+__typeof (posix_spawn_file_actions_destroy)
+  __posix_spawn_file_actions_destroy attribute_hidden;
+
+__typeof (posix_spawn_file_actions_init) __posix_spawn_file_actions_init
+  attribute_hidden;
+
+__typeof (posix_spawnattr_init) __posix_spawnattr_init
+  attribute_hidden;
+
+__typeof (posix_spawnattr_destroy) __posix_spawnattr_destroy
+  attribute_hidden;
+
+__typeof (posix_spawnattr_setflags) __posix_spawnattr_setflags
+  attribute_hidden;
+
+__typeof (posix_spawnattr_setsigdefault) __posix_spawnattr_setsigdefault
+  attribute_hidden;
+
+__typeof (posix_spawnattr_setsigmask) __posix_spawnattr_setsigmask
+  attribute_hidden;
+
+# endif /* !_ISOMAC  */
+#endif /* spawn.h  */
diff --git a/posix/spawn.c b/posix/spawn.c
index 51f67b2755..a82f1c84e2 100644
--- a/posix/spawn.c
+++ b/posix/spawn.c
@@ -30,6 +30,7 @@ __posix_spawn (pid_t *pid, const char *path,
   return __spawni (pid, path, file_actions, attrp, argv, envp, 0);
 }
 versioned_symbol (libc, __posix_spawn, posix_spawn, GLIBC_2_15);
+libc_hidden_def (__posix_spawn)
 
 
 #if SHLIB_COMPAT (libc, GLIBC_2_2, GLIBC_2_15)
diff --git a/posix/spawn_faction_addclose.c b/posix/spawn_faction_addclose.c
index 21081e19b5..e1fafe438c 100644
--- a/posix/spawn_faction_addclose.c
+++ b/posix/spawn_faction_addclose.c
@@ -24,8 +24,8 @@
 /* Add an action to FILE-ACTIONS which tells the implementation to call
    `close' for the given file descriptor during the `spawn' call.  */
 int
-posix_spawn_file_actions_addclose (posix_spawn_file_actions_t *file_actions,
-				   int fd)
+__posix_spawn_file_actions_addclose (posix_spawn_file_actions_t *file_actions,
+				     int fd)
 {
   struct __spawn_action *rec;
 
@@ -48,3 +48,5 @@ posix_spawn_file_actions_addclose (posix_spawn_file_actions_t *file_actions,
 
   return 0;
 }
+weak_alias (__posix_spawn_file_actions_addclose,
+	    posix_spawn_file_actions_addclose)
diff --git a/posix/spawn_faction_adddup2.c b/posix/spawn_faction_adddup2.c
index 363bc29ae5..371b1de3e6 100644
--- a/posix/spawn_faction_adddup2.c
+++ b/posix/spawn_faction_adddup2.c
@@ -24,8 +24,8 @@
 /* Add an action to FILE-ACTIONS which tells the implementation to call
    `dup2' for the given file descriptors during the `spawn' call.  */
 int
-posix_spawn_file_actions_adddup2 (posix_spawn_file_actions_t *file_actions,
-				  int fd, int newfd)
+__posix_spawn_file_actions_adddup2 (posix_spawn_file_actions_t *file_actions,
+				    int fd, int newfd)
 {
   struct __spawn_action *rec;
 
@@ -49,3 +49,5 @@ posix_spawn_file_actions_adddup2 (posix_spawn_file_actions_t *file_actions,
 
   return 0;
 }
+weak_alias (__posix_spawn_file_actions_adddup2,
+	    posix_spawn_file_actions_adddup2)
diff --git a/posix/spawn_faction_destroy.c b/posix/spawn_faction_destroy.c
index 46061ee347..2a2de4e41d 100644
--- a/posix/spawn_faction_destroy.c
+++ b/posix/spawn_faction_destroy.c
@@ -22,7 +22,7 @@
 
 /* Deallocate the file actions.  */
 int
-posix_spawn_file_actions_destroy (posix_spawn_file_actions_t *file_actions)
+__posix_spawn_file_actions_destroy (posix_spawn_file_actions_t *file_actions)
 {
   /* Free the paths in the open actions.  */
   for (int i = 0; i < file_actions->__used; ++i)
@@ -44,3 +44,5 @@ posix_spawn_file_actions_destroy (posix_spawn_file_actions_t *file_actions)
   free (file_actions->__actions);
   return 0;
 }
+weak_alias (__posix_spawn_file_actions_destroy,
+	    posix_spawn_file_actions_destroy)
diff --git a/posix/spawn_faction_init.c b/posix/spawn_faction_init.c
index ddb42e6a77..98432067c6 100644
--- a/posix/spawn_faction_init.c
+++ b/posix/spawn_faction_init.c
@@ -45,9 +45,10 @@ __posix_spawn_file_actions_realloc (posix_spawn_file_actions_t *file_actions)
 
 /* Initialize data structure for file attribute for `spawn' call.  */
 int
-posix_spawn_file_actions_init (posix_spawn_file_actions_t *file_actions)
+__posix_spawn_file_actions_init (posix_spawn_file_actions_t *file_actions)
 {
   /* Simply clear all the elements.  */
   memset (file_actions, '\0', sizeof (*file_actions));
   return 0;
 }
+weak_alias (__posix_spawn_file_actions_init, posix_spawn_file_actions_init)
diff --git a/posix/spawnattr_destroy.c b/posix/spawnattr_destroy.c
index 603e00fffe..0433867785 100644
--- a/posix/spawnattr_destroy.c
+++ b/posix/spawnattr_destroy.c
@@ -19,8 +19,9 @@
 
 /* Initialize data structure for file attribute for `spawn' call.  */
 int
-posix_spawnattr_destroy (posix_spawnattr_t *attr)
+__posix_spawnattr_destroy (posix_spawnattr_t *attr)
 {
   /* Nothing to do in the moment.  */
   return 0;
 }
+weak_alias (__posix_spawnattr_destroy, posix_spawnattr_destroy)
diff --git a/posix/spawnattr_init.c b/posix/spawnattr_init.c
index bab464e62b..4e1218ab44 100644
--- a/posix/spawnattr_init.c
+++ b/posix/spawnattr_init.c
@@ -20,7 +20,7 @@
 
 /* Initialize data structure for file attribute for `spawn' call.  */
 int
-posix_spawnattr_init (posix_spawnattr_t *attr)
+__posix_spawnattr_init (posix_spawnattr_t *attr)
 {
   /* All elements have to be initialized to the default values which
      is generally zero.  */
@@ -28,3 +28,4 @@ posix_spawnattr_init (posix_spawnattr_t *attr)
 
   return 0;
 }
+weak_alias (__posix_spawnattr_init, posix_spawnattr_init)
diff --git a/posix/spawnattr_setdefault.c b/posix/spawnattr_setdefault.c
index c77cda59be..174bcfa423 100644
--- a/posix/spawnattr_setdefault.c
+++ b/posix/spawnattr_setdefault.c
@@ -20,11 +20,12 @@
 
 /* Set signal mask for signals with default handling in ATTR to SIGDEFAULT.  */
 int
-posix_spawnattr_setsigdefault (posix_spawnattr_t *attr,
-			       const sigset_t *sigdefault)
+__posix_spawnattr_setsigdefault (posix_spawnattr_t *attr,
+				 const sigset_t *sigdefault)
 {
   /* Copy the sigset_t data to the user buffer.  */
   memcpy (&attr->__sd, sigdefault, sizeof (sigset_t));
 
   return 0;
 }
+weak_alias (__posix_spawnattr_setsigdefault, posix_spawnattr_setsigdefault)
diff --git a/posix/spawnattr_setflags.c b/posix/spawnattr_setflags.c
index cf9a60181d..0a42e94770 100644
--- a/posix/spawnattr_setflags.c
+++ b/posix/spawnattr_setflags.c
@@ -30,7 +30,7 @@
 
 /* Store flags in the attribute structure.  */
 int
-posix_spawnattr_setflags (posix_spawnattr_t *attr, short int flags)
+__posix_spawnattr_setflags (posix_spawnattr_t *attr, short int flags)
 {
   /* Check no invalid bits are set.  */
   if (flags & ~ALL_FLAGS)
@@ -41,3 +41,4 @@ posix_spawnattr_setflags (posix_spawnattr_t *attr, short int flags)
 
   return 0;
 }
+weak_alias (__posix_spawnattr_setflags, posix_spawnattr_setflags)
diff --git a/posix/spawnattr_setsigmask.c b/posix/spawnattr_setsigmask.c
index 7ae81ad470..12c0111af4 100644
--- a/posix/spawnattr_setsigmask.c
+++ b/posix/spawnattr_setsigmask.c
@@ -20,7 +20,7 @@
 
 /* Set signal mask for the new process in ATTR to SIGMASK.  */
 int
-posix_spawnattr_setsigmask (posix_spawnattr_t *attr,
+__posix_spawnattr_setsigmask (posix_spawnattr_t *attr,
 			    const sigset_t *sigmask)
 {
   /* Copy the sigset_t data to the user buffer.  */
@@ -28,3 +28,4 @@ posix_spawnattr_setsigmask (posix_spawnattr_t *attr,
 
   return 0;
 }
+weak_alias (__posix_spawnattr_setsigmask, posix_spawnattr_setsigmask)
-- 
2.17.1

[PATCH 3/3] posix: Use posix_spawn on system

[Adhemerval Zanella]

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.  While reentracy handling does not require
any direct change of current strategy, cancellation requires a posix_spawn
to be cancellable.  This is done by adding an internal
__posix_spawn_cancellable which does not disable cancellation neither
change process signal mask.

The cancellation code is also moved to generic implementation and enabled
only if SIGCANCEL is defined (similar on how the cancellation handler is
enabled on nptl-init.c).

Checked on x86_64-linux-gnu and i686-linux-gnu.

	* include/spawn.h (__posix_spawn_cancellable): New prototype.
	* posix/spawn.c (__posix_spawn_cancellable): New function.
	* posix/spawn_int.h (SPAWN_XFLAGS_ENABLE_CANCEL): New define.
	* sysdeps/generic/not-errno.h (__kill_noerrno): New prototype.
	* sysdeps/unix/sysv/linux/not-errno.h (__kill_noerrno): New
	implementation.
	* sysdeps/posix/spawni.c (__spawnix): Disable cancellation for
	!SPAWN_XFLAGS_ENABLE_CANCEL.
	* sysdeps/unix/sysv/linux/spawni.c (__spawnix): Likewise.
	* sysdeps/posix/system.c [_LIBC_REENTRANT && SIGCANCEL]
	(cancel_handler): New function.
	(CLEANUP_HANDLER, CLEANUP_RESET): Likewise.
	(do_system): Use posix_spawn instead of fork and execl.
	* 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                              |  18 ++++
 include/spawn.h                        |   2 +
 posix/spawn.c                          |  14 +++
 posix/spawn_int.h                      |   1 +
 sysdeps/generic/not-errno.h            |   2 +
 sysdeps/posix/spawni.c                 |   8 +-
 sysdeps/posix/system.c                 | 132 ++++++++++++++-----------
 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       |  24 +++--
 sysdeps/unix/sysv/linux/system.c       |  76 --------------
 13 files changed, 147 insertions(+), 232 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/include/spawn.h b/include/spawn.h
index 7fdd965bd7..995ab9a8f4 100644
--- a/include/spawn.h
+++ b/include/spawn.h
@@ -5,6 +5,8 @@
 __typeof (posix_spawn) __posix_spawn;
 libc_hidden_proto (__posix_spawn)
 
+__typeof (posix_spawn) __posix_spawn_cancellable attribute_hidden;
+
 __typeof (posix_spawn_file_actions_addclose)
   __posix_spawn_file_actions_addclose attribute_hidden;
 
diff --git a/posix/spawn.c b/posix/spawn.c
index a82f1c84e2..24a416f53c 100644
--- a/posix/spawn.c
+++ b/posix/spawn.c
@@ -33,6 +33,20 @@ versioned_symbol (libc, __posix_spawn, posix_spawn, GLIBC_2_15);
 libc_hidden_def (__posix_spawn)
 
 
+/* Same as __posix_spawn but allows asynchronous cancellation.  It is used
+   internally on SYSTEM implementation to avoid leak a process ID through
+   a cancellation handler.  */
+int
+__posix_spawn_cancellable (pid_t *pid, const char *path,
+			   const posix_spawn_file_actions_t *file_actions,
+			   const posix_spawnattr_t *attrp, char *const argv[],
+			   char *const envp[])
+{
+  return __spawni (pid, path, file_actions, attrp, argv, envp,
+		   SPAWN_XFLAGS_ENABLE_CANCEL);
+}
+
+
 #if SHLIB_COMPAT (libc, GLIBC_2_2, GLIBC_2_15)
 int
 attribute_compat_text_section
diff --git a/posix/spawn_int.h b/posix/spawn_int.h
index 171f67c649..5cb8ce49b3 100644
--- a/posix/spawn_int.h
+++ b/posix/spawn_int.h
@@ -55,6 +55,7 @@ struct __spawn_action
 
 #define SPAWN_XFLAGS_USE_PATH	0x1
 #define SPAWN_XFLAGS_TRY_SHELL	0x2
+#define SPAWN_XFLAGS_ENABLE_CANCEL 0x3
 
 extern int __posix_spawn_file_actions_realloc (posix_spawn_file_actions_t *
 					       file_actions)
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/spawni.c b/sysdeps/posix/spawni.c
index b138ab4393..e1a391659b 100644
--- a/sysdeps/posix/spawni.c
+++ b/sysdeps/posix/spawni.c
@@ -256,8 +256,9 @@ __spawnix (pid_t *pid, const char *file,
 
   /* Disable asynchronous cancellation.  */
   int state;
-  __libc_ptf_call (__pthread_setcancelstate,
-                   (PTHREAD_CANCEL_DISABLE, &state), 0);
+  if ((xflags & SPAWN_XFLAGS_ENABLE_CANCEL) == 0)
+    __libc_ptf_call (__pthread_setcancelstate,
+                     (PTHREAD_CANCEL_DISABLE, &state), 0);
 
   ptrdiff_t argc = 0;
   ptrdiff_t limit = INT_MAX - 1;
@@ -299,7 +300,8 @@ __spawnix (pid_t *pid, const char *file,
   if ((ec == 0) && (pid != NULL))
     *pid = new_pid;
 
-  __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
+  if ((xflags & SPAWN_XFLAGS_ENABLE_CANCEL) == 0)
+    __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
 
   return ec;
 }
diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c
index d7594436ed..38d5e28ca2 100644
--- a/sysdeps/posix/system.c
+++ b/sysdeps/posix/system.c
@@ -17,20 +17,22 @@
 
 #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 _LIBC_REENTRANT
 static struct sigaction intr, quit;
 static int sa_refcntr;
@@ -38,17 +40,53 @@ __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
 
+/* 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.  */
+
+#if defined(_LIBC_REENTRANT) && defined(SIGCANCEL)
+static void cancel_handler (void *arg)
+{
+  pid_t child = *(pid_t *) arg;
+
+  __kill_noerrno (child, SIGKILL);
+
+  TEMP_FAILURE_RETRY (__waitpid (child, NULL, 0));
+
+  DO_LOCK ();
+
+  if (SUB_REF () == 0)
+    {
+      __sigaction (SIGQUIT, &quit, NULL);
+      __sigaction (SIGINT, &intr, NULL);
+    }
+
+  DO_UNLOCK ();
+}
+# define CLEANUP_HANDLER() \
+  __libc_cleanup_region_start (1, cancel_handler, &pid)
+# define CLEANUP_RESET() \
+  __libc_cleanup_region_end (0)
+#else
+# define CLEANUP_HANDLER()
+# define CLEANUP_RESET()
+#endif
+
 
 /* Execute LINE as a shell command, returning its status.  */
 static int
@@ -60,7 +98,9 @@ do_system (const char *line)
 #ifndef _LIBC_REENTRANT
   struct sigaction intr, quit;
 #endif
+  struct sigaction oldintr, oldquit;
   sigset_t omask;
+  sigset_t reset;
 
   sa.sa_handler = SIG_IGN;
   sa.sa_flags = 0;
@@ -98,9 +138,9 @@ do_system (const char *line)
 	  if (SUB_REF () == 0)
 	    {
 	      save = errno;
-	      (void) __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL);
+	      __sigaction (SIGQUIT, &quit, &oldquit);
 	    out_restore_sigint:
-	      (void) __sigaction (SIGINT, &intr, (struct sigaction *) NULL);
+	      __sigaction (SIGINT, &intr, &oldintr);
 	      __set_errno (save);
 	    }
 	out:
@@ -109,39 +149,30 @@ do_system (const char *line)
 	}
     }
 
-#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.  */
+  __sigemptyset (&reset);
+  if (oldintr.sa_handler != SIG_IGN)
+    __sigaddset(&reset, SIGINT);
+  if (oldquit.sa_handler != SIG_IGN)
+    __sigaddset(&reset, SIGQUIT);
+
+  posix_spawnattr_t spawn_attr;
+  /* None of the posix_spawnattr_* function returns, 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);
+
+  CLEANUP_HANDLER ();
+  status = __posix_spawn_cancellable (&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)
     {
       /* Note the system() is a cancellation point.  But since we call
 	 waitpid() which itself is a cancellation point we do not
@@ -149,25 +180,16 @@ do_system (const char *line)
       if (TEMP_FAILURE_RETRY (__waitpid (pid, &status, 0)) != pid)
 	status = -1;
     }
-
-#ifdef CLEANUP_HANDLER
-  CLEANUP_RESET;
-#endif
+  CLEANUP_RESET ();
 
   save = errno;
   DO_LOCK ();
   if ((SUB_REF () == 0
-       && (__sigaction (SIGINT, &intr, (struct sigaction *) NULL)
-	   | __sigaction (SIGQUIT, &quit, (struct sigaction *) NULL)) != 0)
+       && (__sigaction (SIGINT, &intr, NULL)
+	   | __sigaction (SIGQUIT, &quit, 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;
+      status = -1;
     }
   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 85239cedbf..ed5c613e42 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;
@@ -330,10 +330,14 @@ __spawnix (pid_t * pid, const char *file,
   if (__glibc_unlikely (stack == MAP_FAILED))
     return errno;
 
-  /* Disable asynchronous cancellation.  */
   int state;
-  __libc_ptf_call (__pthread_setcancelstate,
-                   (PTHREAD_CANCEL_DISABLE, &state), 0);
+  if ((xflags & SPAWN_XFLAGS_ENABLE_CANCEL) == 0)
+    {
+      /* Disable asynchronous cancellation.  */
+      __libc_ptf_call (__pthread_setcancelstate,
+		       (PTHREAD_CANCEL_DISABLE, &state), 0);
+      __libc_signal_block_all (&args.oldmask);
+    }
 
   /* Child must set args.err to something non-negative - we rely on
      the parent and child sharing VM.  */
@@ -347,8 +351,6 @@ __spawnix (pid_t * pid, const char *file,
   args.envp = envp;
   args.xflags = xflags;
 
-  __libc_signal_block_all (&args.oldmask);
-
   /* The clone flags used will create a new child that will run in the same
      memory space (CLONE_VM) and the execution of calling thread will be
      suspend until the child calls execve or _exit.
@@ -389,9 +391,11 @@ __spawnix (pid_t * pid, const char *file,
   if ((ec == 0) && (pid != NULL))
     *pid = new_pid;
 
-  __libc_signal_restore_set (&args.oldmask);
-
-  __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
+  if ((xflags & SPAWN_XFLAGS_ENABLE_CANCEL) == 0)
+    {
+      __libc_signal_restore_set (&args.oldmask);
+      __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
+    }
 
   return ec;
 }
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

Re: [PATCH 2/3] posix: Use posix_spawn on popen

[David Newall]

It seems to me that there are still reasonable questions about whether 
to use posix_spawn or vfork ("posix_spawn is a badly designed API").  
For (well over) 30 years, I've understood that vfork was the go-to call 
for a fork/exec scenario, so, what is the technical problem with using 
it for popen and system?  (I'm not asking about vfork's overall 
technical merits, I'm asking exclusively about using it for popen and 
system.)

Re: [PATCH 1/3] posix: Add internal symbols for posix_spawn interface

[Florian Weimer]

On 09/15/2018 05:16 PM, Adhemerval Zanella wrote:
> +__typeof (posix_spawn) __posix_spawn;
> +libc_hidden_proto (__posix_spawn)

I think libc_hidden_proto without a GLIBC_PRIVATE export doesn't make 
much sense.  Why can't you use attribute_hidden here, like for the other 
new function symbols?

Thanks,
Florian

Re: [PATCH 2/3] posix: Use posix_spawn on popen

[Rich Felker]

On Sun, Sep 16, 2018 at 02:43:02PM +0930, David Newall wrote:
> It seems to me that there are still reasonable questions about
> whether to use posix_spawn or vfork ("posix_spawn is a badly
> designed API").  For (well over) 30 years, I've understood that
> vfork was the go-to call for a fork/exec scenario, so, what is the
> technical problem with using it for popen and system?  (I'm not
> asking about vfork's overall technical merits, I'm asking
> exclusively about using it for popen and system.)

The historical contract of vfork is that you can basically do nothing
after it returns in the child except for exec or _exit, and there are
good reasons for this; sharing memory and stack with the parent has
lots of subtle issues, especially in the presence of a non-dead-stupid
compiler.

One thing to note is that vfork is completely unsafe to use as
documented if any signal handlers are installed, unless you block all
signals before calling vfork, in which case the exec'd process will
inherit a fully-blocked signal mask which is probably not what you
want. Otherwise signal handlers may wrongly run in the child that's
sharing memory with the parent.

The posix_spawn implementation already takes care of these issues by
not sharing the stack and uninstalling any signal handlers before
unmasking signals.

Rich

Re: [PATCH 2/3] posix: Use posix_spawn on popen

[Adhemerval Zanella]

On 17/09/2018 07:50, Rich Felker wrote:
> On Sun, Sep 16, 2018 at 02:43:02PM +0930, David Newall wrote:
>> It seems to me that there are still reasonable questions about
>> whether to use posix_spawn or vfork ("posix_spawn is a badly
>> designed API").  For (well over) 30 years, I've understood that
>> vfork was the go-to call for a fork/exec scenario, so, what is the
>> technical problem with using it for popen and system?  (I'm not
>> asking about vfork's overall technical merits, I'm asking
>> exclusively about using it for popen and system.)
> 
> The historical contract of vfork is that you can basically do nothing
> after it returns in the child except for exec or _exit, and there are
> good reasons for this; sharing memory and stack with the parent has
> lots of subtle issues, especially in the presence of a non-dead-stupid
> compiler.
> 
> One thing to note is that vfork is completely unsafe to use as
> documented if any signal handlers are installed, unless you block all
> signals before calling vfork, in which case the exec'd process will
> inherit a fully-blocked signal mask which is probably not what you
> want. Otherwise signal handlers may wrongly run in the child that's
> sharing memory with the parent.
> 
> The posix_spawn implementation already takes care of these issues by
> not sharing the stack and uninstalling any signal handlers before
> unmasking signals.
> 
> Rich
> 

And posix_spawn implementation on Linux uses the same performance
improvements that vfork aims to provide. 

Re: [PATCH 1/3] posix: Add internal symbols for posix_spawn interface

[Adhemerval Zanella]

On 17/09/2018 05:12, Florian Weimer wrote:
> On 09/15/2018 05:16 PM, Adhemerval Zanella wrote:
>> +__typeof (posix_spawn) __posix_spawn;
>> +libc_hidden_proto (__posix_spawn)
> 
> I think libc_hidden_proto without a GLIBC_PRIVATE export doesn't make much sense.  Why can't you use attribute_hidden here, like for the other new function symbols?
> 
> Thanks,
> Florian

I had to use libc_hidden_proto because versioned_symbol with a hidden 
symbol produces only a local one:

$ cat foo.c 
void __foo () __attribute__ ((visibility ("hidden")));

void __foo ()
{
}
__asm__ (".symver __foo, foo@@LIBFOO_1.0");
$ cat libfoo.map 
LIBFOO_1.0 {
  global:
    foo;
};
$ gcc -Wall -fpic -shared foo.c -o libfoo.so -Wl,-version-script=libfoo.map
$ objdump -t libfoo.so | grep -w foo
0000000000000000 l    df *ABS*  0000000000000000              foo.c
00000000000005da l     F .text  0000000000000007              foo@@LIBFOO_1.0

Re: [PATCH 2/3] posix: Use posix_spawn on popen

[David Newall]

On 18/09/18 00:20, Rich Felker wrote:
> The historical contract of vfork is that you can basically do nothing
> after it returns in the child except for exec or _exit
Yes, that is true.  My  understanding is that vfork was intended only as 
a fast way of doing fork/exec sequence.

> One thing to note is that vfork is completely unsafe to use as
> documented if any signal handlers are installed, unless you block all
> signals before calling vfork, in which case the exec'd process will
> inherit a fully-blocked signal mask which is probably not what you
> want. Otherwise signal handlers may wrongly run in the child that's
> sharing memory with the parent.

You're saying that kernel will deliver a signal to child pid when it was 
parent pid that was signalled.  Can that really happen?

Re: [PATCH 2/3] posix: Use posix_spawn on popen

[Rich Felker]

On Tue, Sep 18, 2018 at 11:00:48AM +0930, David Newall wrote:
> On 18/09/18 00:20, Rich Felker wrote:
> >The historical contract of vfork is that you can basically do nothing
> >after it returns in the child except for exec or _exit
> Yes, that is true.  My  understanding is that vfork was intended
> only as a fast way of doing fork/exec sequence.
> 
> >One thing to note is that vfork is completely unsafe to use as
> >documented if any signal handlers are installed, unless you block all
> >signals before calling vfork, in which case the exec'd process will
> >inherit a fully-blocked signal mask which is probably not what you
> >want. Otherwise signal handlers may wrongly run in the child that's
> >sharing memory with the parent.
> 
> You're saying that kernel will deliver a signal to child pid when it
> was parent pid that was signalled.  Can that really happen?

There are various conditions under which signals are delivered to an
entire process group; the most well-known is tty signals from ^C, ^\,
^Z, SIGWINCH, etc. to the tty's foreground process group. After vfork
these would be delivered to both the parent and child while they share
memory. The parent is suspended and won't act until the child execs or
exits, but mere execution of the signal handler in the child is
observably and dangerously wrong behavior.

Rich

Re: [PATCH 2/3] posix: Use posix_spawn on popen

[Zack Weinberg]

On Sun, Sep 16, 2018 at 1:13 AM David Newall <glibc@davidnewall.com> wrote:
> It seems to me that there are still reasonable questions about whether
> to use posix_spawn or vfork ("posix_spawn is a badly designed API").

When I said to Sergey that I would rather see the problem they
reported addressed using vfork instead of posix_spawn, I was giving
advice to a new contributor.  I really _would_ rather see it addressed
that way, and I also thought that they were more likely to succeed in
writing those patches.

Adhemerval is not a new contributor and they deeply understand the
problems in this area.  Their patches strike me as a step generally in
the right direction.  I don't have time to review them in detail, but
I don't object to them.  However, do I think some of the fine details
demonstrate why an API that allows for arbitrary computation and
system calls before exec would be preferable, such as there being "no
safe way to clear close-on-exec in the child" (because, IIUC, there's
no posix_spawn action to do that).

zw

Re: [PATCH 2/3] posix: Use posix_spawn on popen

[Rich Felker]

On Tue, Sep 18, 2018 at 02:01:29PM -0400, Zack Weinberg wrote:
> On Sun, Sep 16, 2018 at 1:13 AM David Newall <glibc@davidnewall.com> wrote:
> > It seems to me that there are still reasonable questions about whether
> > to use posix_spawn or vfork ("posix_spawn is a badly designed API").
> 
> When I said to Sergey that I would rather see the problem they
> reported addressed using vfork instead of posix_spawn, I was giving
> advice to a new contributor.  I really _would_ rather see it addressed
> that way, and I also thought that they were more likely to succeed in
> writing those patches.
> 
> Adhemerval is not a new contributor and they deeply understand the
> problems in this area.  Their patches strike me as a step generally in
> the right direction.  I don't have time to review them in detail, but
> I don't object to them.  However, do I think some of the fine details
> demonstrate why an API that allows for arbitrary computation and
> system calls before exec would be preferable, such as there being "no
> safe way to clear close-on-exec in the child" (because, IIUC, there's
> no posix_spawn action to do that).

The resolution to Austin Group issue #411 made it so adddup2(n,n) does
what you want:

http://austingroupbugs.net/view.php?id=411

Rich

Re: [PATCH 2/3] posix: Use posix_spawn on popen

[Adhemerval Zanella]

On 18/09/2018 22:17, Rich Felker wrote:
> On Tue, Sep 18, 2018 at 02:01:29PM -0400, Zack Weinberg wrote:
>> On Sun, Sep 16, 2018 at 1:13 AM David Newall <glibc@davidnewall.com> wrote:
>>> It seems to me that there are still reasonable questions about whether
>>> to use posix_spawn or vfork ("posix_spawn is a badly designed API").
>>
>> When I said to Sergey that I would rather see the problem they
>> reported addressed using vfork instead of posix_spawn, I was giving
>> advice to a new contributor.  I really _would_ rather see it addressed
>> that way, and I also thought that they were more likely to succeed in
>> writing those patches.
>>
>> Adhemerval is not a new contributor and they deeply understand the
>> problems in this area.  Their patches strike me as a step generally in
>> the right direction.  I don't have time to review them in detail, but
>> I don't object to them.  However, do I think some of the fine details
>> demonstrate why an API that allows for arbitrary computation and
>> system calls before exec would be preferable, such as there being "no
>> safe way to clear close-on-exec in the child" (because, IIUC, there's
>> no posix_spawn action to do that).
> 
> The resolution to Austin Group issue #411 made it so adddup2(n,n) does
> what you want:
> 
> http://austingroupbugs.net/view.php?id=411

I has been tracked on https://sourceware.org/bugzilla/show_bug.cgi?id=23640 
as well. I am not found of having adddup2(n,n) semantic different dup2(n,n),
but I don't have a better straightforward solution either. 

Re: [PATCH 1/3] posix: Add internal symbols for posix_spawn interface

[Florian Weimer]

* Adhemerval Zanella:

> On 17/09/2018 05:12, Florian Weimer wrote:
>> On 09/15/2018 05:16 PM, Adhemerval Zanella wrote:
>>> +__typeof (posix_spawn) __posix_spawn;
>>> +libc_hidden_proto (__posix_spawn)
>> 
>> I think libc_hidden_proto without a GLIBC_PRIVATE export doesn't
>> make much sense.  Why can't you use attribute_hidden here, like for
>> the other new function symbols?
>> 
>> Thanks,
>> Florian
>
> I had to use libc_hidden_proto because versioned_symbol with a hidden 
> symbol produces only a local one:

I see, I had forgotten about this.

Thanks,
Florian

Re: [PATCH 1/3] posix: Add internal symbols for posix_spawn interface

[Adhemerval Zanella]

Ping.

On 15/09/2018 12:16, Adhemerval Zanella wrote:
> This patch adds internal hidden definition for mostly of the posix_spawn
> function so it can be used internally on both popen and system
> implementations.
> 
> Checked on x86_64-linux-gnu.
> 
> 	* include/spawn.h (__posix_spawn, posix_spawn_file_actions_addclose,
> 	__posix_spawn_file_actions_adddup2, __posix_spawn_file_actions_destroy,
> 	__posix_spawn_file_actions_init, __posix_spawnattr_init,
> 	__posix_spawnattr_destroy, __posix_spawnattr_setflags,
> 	__posix_spawnattr_setsigdefault, __posix_spawnattr_setsigmask): New
> 	prototype.
> 	* posix/spawn.c (__posix_spawn): Add libc_hidden_def.
> 	* posix/spawn_faction_addclose.c
> 	(__posix_spawn_file_actions_addclose): Add hidden definition.
> 	* posix/spawn_faction_adddup2.c
> 	(__posix_spawn_file_actions_adddup2): Likewise.
> 	* posix/spawn_faction_destroy.c
> 	(__posix_spawn_file_actions_destroy): Likewise.
> 	* posix/spawn_faction_init.c (__posix_spawn_file_actions_init):
> 	Likewise.
> 	* posix/spawnattr_destroy.c (__posix_spawnattr_destroy): Likewise.
> 	* posix/spawnattr_init.c (__posix_spawnattr_init): Likewise.
> 	* posix/spawnattr_setdefault.c (__posix_spawnattr_setsigdefault):
> 	Likewise.
> 	* posix/spawnattr_setflags.c (__posix_spawnattr_setflags): Likewise.
> 	* posix/spawnattr_setsigmask.c (__posix_spawnattr_setsigmask):
> 	Likewise.
> ---
>  ChangeLog                      | 25 ++++++++++++++++++++++++
>  include/spawn.h                | 35 ++++++++++++++++++++++++++++++++++
>  posix/spawn.c                  |  1 +
>  posix/spawn_faction_addclose.c |  6 ++++--
>  posix/spawn_faction_adddup2.c  |  6 ++++--
>  posix/spawn_faction_destroy.c  |  4 +++-
>  posix/spawn_faction_init.c     |  3 ++-
>  posix/spawnattr_destroy.c      |  3 ++-
>  posix/spawnattr_init.c         |  3 ++-
>  posix/spawnattr_setdefault.c   |  5 +++--
>  posix/spawnattr_setflags.c     |  3 ++-
>  posix/spawnattr_setsigmask.c   |  3 ++-
>  12 files changed, 85 insertions(+), 12 deletions(-)
> 
> diff --git a/include/spawn.h b/include/spawn.h
> index a6c7a8adc3..7fdd965bd7 100644
> --- a/include/spawn.h
> +++ b/include/spawn.h
> @@ -1 +1,36 @@
> +#ifndef _SPAWN_H
>  #include <posix/spawn.h>
> +
> +# ifndef _ISOMAC
> +__typeof (posix_spawn) __posix_spawn;
> +libc_hidden_proto (__posix_spawn)
> +
> +__typeof (posix_spawn_file_actions_addclose)
> +  __posix_spawn_file_actions_addclose attribute_hidden;
> +
> +__typeof (posix_spawn_file_actions_adddup2)
> +  __posix_spawn_file_actions_adddup2 attribute_hidden;
> +
> +__typeof (posix_spawn_file_actions_destroy)
> +  __posix_spawn_file_actions_destroy attribute_hidden;
> +
> +__typeof (posix_spawn_file_actions_init) __posix_spawn_file_actions_init
> +  attribute_hidden;
> +
> +__typeof (posix_spawnattr_init) __posix_spawnattr_init
> +  attribute_hidden;
> +
> +__typeof (posix_spawnattr_destroy) __posix_spawnattr_destroy
> +  attribute_hidden;
> +
> +__typeof (posix_spawnattr_setflags) __posix_spawnattr_setflags
> +  attribute_hidden;
> +
> +__typeof (posix_spawnattr_setsigdefault) __posix_spawnattr_setsigdefault
> +  attribute_hidden;
> +
> +__typeof (posix_spawnattr_setsigmask) __posix_spawnattr_setsigmask
> +  attribute_hidden;
> +
> +# endif /* !_ISOMAC  */
> +#endif /* spawn.h  */
> diff --git a/posix/spawn.c b/posix/spawn.c
> index 51f67b2755..a82f1c84e2 100644
> --- a/posix/spawn.c
> +++ b/posix/spawn.c
> @@ -30,6 +30,7 @@ __posix_spawn (pid_t *pid, const char *path,
>    return __spawni (pid, path, file_actions, attrp, argv, envp, 0);
>  }
>  versioned_symbol (libc, __posix_spawn, posix_spawn, GLIBC_2_15);
> +libc_hidden_def (__posix_spawn)
>  
>  
>  #if SHLIB_COMPAT (libc, GLIBC_2_2, GLIBC_2_15)
> diff --git a/posix/spawn_faction_addclose.c b/posix/spawn_faction_addclose.c
> index 21081e19b5..e1fafe438c 100644
> --- a/posix/spawn_faction_addclose.c
> +++ b/posix/spawn_faction_addclose.c
> @@ -24,8 +24,8 @@
>  /* Add an action to FILE-ACTIONS which tells the implementation to call
>     `close' for the given file descriptor during the `spawn' call.  */
>  int
> -posix_spawn_file_actions_addclose (posix_spawn_file_actions_t *file_actions,
> -				   int fd)
> +__posix_spawn_file_actions_addclose (posix_spawn_file_actions_t *file_actions,
> +				     int fd)
>  {
>    struct __spawn_action *rec;
>  
> @@ -48,3 +48,5 @@ posix_spawn_file_actions_addclose (posix_spawn_file_actions_t *file_actions,
>  
>    return 0;
>  }
> +weak_alias (__posix_spawn_file_actions_addclose,
> +	    posix_spawn_file_actions_addclose)
> diff --git a/posix/spawn_faction_adddup2.c b/posix/spawn_faction_adddup2.c
> index 363bc29ae5..371b1de3e6 100644
> --- a/posix/spawn_faction_adddup2.c
> +++ b/posix/spawn_faction_adddup2.c
> @@ -24,8 +24,8 @@
>  /* Add an action to FILE-ACTIONS which tells the implementation to call
>     `dup2' for the given file descriptors during the `spawn' call.  */
>  int
> -posix_spawn_file_actions_adddup2 (posix_spawn_file_actions_t *file_actions,
> -				  int fd, int newfd)
> +__posix_spawn_file_actions_adddup2 (posix_spawn_file_actions_t *file_actions,
> +				    int fd, int newfd)
>  {
>    struct __spawn_action *rec;
>  
> @@ -49,3 +49,5 @@ posix_spawn_file_actions_adddup2 (posix_spawn_file_actions_t *file_actions,
>  
>    return 0;
>  }
> +weak_alias (__posix_spawn_file_actions_adddup2,
> +	    posix_spawn_file_actions_adddup2)
> diff --git a/posix/spawn_faction_destroy.c b/posix/spawn_faction_destroy.c
> index 46061ee347..2a2de4e41d 100644
> --- a/posix/spawn_faction_destroy.c
> +++ b/posix/spawn_faction_destroy.c
> @@ -22,7 +22,7 @@
>  
>  /* Deallocate the file actions.  */
>  int
> -posix_spawn_file_actions_destroy (posix_spawn_file_actions_t *file_actions)
> +__posix_spawn_file_actions_destroy (posix_spawn_file_actions_t *file_actions)
>  {
>    /* Free the paths in the open actions.  */
>    for (int i = 0; i < file_actions->__used; ++i)
> @@ -44,3 +44,5 @@ posix_spawn_file_actions_destroy (posix_spawn_file_actions_t *file_actions)
>    free (file_actions->__actions);
>    return 0;
>  }
> +weak_alias (__posix_spawn_file_actions_destroy,
> +	    posix_spawn_file_actions_destroy)
> diff --git a/posix/spawn_faction_init.c b/posix/spawn_faction_init.c
> index ddb42e6a77..98432067c6 100644
> --- a/posix/spawn_faction_init.c
> +++ b/posix/spawn_faction_init.c
> @@ -45,9 +45,10 @@ __posix_spawn_file_actions_realloc (posix_spawn_file_actions_t *file_actions)
>  
>  /* Initialize data structure for file attribute for `spawn' call.  */
>  int
> -posix_spawn_file_actions_init (posix_spawn_file_actions_t *file_actions)
> +__posix_spawn_file_actions_init (posix_spawn_file_actions_t *file_actions)
>  {
>    /* Simply clear all the elements.  */
>    memset (file_actions, '\0', sizeof (*file_actions));
>    return 0;
>  }
> +weak_alias (__posix_spawn_file_actions_init, posix_spawn_file_actions_init)
> diff --git a/posix/spawnattr_destroy.c b/posix/spawnattr_destroy.c
> index 603e00fffe..0433867785 100644
> --- a/posix/spawnattr_destroy.c
> +++ b/posix/spawnattr_destroy.c
> @@ -19,8 +19,9 @@
>  
>  /* Initialize data structure for file attribute for `spawn' call.  */
>  int
> -posix_spawnattr_destroy (posix_spawnattr_t *attr)
> +__posix_spawnattr_destroy (posix_spawnattr_t *attr)
>  {
>    /* Nothing to do in the moment.  */
>    return 0;
>  }
> +weak_alias (__posix_spawnattr_destroy, posix_spawnattr_destroy)
> diff --git a/posix/spawnattr_init.c b/posix/spawnattr_init.c
> index bab464e62b..4e1218ab44 100644
> --- a/posix/spawnattr_init.c
> +++ b/posix/spawnattr_init.c
> @@ -20,7 +20,7 @@
>  
>  /* Initialize data structure for file attribute for `spawn' call.  */
>  int
> -posix_spawnattr_init (posix_spawnattr_t *attr)
> +__posix_spawnattr_init (posix_spawnattr_t *attr)
>  {
>    /* All elements have to be initialized to the default values which
>       is generally zero.  */
> @@ -28,3 +28,4 @@ posix_spawnattr_init (posix_spawnattr_t *attr)
>  
>    return 0;
>  }
> +weak_alias (__posix_spawnattr_init, posix_spawnattr_init)
> diff --git a/posix/spawnattr_setdefault.c b/posix/spawnattr_setdefault.c
> index c77cda59be..174bcfa423 100644
> --- a/posix/spawnattr_setdefault.c
> +++ b/posix/spawnattr_setdefault.c
> @@ -20,11 +20,12 @@
>  
>  /* Set signal mask for signals with default handling in ATTR to SIGDEFAULT.  */
>  int
> -posix_spawnattr_setsigdefault (posix_spawnattr_t *attr,
> -			       const sigset_t *sigdefault)
> +__posix_spawnattr_setsigdefault (posix_spawnattr_t *attr,
> +				 const sigset_t *sigdefault)
>  {
>    /* Copy the sigset_t data to the user buffer.  */
>    memcpy (&attr->__sd, sigdefault, sizeof (sigset_t));
>  
>    return 0;
>  }
> +weak_alias (__posix_spawnattr_setsigdefault, posix_spawnattr_setsigdefault)
> diff --git a/posix/spawnattr_setflags.c b/posix/spawnattr_setflags.c
> index cf9a60181d..0a42e94770 100644
> --- a/posix/spawnattr_setflags.c
> +++ b/posix/spawnattr_setflags.c
> @@ -30,7 +30,7 @@
>  
>  /* Store flags in the attribute structure.  */
>  int
> -posix_spawnattr_setflags (posix_spawnattr_t *attr, short int flags)
> +__posix_spawnattr_setflags (posix_spawnattr_t *attr, short int flags)
>  {
>    /* Check no invalid bits are set.  */
>    if (flags & ~ALL_FLAGS)
> @@ -41,3 +41,4 @@ posix_spawnattr_setflags (posix_spawnattr_t *attr, short int flags)
>  
>    return 0;
>  }
> +weak_alias (__posix_spawnattr_setflags, posix_spawnattr_setflags)
> diff --git a/posix/spawnattr_setsigmask.c b/posix/spawnattr_setsigmask.c
> index 7ae81ad470..12c0111af4 100644
> --- a/posix/spawnattr_setsigmask.c
> +++ b/posix/spawnattr_setsigmask.c
> @@ -20,7 +20,7 @@
>  
>  /* Set signal mask for the new process in ATTR to SIGMASK.  */
>  int
> -posix_spawnattr_setsigmask (posix_spawnattr_t *attr,
> +__posix_spawnattr_setsigmask (posix_spawnattr_t *attr,
>  			    const sigset_t *sigmask)
>  {
>    /* Copy the sigset_t data to the user buffer.  */
> @@ -28,3 +28,4 @@ posix_spawnattr_setsigmask (posix_spawnattr_t *attr,
>  
>    return 0;
>  }
> +weak_alias (__posix_spawnattr_setsigmask, posix_spawnattr_setsigmask)
> 

Re: [PATCH 2/3] posix: Use posix_spawn on popen

[Adhemerval Zanella]

Ping.

On 15/09/2018 12:16, Adhemerval Zanella wrote:
> This patch uses posix_spawn on popen instead of fork and execl.  On Linux
> this has the advantage of much lower memory consumption (usually 32 Kb
> minimum for the mmap stack area).
> 
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> 
> 	* libio/iopopen.c (_IO_new_proc_open): use posix_spawn instead of
> 	fork and execl.
> ---
>  ChangeLog       |  3 ++
>  libio/iopopen.c | 97 +++++++++++++++++++++++++++++--------------------
>  2 files changed, 61 insertions(+), 39 deletions(-)
> 
> diff --git a/libio/iopopen.c b/libio/iopopen.c
> index 2eff45b4c8..3cce2e5596 100644
> --- a/libio/iopopen.c
> +++ b/libio/iopopen.c
> @@ -34,7 +34,8 @@
>  #include <not-cancel.h>
>  #include <sys/types.h>
>  #include <sys/wait.h>
> -#include <kernel-features.h>
> +#include <spawn.h>
> +#include <paths.h>
>  
>  struct _IO_proc_file
>  {
> @@ -63,9 +64,8 @@ FILE *
>  _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
>  {
>    int read_or_write;
> -  int parent_end, child_end;
>    int pipe_fds[2];
> -  pid_t child_pid;
> +  int op;
>  
>    int do_read = 0;
>    int do_write = 0;
> @@ -108,59 +108,78 @@ _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
>  
>    if (do_read)
>      {
> -      parent_end = pipe_fds[0];
> -      child_end = pipe_fds[1];
> +      op = 0;
>        read_or_write = _IO_NO_WRITES;
>      }
>    else
>      {
> -      parent_end = pipe_fds[1];
> -      child_end = pipe_fds[0];
> +      op = 1;
>        read_or_write = _IO_NO_READS;
>      }
>  
> -  ((_IO_proc_file *) fp)->pid = child_pid = __fork ();
> -  if (child_pid == 0)
> -    {
> -      int child_std_end = do_read ? 1 : 0;
> -      struct _IO_proc_file *p;
> -
> -      if (child_end != child_std_end)
> -	__dup2 (child_end, child_std_end);
> -      else
> -	/* The descriptor is already the one we will use.  But it must
> -	   not be marked close-on-exec.  Undo the effects.  */
> -	__fcntl (child_end, F_SETFD, 0);
> -      /* POSIX.2:  "popen() shall ensure that any streams from previous
> -         popen() calls that remain open in the parent process are closed
> -	 in the new child process." */
> -      for (p = proc_file_chain; p; p = p->next)
> -	{
> -	  int fd = _IO_fileno ((FILE *) p);
> +  {
> +    posix_spawn_file_actions_t fa;
> +    /* posix_spawn_file_actions_init does not fail.  */
> +    __posix_spawn_file_actions_init (&fa);
>  
> -	  /* If any stream from previous popen() calls has fileno
> -	     child_std_end, it has been already closed by the dup2 syscall
> -	     above.  */
> -	  if (fd != child_std_end)
> -	    __close_nocancel (fd);
> -	}
> +    /* The descriptor is already in the one the child will use.  In this case
> +       it must be moved to another one, otherwise there is no safe way to
> +       remove the close-on-exec flag in the child without creating a FD leak
> +       race in the parent.  */
> +    if (pipe_fds[1 - op] == 1 - op)
> +      {
> +	int tmp = __fcntl (1 - op, F_DUPFD_CLOEXEC, 0);
> +	if (tmp < 0)
> +	  goto spawn_failure;
> +	__close_nocancel (pipe_fds[1 - op]);
> +	pipe_fds[1 - op] = tmp;
> +      }
>  
> -      execl ("/bin/sh", "sh", "-c", command, (char *) 0);
> -      _exit (127);
> -    }
> -  __close_nocancel (child_end);
> -  if (child_pid < 0)
> +    if (__posix_spawn_file_actions_adddup2 (&fa, pipe_fds[1 - op], 1 - op)
> +	!= 0)
> +      goto spawn_failure;
> +
> +    /* POSIX.2: "popen() shall ensure that any streams from previous popen()
> +       calls that remain open in the parent process are closed in the new
> +       child process." */
> +    for (struct _IO_proc_file *p = proc_file_chain; p; p = p->next)
> +      {
> +	int fd = _IO_fileno ((FILE *) p);
> +
> +	/* If any stream from previous popen() calls has fileno
> +	   child_send, it has been already closed by the dup2 syscall
> +	   above.  */
> +	if (fd != 1 - op
> +	    && __posix_spawn_file_actions_addclose (&fa, fd) != 0)
> +	  goto spawn_failure;
> +      }
> +
> +    if (__posix_spawn (&((_IO_proc_file *) fp)->pid, _PATH_BSHELL, &fa, 0,
> +		     (char *const[]){ (char*) "sh", (char*) "-c",
> +		     (char *) command, NULL }, __environ) != 0)
> +      {
> +      spawn_failure:
> +	__posix_spawn_file_actions_destroy (&fa);
> +	__close_nocancel (pipe_fds[1 - op]);
> +	__set_errno (ENOMEM);
> +	return NULL;
> +      }
> +
> +    __posix_spawn_file_actions_destroy (&fa);
> +  }
> +  __close_nocancel (pipe_fds[1 - op]);
> +  if (((_IO_proc_file *) fp)->pid < 0)
>      {
> -      __close_nocancel (parent_end);
> +      __close_nocancel (pipe_fds[op]);
>        return NULL;
>      }
>  
>    if (!do_cloexec)
>      /* Undo the effects of the pipe2 call which set the
>         close-on-exec flag.  */
> -    __fcntl (parent_end, F_SETFD, 0);
> +    __fcntl (pipe_fds[op], F_SETFD, 0);
>  
> -  _IO_fileno (fp) = parent_end;
> +  _IO_fileno (fp) = pipe_fds[op];
>  
>    /* Link into proc_file_chain. */
>  #ifdef _IO_MTSAFE_IO
> 

Re: [PATCH 3/3] posix: Use posix_spawn on system

[Adhemerval Zanella]

On 15/09/2018 12:16, 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.  While reentracy handling does not require
> any direct change of current strategy, cancellation requires a posix_spawn
> to be cancellable.  This is done by adding an internal
> __posix_spawn_cancellable which does not disable cancellation neither
> change process signal mask.
> 
> The cancellation code is also moved to generic implementation and enabled
> only if SIGCANCEL is defined (similar on how the cancellation handler is
> enabled on nptl-init.c).
> 
> Checked on x86_64-linux-gnu and i686-linux-gnu.

A small fix below (I can resend the patch if required).

> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index 85239cedbf..ed5c613e42 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;
> @@ -330,10 +330,14 @@ __spawnix (pid_t * pid, const char *file,
>    if (__glibc_unlikely (stack == MAP_FAILED))
>      return errno;
>  
> -  /* Disable asynchronous cancellation.  */
>    int state;
> -  __libc_ptf_call (__pthread_setcancelstate,
> -                   (PTHREAD_CANCEL_DISABLE, &state), 0);
> +  if ((xflags & SPAWN_XFLAGS_ENABLE_CANCEL) == 0)
> +    {
> +      /* Disable asynchronous cancellation.  */
> +      __libc_ptf_call (__pthread_setcancelstate,
> +		       (PTHREAD_CANCEL_DISABLE, &state), 0);
> +      __libc_signal_block_all (&args.oldmask);
> +    }

In fact I think it would be safer to just enable glibc internal signals
instead of current process mask.  I changed it locally to:

  if (xflags & SPAWN_XFLAGS_ENABLE_CANCEL)
    __libc_signal_block_app (&args.oldmask);
  else
    { 
      /* Disable asynchronous cancellation.  */
      __libc_ptf_call (__pthread_setcancelstate,
                       (PTHREAD_CANCEL_DISABLE, &state), 0);
      __libc_signal_block_all (&args.oldmask);
    }


>  
>    /* Child must set args.err to something non-negative - we rely on
>       the parent and child sharing VM.  */
> @@ -347,8 +351,6 @@ __spawnix (pid_t * pid, const char *file,
>    args.envp = envp;
>    args.xflags = xflags;
>  
> -  __libc_signal_block_all (&args.oldmask);
> -
>    /* The clone flags used will create a new child that will run in the same
>       memory space (CLONE_VM) and the execution of calling thread will be
>       suspend until the child calls execve or _exit.
> @@ -389,9 +391,11 @@ __spawnix (pid_t * pid, const char *file,
>    if ((ec == 0) && (pid != NULL))
>      *pid = new_pid;
>  
> -  __libc_signal_restore_set (&args.oldmask);
> -
> -  __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
> +  if ((xflags & SPAWN_XFLAGS_ENABLE_CANCEL) == 0)
> +    {
> +      __libc_signal_restore_set (&args.oldmask);
> +      __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
> +    }

As as before I changed locally to:

  __libc_signal_restore_set (&args.oldmask);
  if ((xflags & SPAWN_XFLAGS_ENABLE_CANCEL) == 0)
    __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);

Re: [PATCH 2/3] posix: Use posix_spawn on popen

[Adhemerval Zanella]

I also fixed BZ#17490.  Although POSIX pthread_atfork [1] description only
list 'fork' as the function where should issue the atfork handlers and 
popen description [2] states that:

  '[...] shall be *as if* a child process were created within the popen() 
   call using the fork() function [...]' 

Other libc/system seems to follow the idea atfork handlers should not be
issue for popen:

libc/system	| run atfork handles   | notes 
----------------|----------------------|---------------------------------------	
freebsd	master  |        no            | uses vfork
solaris 11	|        no            |
MacOSX (11.13)  |        no            | implemented through posix_spawn syscall
----------------|----------------------|----------------------------------------

And I also agree that, as for posix_spawn and system, popen idea is to spawn 
a different binary so all the POSIX rationale to run the atfork handlers to 
avoid internal process inconsistent are not really required and in some cases
might be unsafe.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/
[2] http://pubs.opengroup.org/onlinepubs/9699919799/

On 17/10/2018 14:11, Adhemerval Zanella wrote:
> Ping.
> 
> On 15/09/2018 12:16, Adhemerval Zanella wrote:
>> This patch uses posix_spawn on popen instead of fork and execl.  On Linux
>> this has the advantage of much lower memory consumption (usually 32 Kb
>> minimum for the mmap stack area).
>>
>> Checked on x86_64-linux-gnu and i686-linux-gnu.
>>
>> 	* libio/iopopen.c (_IO_new_proc_open): use posix_spawn instead of
>> 	fork and execl.
>> ---
>>  ChangeLog       |  3 ++
>>  libio/iopopen.c | 97 +++++++++++++++++++++++++++++--------------------
>>  2 files changed, 61 insertions(+), 39 deletions(-)
>>
>> diff --git a/libio/iopopen.c b/libio/iopopen.c
>> index 2eff45b4c8..3cce2e5596 100644
>> --- a/libio/iopopen.c
>> +++ b/libio/iopopen.c
>> @@ -34,7 +34,8 @@
>>  #include <not-cancel.h>
>>  #include <sys/types.h>
>>  #include <sys/wait.h>
>> -#include <kernel-features.h>
>> +#include <spawn.h>
>> +#include <paths.h>
>>  
>>  struct _IO_proc_file
>>  {
>> @@ -63,9 +64,8 @@ FILE *
>>  _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
>>  {
>>    int read_or_write;
>> -  int parent_end, child_end;
>>    int pipe_fds[2];
>> -  pid_t child_pid;
>> +  int op;
>>  
>>    int do_read = 0;
>>    int do_write = 0;
>> @@ -108,59 +108,78 @@ _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
>>  
>>    if (do_read)
>>      {
>> -      parent_end = pipe_fds[0];
>> -      child_end = pipe_fds[1];
>> +      op = 0;
>>        read_or_write = _IO_NO_WRITES;
>>      }
>>    else
>>      {
>> -      parent_end = pipe_fds[1];
>> -      child_end = pipe_fds[0];
>> +      op = 1;
>>        read_or_write = _IO_NO_READS;
>>      }
>>  
>> -  ((_IO_proc_file *) fp)->pid = child_pid = __fork ();
>> -  if (child_pid == 0)
>> -    {
>> -      int child_std_end = do_read ? 1 : 0;
>> -      struct _IO_proc_file *p;
>> -
>> -      if (child_end != child_std_end)
>> -	__dup2 (child_end, child_std_end);
>> -      else
>> -	/* The descriptor is already the one we will use.  But it must
>> -	   not be marked close-on-exec.  Undo the effects.  */
>> -	__fcntl (child_end, F_SETFD, 0);
>> -      /* POSIX.2:  "popen() shall ensure that any streams from previous
>> -         popen() calls that remain open in the parent process are closed
>> -	 in the new child process." */
>> -      for (p = proc_file_chain; p; p = p->next)
>> -	{
>> -	  int fd = _IO_fileno ((FILE *) p);
>> +  {
>> +    posix_spawn_file_actions_t fa;
>> +    /* posix_spawn_file_actions_init does not fail.  */
>> +    __posix_spawn_file_actions_init (&fa);
>>  
>> -	  /* If any stream from previous popen() calls has fileno
>> -	     child_std_end, it has been already closed by the dup2 syscall
>> -	     above.  */
>> -	  if (fd != child_std_end)
>> -	    __close_nocancel (fd);
>> -	}
>> +    /* The descriptor is already in the one the child will use.  In this case
>> +       it must be moved to another one, otherwise there is no safe way to
>> +       remove the close-on-exec flag in the child without creating a FD leak
>> +       race in the parent.  */
>> +    if (pipe_fds[1 - op] == 1 - op)
>> +      {
>> +	int tmp = __fcntl (1 - op, F_DUPFD_CLOEXEC, 0);
>> +	if (tmp < 0)
>> +	  goto spawn_failure;
>> +	__close_nocancel (pipe_fds[1 - op]);
>> +	pipe_fds[1 - op] = tmp;
>> +      }
>>  
>> -      execl ("/bin/sh", "sh", "-c", command, (char *) 0);
>> -      _exit (127);
>> -    }
>> -  __close_nocancel (child_end);
>> -  if (child_pid < 0)
>> +    if (__posix_spawn_file_actions_adddup2 (&fa, pipe_fds[1 - op], 1 - op)
>> +	!= 0)
>> +      goto spawn_failure;
>> +
>> +    /* POSIX.2: "popen() shall ensure that any streams from previous popen()
>> +       calls that remain open in the parent process are closed in the new
>> +       child process." */
>> +    for (struct _IO_proc_file *p = proc_file_chain; p; p = p->next)
>> +      {
>> +	int fd = _IO_fileno ((FILE *) p);
>> +
>> +	/* If any stream from previous popen() calls has fileno
>> +	   child_send, it has been already closed by the dup2 syscall
>> +	   above.  */
>> +	if (fd != 1 - op
>> +	    && __posix_spawn_file_actions_addclose (&fa, fd) != 0)
>> +	  goto spawn_failure;
>> +      }
>> +
>> +    if (__posix_spawn (&((_IO_proc_file *) fp)->pid, _PATH_BSHELL, &fa, 0,
>> +		     (char *const[]){ (char*) "sh", (char*) "-c",
>> +		     (char *) command, NULL }, __environ) != 0)
>> +      {
>> +      spawn_failure:
>> +	__posix_spawn_file_actions_destroy (&fa);
>> +	__close_nocancel (pipe_fds[1 - op]);
>> +	__set_errno (ENOMEM);
>> +	return NULL;
>> +      }
>> +
>> +    __posix_spawn_file_actions_destroy (&fa);
>> +  }
>> +  __close_nocancel (pipe_fds[1 - op]);
>> +  if (((_IO_proc_file *) fp)->pid < 0)
>>      {
>> -      __close_nocancel (parent_end);
>> +      __close_nocancel (pipe_fds[op]);
>>        return NULL;
>>      }
>>  
>>    if (!do_cloexec)
>>      /* Undo the effects of the pipe2 call which set the
>>         close-on-exec flag.  */
>> -    __fcntl (parent_end, F_SETFD, 0);
>> +    __fcntl (pipe_fds[op], F_SETFD, 0);
>>  
>> -  _IO_fileno (fp) = parent_end;
>> +  _IO_fileno (fp) = pipe_fds[op];
>>  
>>    /* Link into proc_file_chain. */
>>  #ifdef _IO_MTSAFE_IO
>>