diff mbox

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

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

Commit Message

Adhemerval Zanella Netto Feb. 26, 2016, 1:56 p.m. UTC 
GLIBC execl{e,p} implementation might use malloc if the total number of
arguments exceed 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 execl is used in a signal handler with a large argument set (that
   may call malloc internally) and if 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  | 69 ++++++++++++++++++++++-----------------------------------
 posix/execle.c | 70 +++++++++++++++++++++++-----------------------------------
 posix/execlp.c | 67 ++++++++++++++++++++++---------------------------------
 5 files changed, 89 insertions(+), 126 deletions(-)

Comments

Paul Eggert Feb. 26, 2016, 6:47 p.m. UTC | #1 
On 02/26/2016 05:56 AM, Adhemerval Zanella wrote:
> +  for (i = 1; i < argc; i++)
> +     argv[i] = va_arg (ap, char *);
> +  argv[i] = NULL;

Change "i < argc" to "i <= argc" and remove the "argv[i] = NULL;", as 
that's a bit simpler and faster.

> +  int i;
> +  char *argv[argc + 1];
> +  char **envp;
> +  va_start (ap, arg);
> +  argv[0] = (char *) arg;
> +  for (i = 1; i <= argc; i++)

This sort of thing has undefined behavior on x86-64 if argc == INT_MAX. 
You can fix this by changing the type of argc and of i from int to 
ptrdiff_t.

> +      if (argc == INT_MAX)
>   	{
> +	  errno = E2BIG;
> +	  return -1;
>   	}

Doesn't that have undefined behavior? My impression from C11 is that 
since the function has called va_start it must call va_end before returning.

> +      continue;
>       }

That 'continue;' is redundant and should be removed.
Adhemerval Zanella Netto Feb. 26, 2016, 7:40 p.m. UTC | #2 
On 26-02-2016 15:47, Paul Eggert wrote:
> On 02/26/2016 05:56 AM, Adhemerval Zanella wrote:
>> +  for (i = 1; i < argc; i++)
>> +     argv[i] = va_arg (ap, char *);
>> +  argv[i] = NULL;
> 
> Change "i < argc" to "i <= argc" and remove the "argv[i] = NULL;", as that's a bit simpler and faster.

I added to make it explicit, I will change that.

> 
>> +  int i;
>> +  char *argv[argc + 1];
>> +  char **envp;
>> +  va_start (ap, arg);
>> +  argv[0] = (char *) arg;
>> +  for (i = 1; i <= argc; i++)
> 
> This sort of thing has undefined behavior on x86-64 if argc == INT_MAX. You can fix this by changing the type of argc and of i from int to ptrdiff_t.
> 

Indeed, but afaik this code won't execute if argc == INT_MAX (the argument
sanity check will make the function with E2BIG).

>> +      if (argc == INT_MAX)
>>       {
>> +      errno = E2BIG;
>> +      return -1;
>>       }
> 
> Doesn't that have undefined behavior? My impression from C11 is that since the function has called va_start it must call va_end before returning.
> 

Yes, I will remove it.

>> +      continue;
>>       }
> 
> That 'continue;' is redundant and should be removed.

I will remove it.
Paul Eggert Feb. 26, 2016, 8:07 p.m. UTC | #3 
On 02/26/2016 11:40 AM, Adhemerval Zanella wrote:
> Indeed, but afaik this code won't execute if argc == INT_MAX (the argument
> sanity check will make the function with E2BIG).

No, because we add 1 to argc after testing that it is not INT_MAX. So 
argc can equal INT_MAX after that.
Adhemerval Zanella Netto Feb. 26, 2016, 8:13 p.m. UTC | #4 
On 26-02-2016 17:07, Paul Eggert wrote:
> On 02/26/2016 11:40 AM, Adhemerval Zanella wrote:
>> Indeed, but afaik this code won't execute if argc == INT_MAX (the argument
>> sanity check will make the function with E2BIG).
> 
> No, because we add 1 to argc after testing that it is not INT_MAX. So argc can equal INT_MAX after that.

Right, I am assuming you are referring to the potentially overflow in
addition, now the stack allocation.  I will change to ptrdiff_t.
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..5584a4c 100644
--- a/posix/execl.c
+++ b/posix/execl.c
@@ -16,58 +16,43 @@ 
    <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)
+  int argc;
+  va_list ap;
+  va_start (ap, arg);
+  for (argc = 1; va_arg (ap, const char *); argc++)
     {
-      if (i == argv_max)
+      if (argc == INT_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;
+	  errno = E2BIG;
+	  return -1;
 	}
-
-      argv[i] = va_arg (args, const char *);
+      continue;
     }
-  va_end (args);
-
-  int ret = __execve (path, (char *const *) argv, __environ);
-  if (argv != initial_argv)
-    free (argv);
-
-  return ret;
+  va_end (ap);
+
+  /* Avoid dynamic memory allocation due two main issues:
+     1. The function should be async-signal-safe and a running on a signal
+        handler with a fail outcome might lead to malloc bad state.
+     2. It might be used in a vfork/clone(VFORK) scenario where using
+        malloc also might lead to 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 __execve (path, argv, __environ);
 }
 libc_hidden_def (execl)
diff --git a/posix/execle.c b/posix/execle.c
index 8edc03a..66b5738 100644
--- a/posix/execle.c
+++ b/posix/execle.c
@@ -17,57 +17,43 @@ 
 
 #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)
+  int argc;
+  va_list ap;
+  va_start (ap, arg);
+  for (argc = 1; va_arg (ap, const char *); argc++)
     {
-      if (i == argv_max)
+      if (argc == INT_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;
+	  errno = E2BIG;
+	  return -1;
 	}
-
-      argv[i] = va_arg (args, const char *);
+      continue;
     }
-
-  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;
+  va_end (ap);
+
+  /* Avoid dynamic memory allocation due two main issues:
+     1. The function should be async-signal-safe and a running on a signal
+        handler with a fail outcome might lead to malloc bad state.
+     2. It might be used in a vfork/clone(VFORK) scenario where using
+        malloc also might lead to internal bad state.  */
+  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..7b84ffa 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,33 @@ 
 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)
+  int argc;
+  va_list ap;
+  va_start (ap, arg);
+  for (argc = 1; va_arg (ap, const char *); argc++)
     {
-      if (i == argv_max)
+      if (argc == INT_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;
+	  errno = E2BIG;
+	  return -1;
 	}
-
-      argv[i] = va_arg (args, const char *);
+      continue;
     }
-  va_end (args);
-
-  int ret = execvp (file, (char *const *) argv);
-  if (argv != initial_argv)
-    free (argv);
-
-  return ret;
+  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)