diff mbox

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

Message ID 1454343665-1706-2-git-send-email-adhemerval.zanella@linaro.org
State Superseded
Headers show

Commit Message

Adhemerval Zanella Feb. 1, 2016, 4:21 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.

Changes from previous version:

- Remove arbitrary limit on stack allocation for argument handling
  (it is arbitrary and does no impose any limit since it does not 
  consider current stack size neither stack size limit).

[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/execl.c  | 65 +++++++++++++++++----------------------------------------
 posix/execle.c | 66 ++++++++++++++++++----------------------------------------
 posix/execlp.c | 64 +++++++++++++++++---------------------------------------
 4 files changed, 65 insertions(+), 137 deletions(-)

-- 
1.9.1

Comments

Adhemerval Zanella Feb. 1, 2016, 5:18 p.m. UTC | #1 
On 01-02-2016 14:52, Joseph Myers wrote:
> On Mon, 1 Feb 2016, Adhemerval Zanella wrote:

> 

>> +  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;

> 

> I don't see how you're ensuring this stack allocation is safe (i.e. if 

> it's too big, it doesn't corrupt memory that's in use by other threads).  

> Can't it jump beyond any guard page and start overwriting other memory, 

> possibly in use by another thread, before reaching unmapped memory?  I'd 

> expect safe large stack allocations (as with -fstack-check) to need to 

> touch the pages in the right order (and doing that safely probably means 

> using -fstack-check).

> 


Right, it is not ensuring the safeness. Is '-fstack-check' the suffice 
option to ensure it or do we need a more strict one?
Adhemerval Zanella Feb. 1, 2016, 6:09 p.m. UTC | #2 
On 01-02-2016 15:53, Joseph Myers wrote:
> On Mon, 1 Feb 2016, Adhemerval Zanella wrote:

> 

>> Right, it is not ensuring the safeness. Is '-fstack-check' the suffice 

>> option to ensure it or do we need a more strict one?

> 

> I think it's the right option, but don't know if it works reliably across 

> supported architectures and GCC versions (or, at least, does not generate 

> wrong code even if the checks aren't fully safe in all cases) - it's not 

> very widely used.

> 


I think we can use this option, since it supported on gcc (and I also do not
have any other better option in mind that fits in these function memory
constraints).
Adhemerval Zanella Feb. 2, 2016, 12:47 p.m. UTC | #3 
On 02-02-2016 09:24, Florian Weimer wrote:
> On 02/01/2016 06:18 PM, Adhemerval Zanella wrote:

> 

>> Right, it is not ensuring the safeness. Is '-fstack-check' the suffice 

>> option to ensure it or do we need a more strict one?

> 

> In my tests, the initial stack banging probe is sometimes more than a

> page away from the current stack pointer, so it does not look safe to me.


I am not aware of a better option, if any, to avoid stack overflow in case of
a exec function call with large arguments.  Also, check at least on x86_64
I do see trying to overflow the stack with new implementation does force a
segfault on the stack protector code (it tries to orq a memory beyond stack).

> 

> For posix_spawn, it's probably simpler for now to compute the shell

> invocation in the parent.  That is, perform two vforks in case the first

> execve fails.

> 

> Florian

> 


I do not think it would require to clone(VFORK) twice in parent: we can
calculate the total argument size prior any call and calculate the
total stack to mmap based on this plus a slack for possible local
variables (128 or 256 bytes or even higher). I will add some latency
in any posix_spawn{p} case.

Another option is to just create a guard page in the end of the allocated
stack page (as pthread_create does) to force a segfaults in case of
a stack allocation higher. However, as for execl using stack-check will
also force a segfault in case of stack overflow in this case.
Adhemerval Zanella Feb. 10, 2016, 4:36 p.m. UTC | #4 
On 09-02-2016 11:30, Szabolcs Nagy wrote:
> On 09/02/16 11:35, Richard Henderson wrote:

>> On 02/08/2016 08:28 AM, Rasmus Villemoes wrote:

>>> On Tue, Feb 02 2016, Rich Felker <dalias@libc.org> wrote:

>>>

>>>> On Mon, Feb 01, 2016 at 04:52:15PM +0000, Joseph Myers wrote:

>>>>> On Mon, 1 Feb 2016, Adhemerval Zanella wrote:

>>>>>

>>>>>> +  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;

>>>>>

>>>>> I don't see how you're ensuring this stack allocation is safe (i.e. if

>>>>> it's too big, it doesn't corrupt memory that's in use by other threads).

>>>>

>>>> There's no obligation to. If you pass something like a million

>>>> arguments to a variadic function, the compiler will generate code in

>>>> the caller that overflows the stack before the callee is even reached.

>>>> The size of the vla used in execl is exactly the same size as the

>>>> argument block on the stack used for passing arguments to execl from

>>>> its caller, and it's nobody's fault but the programmer's if this is

>>>> way too big. It's not a runtime variable.

>>>

>>> This is true, and maybe it's not worth the extra complication, but if

>>> we're willing to make arch-specific versions of execl and execle we can

>>> avoid the double stack use and the time spent copying the argv

>>> array. That won't remove the possible stack overflow, of course, but

>>> then it'll in all likelihood happen in the user code and not glibc.

>>

>> I like the idea.  It's a quality of implementation issue, wherein by re-using the data that's (mostly) on the

>> stack already we don't need twice again the amount of stack space for any given call.

>>

>> I think that Adhemerval's patch should go in as a default implementation, and various targets can implement the

>> assembly as desired.

>>

>> I've tested the following on x86_64 and i686.  I've compile-tested for x32 (but need a more complete x32

>> installation for testing), and alpha (testing is just slow).

>>

>> Thoughts?

>>

> 

> i think it is a hard to maintain, nasty hack

> 

> is execl stack usage the bottleneck somewhere?

> 

> to improve the stack usage in glibc, the extreme

> wasteful cases should be fixed first.

> (crypt uses >100k, strtod uses ???, etc)

> 

> e.g. musl guarantees hard limits on libc stack usage

> and execl is one of the (two) exceptions where it just

> seems useless to do so (you cannot do it portably anyway).

> 



I agree with Szabolcs and I also see these kind of asm specific
implementation also adds platform different behaviour which I also
think we should avoid to add within GLIBC.

The only doubt with my patch I have now is if it is worth to add
the -fstack-check or not. Florian has raised some questioning about
its efficacy and I am not sure how well it is supported on all 
architectures.
diff mbox

Patch

diff --git a/posix/execl.c b/posix/execl.c
index 102d19d..8b8a324 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..1a0c9ee 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..a0e1859 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,22 @@ 
 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);
+
+  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)