Re: [PATCH v2] Fix writes past the allocated array bounds in execvpe (BZ#20847)

[Dominik Vogt <vogt at linux dot vnet dot ibm dot com>]

On Wed, Nov 23, 2016 at 01:28:16PM +0100, Stefan Liebler wrote:
> On 11/22/2016 02:28 PM, Adhemerval Zanella wrote:
> >diff --git a/posix/execvpe.c b/posix/execvpe.c
> >index 7cdb06a..cf167d0 100644
> >--- a/posix/execvpe.c
> >+++ b/posix/execvpe.c
> >@@ -38,8 +38,8 @@
> > static void
> > maybe_script_execute (const char *file, char *const argv[], char *const envp[])
> > {
> >-  ptrdiff_t argc = 0;
> >-  while (argv[argc++] != NULL)
> >+  ptrdiff_t argc;
> >+  for (argc = 0; argv[argc] != NULL; argc++)
> >     {
> >       if (argc == INT_MAX - 1)
> >        {
> >@@ -50,11 +50,11 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[])
> >
> >   /* Construct an argument list for the shell.  It will contain at minimum 3
> >      arguments (current shell, script, and an ending NULL.  */
> >-  char *new_argv[argc + 1];
> >+  char *new_argv[2 + argc];
> >   new_argv[0] = (char *) _PATH_BSHELL;
> >   new_argv[1] = (char *) file;
> >   if (argc > 1)
> >-    memcpy (new_argv + 2, argv + 1, (argc - 1) * sizeof(char *));
> >+    memcpy (new_argv + 2, argv + 1, argc * sizeof(char *));
> >   else
> >     new_argv[2] = NULL;
> >
> >

> there is an additional special case.
> If someone passes an argv array with argv[0] = NULL, then argc is
> zero and new_argv contains two elements but the else path writes
> NULL to new_argv[2], which is past the allocated array bounds.

Sigh, seems to be virtually impossible to get this right.
 
> I don't know if this case is allowed at all.
> According to the man-page:
> The first argument, by convention, should point to the
> filename associated with the file being executed.

Stefan and me tried to understand the Posix spec for the exec
function family, and it's not exactly clear to us whether a NULL
argument list is a usage error or not.  However, since it's
possible to support that case, and funny applications may rely on
it, so __execvpe should probably accept that case.

Maybe fix it with

  char *new_argv[(argc > 0) ? 2 + argc: 3];

?


Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany