[PATCH] posix: Remove dynamic memory allocation from execl{e,p}

[PATCH] posix: Remove dynamic memory allocation from execl{e,p}

[Adhemerval Zanella]

GLIBC execl{e,p} implementation might use malloc if the total number of
arguments exceeds initial assumption size (1024).  This might lead to
issues in two situations:

1. execl/execle is stated to be async-signal-safe by POSIX [1].  However
   if it is used in a signal handler with a large argument set (that
   may call malloc internally) and the resulting call fails, it might
   lead malloc in the program in a bad state.

2. If the functions are used in a vfork/clone(VFORK) situation it also
   might break internal malloc state from parent.

This patch fixes it by using stack allocation instead.  It fixes
BZ#19534.

One caveat is current stack allocation allocation limit the patch is using
is based on previous posix_spawn{p}/execvpe comments to use internal
__MAX_ALLOCA_CUTOFF definition.  It is an arbitrary value that limits
total argument handlings to 8192 for 32-bits and 4096 for 64-bits.
I think it would be better to increase this value for higher threshold,
either by increasing the __MAX_ALLOCA_CUTOFF value or using a different
strategy to limit stack allocation on exec functions.

Tested on x86_64.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html

	[BZ #19534]
	* posix/execl.c (execl): Remove dynamic memory allocation.
	* posix/execle.c (execle): Likewise.
	* posix/execlp.c (execlp): Likewise.
---
 ChangeLog      |  5 +++++
 posix/execl.c  | 70 ++++++++++++++++++++-------------------------------------
 posix/execle.c | 71 +++++++++++++++++++++-------------------------------------
 posix/execlp.c | 69 ++++++++++++++++++++------------------------------------
 4 files changed, 78 insertions(+), 137 deletions(-)

diff --git a/posix/execl.c b/posix/execl.c
index 102d19d..26c28e1 100644
--- a/posix/execl.c
+++ b/posix/execl.c
@@ -16,58 +16,36 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <unistd.h>
+#include <errno.h>
 #include <stdarg.h>
-#include <stddef.h>
-#include <stdlib.h>
-#include <string.h>
-
-#include <stackinfo.h>
-
+#include <sys/param.h>
 
 /* Execute PATH with all arguments after PATH until
    a NULL pointer and environment from `environ'.  */
 int
 execl (const char *path, const char *arg, ...)
 {
-#define INITIAL_ARGV_MAX 1024
-  size_t argv_max = INITIAL_ARGV_MAX;
-  const char *initial_argv[INITIAL_ARGV_MAX];
-  const char **argv = initial_argv;
-  va_list args;
-
-  argv[0] = arg;
-
-  va_start (args, arg);
-  unsigned int i = 0;
-  while (argv[i++] != NULL)
-    {
-      if (i == argv_max)
-	{
-	  argv_max *= 2;
-	  const char **nptr = realloc (argv == initial_argv ? NULL : argv,
-				       argv_max * sizeof (const char *));
-	  if (nptr == NULL)
-	    {
-	      if (argv != initial_argv)
-		free (argv);
-	      va_end (args);
-	      return -1;
-	    }
-	  if (argv == initial_argv)
-	    /* We have to copy the already filled-in data ourselves.  */
-	    memcpy (nptr, argv, i * sizeof (const char *));
-
-	  argv = nptr;
-	}
-
-      argv[i] = va_arg (args, const char *);
-    }
-  va_end (args);
-
-  int ret = __execve (path, (char *const *) argv, __environ);
-  if (argv != initial_argv)
-    free (argv);
-
-  return ret;
+  int argc;
+  int limit = MIN (NCARGS, __MAX_ALLOCA_CUTOFF / sizeof (char *));
+  va_list ap;
+  va_start (ap, arg);
+  for (argc = 1; va_arg (ap, const char *); argc++)
+    if (argc+1 >= limit)
+      {
+	errno = E2BIG;
+	return -1;
+      }
+  va_end (ap);
+
+  int i;
+  char *argv[argc+1];
+  va_start (ap, arg);
+  argv[0] = (char*) arg;
+  for (i = 1; i < argc; i++)
+     argv[i] = va_arg (ap, char *);
+  argv[i] = NULL;
+  va_end (ap);
+
+  return __execve (path, argv, __environ);
 }
 libc_hidden_def (execl)
diff --git a/posix/execle.c b/posix/execle.c
index 8edc03a..79c13e3 100644
--- a/posix/execle.c
+++ b/posix/execle.c
@@ -17,57 +17,36 @@
 
 #include <unistd.h>
 #include <stdarg.h>
-#include <stddef.h>
-#include <stdlib.h>
-#include <string.h>
-
-#include <stackinfo.h>
+#include <errno.h>
+#include <sys/param.h>
 
 /* Execute PATH with all arguments after PATH until a NULL pointer,
    and the argument after that for environment.  */
 int
 execle (const char *path, const char *arg, ...)
 {
-#define INITIAL_ARGV_MAX 1024
-  size_t argv_max = INITIAL_ARGV_MAX;
-  const char *initial_argv[INITIAL_ARGV_MAX];
-  const char **argv = initial_argv;
-  va_list args;
-  argv[0] = arg;
-
-  va_start (args, arg);
-  unsigned int i = 0;
-  while (argv[i++] != NULL)
-    {
-      if (i == argv_max)
-	{
-	  argv_max *= 2;
-	  const char **nptr = realloc (argv == initial_argv ? NULL : argv,
-				       argv_max * sizeof (const char *));
-	  if (nptr == NULL)
-	    {
-	      if (argv != initial_argv)
-		free (argv);
-	      va_end (args);
-	      return -1;
-	    }
-	  if (argv == initial_argv)
-	    /* We have to copy the already filled-in data ourselves.  */
-	    memcpy (nptr, argv, i * sizeof (const char *));
-
-	  argv = nptr;
-	}
-
-      argv[i] = va_arg (args, const char *);
-    }
-
-  const char *const *envp = va_arg (args, const char *const *);
-  va_end (args);
-
-  int ret = __execve (path, (char *const *) argv, (char *const *) envp);
-  if (argv != initial_argv)
-    free (argv);
-
-  return ret;
+  int argc;
+  int limit = MIN (NCARGS, __MAX_ALLOCA_CUTOFF / sizeof (char *));
+  va_list ap;
+  va_start (ap, arg);
+  for (argc = 1; va_arg (ap, const char *); argc++)
+    if (argc+1 >= limit)
+      {
+	errno = E2BIG;
+	return -1;
+      }
+  va_end (ap);
+
+  int i;
+  char *argv[argc+1];
+  char **envp;
+  va_start (ap, arg);
+  argv[0] = (char*) arg;
+  for (i = 1; i < argc; i++)
+     argv[i] = va_arg (ap, char *);
+  envp = va_arg (ap, char **);
+  va_end (ap);
+
+  return __execve (path, argv, envp);
 }
 libc_hidden_def (execle)
diff --git a/posix/execlp.c b/posix/execlp.c
index 6700994..a4b603c 100644
--- a/posix/execlp.c
+++ b/posix/execlp.c
@@ -17,11 +17,8 @@
 
 #include <unistd.h>
 #include <stdarg.h>
-#include <stddef.h>
-#include <stdlib.h>
-#include <string.h>
-
-#include <stackinfo.h>
+#include <errno.h>
+#include <sys/param.h>
 
 /* Execute FILE, searching in the `PATH' environment variable if
    it contains no slashes, with all arguments after FILE until a
@@ -29,45 +26,27 @@
 int
 execlp (const char *file, const char *arg, ...)
 {
-#define INITIAL_ARGV_MAX 1024
-  size_t argv_max = INITIAL_ARGV_MAX;
-  const char *initial_argv[INITIAL_ARGV_MAX];
-  const char **argv = initial_argv;
-  va_list args;
-
-  argv[0] = arg;
-
-  va_start (args, arg);
-  unsigned int i = 0;
-  while (argv[i++] != NULL)
-    {
-      if (i == argv_max)
-	{
-	  argv_max *= 2;
-	  const char **nptr = realloc (argv == initial_argv ? NULL : argv,
-				       argv_max * sizeof (const char *));
-	  if (nptr == NULL)
-	    {
-	      if (argv != initial_argv)
-		free (argv);
-	      va_end (args);
-	      return -1;
-	    }
-	  if (argv == initial_argv)
-	    /* We have to copy the already filled-in data ourselves.  */
-	    memcpy (nptr, argv, i * sizeof (const char *));
-
-	  argv = nptr;
-	}
-
-      argv[i] = va_arg (args, const char *);
-    }
-  va_end (args);
-
-  int ret = execvp (file, (char *const *) argv);
-  if (argv != initial_argv)
-    free (argv);
-
-  return ret;
+  int argc;
+  int limit = MIN (NCARGS, __MAX_ALLOCA_CUTOFF / sizeof (char *));
+  va_list ap;
+  va_start (ap, arg);
+  for (argc = 1; va_arg (ap, const char *); argc++)
+    if (argc+1 >= limit)
+      {
+	errno = E2BIG;
+	return -1;
+      }
+  va_end (ap);
+
+  int i;
+  char *argv[argc+1];
+  va_start (ap, arg);
+  argv[0] = (char*) arg;
+  for (i = 1; i < argc; i++)
+     argv[i] = va_arg (ap, char *);
+  argv[i] = NULL;
+  va_end (ap);
+
+  return __execvpe (file, argv, __environ);
 }
 libc_hidden_def (execlp)
