diff mbox

[1/3] posix: Remove dynamic memory allocation from execl{e,p}

Message ID 1456146172-12850-2-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella Netto Feb. 22, 2016, 1:02 p.m. UTC 
GLIBC execl{e,p} implementation might use malloc if the total number of i
arguments exceed initial assumption size (1024).  This might lead to
issue in two situations:

1. execl/execle is stated to be async-signal-safe by POSIX [1].  However
   if execl 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 issue malloc internal bad state.

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

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.
---
 posix/Makefile |  2 +-
 posix/execl.c  | 65 ++++++++++++++++---------------------------------------
 posix/execle.c | 66 +++++++++++++++++---------------------------------------
 posix/execlp.c | 68 ++++++++++++++++++++--------------------------------------
 5 files changed, 70 insertions(+), 138 deletions(-)

Comments

Paul Eggert Feb. 25, 2016, 7:27 a.m. UTC | #1 
Adhemerval Zanella wrote:
> +  int argc;
> +  va_list ap;
> +  va_start (ap, arg);
> +  for (argc = 1; va_arg (ap, const char *); argc++)
> +    continue;

With my "no arbitrary limits" hat on, I noticed that this has undefined behavior 
if more than INT_MAX arguments are passed to execl. The existing code is no 
saint in this area (it messes up badly if more than UINT_MAX args are passed), 
but the new code should not make things worse, and we might as well fix the 
UINT_MAX bug while we're at it.

Attached please find a contrived test case illustrating the bug on x86-64. This 
test succeeds on x86-64 now (in that the program prints "execl: Cannot allocate 
memory" and exits with status 0) but could crash with the proposed patch. 
Perhaps you can add this to the glibc test cases while you're at it.
Adhemerval Zanella Netto Feb. 25, 2016, 1:10 p.m. UTC | #2 
On 25-02-2016 04:27, Paul Eggert wrote:
> Adhemerval Zanella wrote:
>> +  int argc;
>> +  va_list ap;
>> +  va_start (ap, arg);
>> +  for (argc = 1; va_arg (ap, const char *); argc++)
>> +    continue;
> 
> With my "no arbitrary limits" hat on, I noticed that this has undefined behavior if more than INT_MAX arguments are passed to execl. The existing code is no saint in this area (it messes up badly if more than UINT_MAX args are passed), but the new code should not make things worse, and we might as well fix the UINT_MAX bug while we're at it.
> 

AFAIK the C standard defines the main entrypoint argc as signed int, so 
I think it is indeed undefined behaviour if you intend to call a program
with more than INT_MAX arguments.


> Attached please find a contrived test case illustrating the bug on x86-64. This test succeeds on x86-64 now (in that the program prints "execl: Cannot allocate memory" and exits with status 0) but could crash with the proposed patch. Perhaps you can add this to the glibc test cases while you're at it.
>
Zack Weinberg Feb. 25, 2016, 2:41 p.m. UTC | #3 
On Thu, Feb 25, 2016 at 8:10 AM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> On 25-02-2016 04:27, Paul Eggert wrote:
>>
>> With my "no arbitrary limits" hat on, I noticed that this has
>> undefined behavior if more than INT_MAX arguments are passed to execl.
>
> AFAIK the C standard defines the main entrypoint argc as signed int, so
> I think it is indeed undefined behaviour if you intend to call a program
> with more than INT_MAX arguments.

The C standard does indeed define main's first argument as signed int,
but I don't think that's a sufficient reason to allow exec* to exhibit
UB for too many arguments.  POSIX does *not* call this case out as UB.

I think detecting this situation and failing with errno==E2BIG would
be appropriate.  E2BIG is already specified as the error code for
exceeding ARG_MAX bytes of argument list + environment.

zw
diff mbox

Patch

diff --git a/posix/Makefile b/posix/Makefile
index 4e90a95..55f4f31 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -221,8 +221,8 @@  CFLAGS-fexecve.os = -fomit-frame-pointer
 CFLAGS-execv.os = -fomit-frame-pointer
 CFLAGS-execle.os = -fomit-frame-pointer
 CFLAGS-execl.os = -fomit-frame-pointer
-CFLAGS-execvp.os = -fomit-frame-pointer
 CFLAGS-execlp.os = -fomit-frame-pointer
+CFLAGS-execvp.os = -fomit-frame-pointer
 
 tstgetopt-ARGS = -a -b -cfoobar --required foobar --optional=bazbug \
 		--none random --col --color --colour
diff --git a/posix/execl.c b/posix/execl.c
index 102d19d..cd40ff8 100644
--- a/posix/execl.c
+++ b/posix/execl.c
@@ -16,58 +16,31 @@ 
    <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;
+  va_list ap;
+  va_start (ap, arg);
+  for (argc = 1; va_arg (ap, const char *); argc++)
+    continue;
+  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..17fc1d4 100644
--- a/posix/execle.c
+++ b/posix/execle.c
@@ -17,57 +17,31 @@ 
 
 #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;
+  va_list ap;
+  va_start (ap, arg);
+  for (argc = 1; va_arg (ap, const char *); argc++)
+    continue;
+  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..0f789af 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,26 @@ 
 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;
+  va_list ap;
+  va_start (ap, arg);
+  for (argc = 1; va_arg (ap, const char *); argc++)
+    continue;
+  va_end (ap);
+
+  /* Although posix does not state execlp as an async-safe function
+     it can not use malloc to allocate the arguments since it might
+     be used in a vfork scenario and it may lead to malloc internal
+     bad state.  */
+  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)