[PATCH] posix: Optimize Linux posix_spawn

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

The current internal posix_spawn symbol for Linux (__spawni) requires
to allocate a dynamic stack based on input arguments to handle the
SPAWN_XFLAGS_USE_PATH internal flag, which re-issue the input binary
as a shell script if execve call return ENOEXEC (to execute shell
scripts with an initial shebang).

This is done only for compatibility mode and the generic case does not
require the extra calculation plus the potential large mmap/munmap
call.  For default case, a pre-defined buffer is sufficed to use on the
clone call instead.

This patch optimizes Linux spawni by allocating a dynamic stack only
for compatibility symbol (SPAWN_XFLAGS_USE_PATH).  For generic case,
a stack allocated buffer is used instead.

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

	* sysdeps/unix/sysv/linux/spawni.c (posix_spawn_args): Remove
	argc member.
	(maybe_script_execute): Remove function.
	(execve_compat, __spawni_clone, __spawnix_compat): New function.
	(__spawni_child): Remove maybe_script_execute call.
	(__spawnix): Remove magic stack slack constant with stack_slack
	identifier.
	(__spawni): Only allocates a variable stack when
	SPAWN_XFLAGS_TRY_SHELL is used.
---
 sysdeps/unix/sysv/linux/spawni.c | 205 ++++++++++++++++++-------------
 1 file changed, 118 insertions(+), 87 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index c1abf3f960..822cce2a77 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -74,6 +74,11 @@
 # define STACK(__stack, __stack_size) (__stack + __stack_size)
 #endif
 
+/* Additional stack size added on required space to execute the clone child
+   function (__spawni_child).  */
+enum {
+  stack_slack = 512
+};
 
 struct posix_spawn_args
 {
@@ -83,35 +88,39 @@ struct posix_spawn_args
   const posix_spawn_file_actions_t *fa;
   const posix_spawnattr_t *restrict attr;
   char *const *argv;
-  ptrdiff_t argc;
   char *const *envp;
   int xflags;
   int err;
 };
 
-/* Older version requires that shell script without shebang definition
-   to be called explicitly using /bin/sh (_PATH_BSHELL).  */
-static void
-maybe_script_execute (struct posix_spawn_args *args)
+/* This is compatibility function required to enable posix_spawn run
+   script without shebang definition for older posix_spawn versions
+   (2.15).  */
+static int
+execve_compat (const char *filename, char *const argv[], char *const envp[])
 {
-  if (SHLIB_COMPAT (libc, GLIBC_2_2, GLIBC_2_15)
-      && (args->xflags & SPAWN_XFLAGS_TRY_SHELL) && errno == ENOEXEC)
+  __execve (filename, argv, envp);
+
+  if (errno == ENOEXEC)
     {
-      char *const *argv = args->argv;
-      ptrdiff_t argc = args->argc;
+      char *const *cargv = argv;
+      ptrdiff_t argc = 0;
+      while (cargv[argc++] != NULL);
 
       /* Construct an argument list for the shell.  */
       char *new_argv[argc + 2];
       new_argv[0] = (char *) _PATH_BSHELL;
-      new_argv[1] = (char *) args->file;
+      new_argv[1] = (char *) filename;
       if (argc > 1)
 	memcpy (new_argv + 2, argv + 1, argc * sizeof (char *));
       else
 	new_argv[2] = NULL;
 
       /* Execute the shell.  */
-      args->exec (new_argv[0], new_argv, args->envp);
+      __execve (new_argv[0], new_argv, envp);
     }
+
+  return -1;
 }
 
 /* Function used in the clone call to setup the signals mask, posix_spawn
@@ -291,11 +300,6 @@ __spawni_child (void *arguments)
 
   args->exec (args->file, args->argv, args->envp);
 
-  /* This is compatibility function required to enable posix_spawn run
-     script without shebang definition for older posix_spawn versions
-     (2.15).  */
-  maybe_script_execute (args);
-
 fail:
   /* errno should have an appropriate non-zero value; otherwise,
      there's a bug in glibc or the kernel.  For lack of an error code
@@ -306,18 +310,61 @@ fail:
   _exit (SPAWN_ERROR);
 }
 
-/* Spawn a new process executing PATH with the attributes describes in *ATTRP.
-   Before running the process perform the actions described in FILE-ACTIONS. */
 static int