-- 
1.9.1

Re: [PATCH] posix: Remove dynamic memory allocation from execl{e,p}

[Florian Weimer]

On 01/29/2016 02:53 PM, Adhemerval Zanella wrote:
> GLIBC execl{e,p} implementation might use malloc if the total number of
> arguments exceeds initial assumption size (1024).  This might lead to
> issues in two situations:
> 
> 1. execl/execle is stated to be async-signal-safe by POSIX [1].  However
>    if it is used in a signal handler with a large argument set (that
>    may call malloc internally) and the resulting call fails, it might
>    lead malloc in the program in a bad state.
> 
> 2. If the functions are used in a vfork/clone(VFORK) situation it also
>    might break internal malloc state from parent.
> 
> This patch fixes it by using stack allocation instead.  It fixes
> BZ#19534.

I would rather see a variant of struct scratch_buffer which uses mmap
for the fallback allocation, and use it here, rather than imposing
arbitrary limits.  It's not clear to me at all if __MAX_ALLOCA_CUTOFF is
a reasonable value inside a signal handler.

Florian

Re: [PATCH] posix: Remove dynamic memory allocation from execl{e,p}

[Zack Weinberg]

On Fri, Jan 29, 2016 at 8:53 AM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> GLIBC execl{e,p} implementation might use malloc if the total number of
> arguments exceeds initial assumption size (1024).  This might lead to
> issues in two situations: [...]

I think it would help the discussion if you could outline the
conditions under which these functions need to allocate memory in the
first place.  Does it happen all the time, or only when falling back
to non-#! shell script invocation, or what?

zw

Re: [PATCH] posix: Remove dynamic memory allocation from execl{e,p}

[Joseph Myers]

On Fri, 29 Jan 2016, Florian Weimer wrote:

> I would rather see a variant of struct scratch_buffer which uses mmap
> for the fallback allocation, and use it here, rather than imposing
> arbitrary limits.  It's not clear to me at all if __MAX_ALLOCA_CUTOFF is

Agreed.  glibc functions should avoid arbitrary limits, which means they 
should not fail simply because the limit for stack allocation was 
exceeded.

(This does mean the functions need to do deallocation of the memory 
allocated with mmap, if the underlying execve fails.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] posix: Remove dynamic memory allocation from execl{e,p}

[Adhemerval Zanella]

> Em 29 de jan de 2016, às 13:00, Zack Weinberg <zackw@panix.com> escreveu:
> 
> On Fri, Jan 29, 2016 at 8:53 AM, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>> GLIBC execl{e,p} implementation might use malloc if the total number of
>> arguments exceeds initial assumption size (1024).  This might lead to
>> issues in two situations: [...]
> 
> I think it would help the discussion if you could outline the
> conditions under which these functions need to allocate memory in the
> first place.  Does it happen all the time, or only when falling back
> to non-#! shell script invocation, or what?
> 
> zw

Excel{e,p} first try to use a stack allocate buffer for up to 1024 arguments and try to allocate dynamic memory for large values (by trying to allocate memory to 2048 argument and doubling each time it does no fulfill the requirements).

These functions does not try to run shell script with shebang (different than execpe).

Re: [PATCH] posix: Remove dynamic memory allocation from execl{e,p}

[Adhemerval Zanella]

> Em 29 de jan de 2016, às 11:58, Florian Weimer <fweimer@redhat.com> escreveu:
> 
>> On 01/29/2016 02:53 PM, Adhemerval Zanella wrote:
>> GLIBC execl{e,p} implementation might use malloc if the total number of
>> arguments exceeds initial assumption size (1024).  This might lead to
>> issues in two situations:
>> 
>> 1. execl/execle is stated to be async-signal-safe by POSIX [1].  However
>>   if it is used in a signal handler with a large argument set (that
>>   may call malloc internally) and the resulting call fails, it might
>>   lead malloc in the program in a bad state.
>> 
>> 2. If the functions are used in a vfork/clone(VFORK) situation it also
>>   might break internal malloc state from parent.
>> 
>> This patch fixes it by using stack allocation instead.  It fixes
>> BZ#19534.
> 
> I would rather see a variant of struct scratch_buffer which uses mmap
> for the fallback allocation, and use it here, rather than imposing
> arbitrary limits.  It's not clear to me at all if __MAX_ALLOCA_CUTOFF is
> a reasonable value inside a signal handler.
> 
> Florian
> 