-__spawnix (pid_t * pid, const char *file,
-	   const posix_spawn_file_actions_t * file_actions,
-	   const posix_spawnattr_t * attrp, char *const argv[],
-	   char *const envp[], int xflags,
-	   int (*exec) (const char *, char *const *, char *const *))
+__spawni_clone (struct posix_spawn_args *args, void *stack, size_t stack_size,
+		pid_t *pid)
 {
-  pid_t new_pid;
-  struct posix_spawn_args args;
   int ec;
+  pid_t new_pid;
+
+  /* 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.
+
+     Also since the calling thread execution will be suspend, there is not
+     need for CLONE_SETTLS.  Although parent and child share the same TLS
+     namespace, there will be no concurrent access for TLS variables (errno
+     for instance).  */
+  new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size,
+		   CLONE_VM | CLONE_VFORK | SIGCHLD, args);
+
+  /* It needs to collect the case where the auxiliary process was created
+     but failed to execute the file (due either any preparation step or
+     for execve itself).  */
+  if (new_pid > 0)
+    {
+      /* Also, it handles the unlikely case where the auxiliary process was
+	 terminated before calling execve as if it was successfully.  The
+	 args.err is set to 0 as default and changed to a positive value
+	 only in case of failure, so in case of premature termination
+	 due a signal args.err will remain zeroed and it will be up to
+	 caller to actually collect it.  */
+      ec = args->err;
+      if (ec > 0)
+	/* There still an unlikely case where the child is cancelled after
+	   setting args.err, due to a positive error value.  Also there is
+	   possible pid reuse race (where the kernel allocated the same pid
+	   to an unrelated process).  Unfortunately due synchronization
+	   issues where the kernel might not have the process collected
+	   the waitpid below can not use WNOHANG.  */
+	__waitpid (new_pid, NULL, 0);
+    }
+  else
+    ec = -new_pid;
+
+  if ((ec == 0) && (pid != NULL))
+    *pid = new_pid;
+
+  return ec;
+}
+
+/* Allocates a stack using mmap to call clone.  The stack size is based on
+   number of arguments since it would be used on compat mode which may call
+   execvpe/execve_compat.  */
+static int
+__spawnix_compat (struct posix_spawn_args *args, pid_t *pid)
+{
+  char *const *argv = args->argv;
 
   /* To avoid imposing hard limits on posix_spawn{p} the total number of
      arguments is first calculated to allocate a mmap to hold all possible
@@ -340,7 +387,8 @@ __spawnix (pid_t * pid, const char *file,
 	     | ((GL (dl_stack_flags) & PF_X) ? PROT_EXEC : 0));
 
   /* Add a slack area for child's stack.  */
-  size_t argv_size = (argc * sizeof (void *)) + 512;
+  size_t argv_size = (argc * sizeof (void *)) + stack_slack;
+
   /* We need at least a few pages in case the compiler's stack checking is
      enabled.  In some configs, it is known to use at least 24KiB.  We use
      32KiB to be "safe" from anything the compiler might do.  Besides, the
@@ -352,64 +400,61 @@ __spawnix (pid_t * pid, const char *file,
   if (__glibc_unlikely (stack == MAP_FAILED))
     return errno;
 
+  int ec = __spawni_clone (args, stack, stack_size, pid);
+
+  __munmap (stack, stack_size);
+
+  return ec;
+}
+
+/* Spawn a new process executing PATH with the attributes describes in *ATTRP.
+   Before running the process perform the actions described in FILE-ACTIONS. */
+int
+__spawni (pid_t * pid, const char *file,
+	  const posix_spawn_file_actions_t * file_actions,
+	  const posix_spawnattr_t * attrp, char *const argv[],
+	  char *const envp[], int xflags)
+{
+  /* For SPAWN_XFLAGS_TRY_SHELL we need to execute a script even without
+     a shebang.  To accomplish it we pass as callback to __spawni_child
+     __execvpe (which call maybe_script_execute for such case) or
+     execve_compat (which mimics the semantic using execve).  */
+  int (*exec) (const char *, char *const *, char *const *) =
+    xflags & SPAWN_XFLAGS_TRY_SHELL
+    ? xflags & SPAWN_XFLAGS_USE_PATH ? __execvpe  : execve_compat
+    : xflags & SPAWN_XFLAGS_USE_PATH ? __execvpex : __execve;
+
+  /* Child must set args.err to something non-negative - we rely on
+     the parent and child sharing VM.  */
+  struct posix_spawn_args args = {
+    .err = 0,
+    .file = file,
+    .exec = exec,
+    .fa = file_actions,
+    .attr = attrp ? attrp : &(const posix_spawnattr_t) { 0 },
+    .argv = argv,
+    .envp = envp,
+    .xflags = xflags
+  };
+
   /* Disable asynchronous cancellation.  */
   int state;
   __libc_ptf_call (__pthread_setcancelstate,
                    (PTHREAD_CANCEL_DISABLE, &state), 0);
 
-  /* Child must set args.err to something non-negative - we rely on
-     the parent and child sharing VM.  */
-  args.err = 0;
-  args.file = file;
-  args.exec = exec;
-  args.fa = file_actions;
-  args.attr = attrp ? attrp : &(const posix_spawnattr_t) { 0 };
-  args.argv = argv;
-  args.argc = argc;
-  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.
-
-     Also since the calling thread execution will be suspend, there is not
-     need for CLONE_SETTLS.  Although parent and child share the same TLS
-     namespace, there will be no concurrent access for TLS variables (errno
-     for instance).  */
-  new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size,
-		   CLONE_VM | CLONE_VFORK | SIGCHLD, &args);
+  int ec;
 
-  /* It needs to collect the case where the auxiliary process was created
-     but failed to execute the file (due either any preparation step or
-     for execve itself).  */
-  if (new_pid > 0)
+  if (__glibc_likely ((xflags & SPAWN_XFLAGS_TRY_SHELL) == 0))
     {
-      /* Also, it handles the unlikely case where the auxiliary process was
-	 terminated before calling execve as if it was successfully.  The
-	 args.err is set to 0 as default and changed to a positive value
-	 only in case of failure, so in case of premature termination
-	 due a signal args.err will remain zeroed and it will be up to
-	 caller to actually collect it.  */
-      ec = args.err;
-      if (ec > 0)
-	/* There still an unlikely case where the child is cancelled after
-	   setting args.err, due to a positive error value.  Also there is
-	   possible pid reuse race (where the kernel allocated the same pid
-	   to an unrelated process).  Unfortunately due synchronization
-	   issues where the kernel might not have the process collected
-	   the waitpid below can not use WNOHANG.  */
-	__waitpid (new_pid, NULL, 0);
+      /* execvpe allocates at least (NAME_MAX + 1) + PATH_MAX to create the
+	 combination of PATH entry and program name.  */
+      char stack[(NAME_MAX + 1) + PATH_MAX + stack_slack];
+      ec = __spawni_clone (&args, stack, sizeof stack, pid);
     }
   else
-    ec = -new_pid;
-
-  __munmap (stack, stack_size);
-
-  if ((ec == 0) && (pid != NULL))
-    *pid = new_pid;
+    ec = __spawnix_compat (&args, pid);
 
   __libc_signal_restore_set (&args.oldmask);
 
@@ -417,17 +462,3 @@ __spawnix (pid_t * pid, const char *file,
 
   return ec;
 }
-
-/* Spawn a new process executing PATH with the attributes describes in *ATTRP.
-   Before running the process perform the actions described in FILE-ACTIONS. */
-int
-__spawni (pid_t * pid, const char *file,
-	  const posix_spawn_file_actions_t * acts,
-	  const posix_spawnattr_t * attrp, char *const argv[],
-	  char *const envp[], int xflags)
-{
-  /* It uses __execvpex to avoid run ENOEXEC in non compatibility mode (it
-     will be handled by maybe_script_execute).  */
-  return __spawnix (pid, file, acts, attrp, argv, envp, xflags,
-		    xflags & SPAWN_XFLAGS_USE_PATH ? __execvpex :__execve);
-}
-- 
2.17.1