I agree that __MAX_ALLOCA_CUTOFF is arbitrary and does not solve anything (for instance it does not impose constraints for deep stacked calls).

Also mmap usage does not solve the second case (vfork/clone) in case of exec success since the call won't unmap the region.

Re: [PATCH] posix: Remove dynamic memory allocation from execl{e,p}

[Adhemerval Zanella]

On 29-01-2016 13:43, Joseph Myers wrote:
> On Fri, 29 Jan 2016, Florian Weimer wrote:
> 
>> I would rather see a variant of struct scratch_buffer which uses mmap
>> for the fallback allocation, and use it here, rather than imposing
>> arbitrary limits.  It's not clear to me at all if __MAX_ALLOCA_CUTOFF is
> 
> Agreed.  glibc functions should avoid arbitrary limits, which means they 
> should not fail simply because the limit for stack allocation was 
> exceeded.
> 
> (This does mean the functions need to do deallocation of the memory 
> allocated with mmap, if the underlying execve fails.)
> 

The problem is the constraint associated with the functions where only
stack allocation make sense. For all exec function family, where they
can be called either through a signal handler or in vfork child, we
can not really on dynamic memory allocation (either through malloc or
directly by mmap).

And I also see that __MAX_ALLOCA_CUTOFF is a limit that does not make
any sense in this context: it is just an arbitrary value that will
potentially limit the total function usability (total number of 
arguments). It also won't prevent stack overflow, since the call
can be deeply nested and an allocation smaller than __MAX_ALLOCA_CUTOFF 
can trigger the overflow.

My view is for such function is just try to allocate the buffer on
stack and allow it fails through invalid access in case of buffer
overflow.

Re: [PATCH] posix: Remove dynamic memory allocation from execl{e,p}

[Joseph Myers]

On Fri, 29 Jan 2016, Adhemerval Zanella wrote:

> My view is for such function is just try to allocate the buffer on
> stack and allow it fails through invalid access in case of buffer
> overflow.

I'd say just trying the allocation is OK in this case (if stack allocation 
is all that's permitted by the function semantics, and bearing in mind 
that current Linux versions determine ARG_MAX dynamically based on the 
stack limit) *if* it's allocated in a way that guarantees failure if it's 
too large (i.e. touching each page in turn) rather than potentially 
overflowing into unrelated memory.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] posix: Remove dynamic memory allocation from execl{e,p}

[Adhemerval Zanella]

On 29-01-2016 15:45, Joseph Myers wrote:
> On Fri, 29 Jan 2016, Adhemerval Zanella wrote:
> 
>> My view is for such function is just try to allocate the buffer on
>> stack and allow it fails through invalid access in case of buffer
>> overflow.
> 
> I'd say just trying the allocation is OK in this case (if stack allocation 
> is all that's permitted by the function semantics, and bearing in mind 
> that current Linux versions determine ARG_MAX dynamically based on the 
> stack limit) *if* it's allocated in a way that guarantees failure if it's 
> too large (i.e. touching each page in turn) rather than potentially 
> overflowing into unrelated memory.
>

That is the idea, the buffer is allocated to exactly fit the required
arguments (different that current algorithm that double each iteration).

For posix_spawn{p} I used a different strategy for the clone(VFORK),
which I think it is slight better but does impose some constraints:
allocate a mmap(STACK) with default architecture stack size and set
is as the stack for children. I think it is better than possible clobber
a stack allocated buffer from parent.

Re: [PATCH] posix: Remove dynamic memory allocation from execl{e,p}

[Florian Weimer]

On 01/29/2016 04:52 PM, Adhemerval Zanella wrote:

> Also mmap usage does not solve the second case (vfork/clone) in case of exec success since the call won't unmap the region.

I've written a test case to confirm this.  How unfortunate.

Are there different clone flags we could use?  Probably not, because the
goal is to share the page tables.

For posix_spawn, this doesn't really matter because posix_spawn could
allocate and deallocate the mapping.

Florian