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

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

On Mon, Nov 21, 2016 at 02:23:27PM -0200, Adhemerval Zanella wrote:
> 
> 
> On 21/11/2016 13:51, Dominik Vogt wrote:
> > On Mon, Nov 21, 2016 at 11:18:42AM -0200, Adhemerval Zanella wrote:
> >> @@ -41,15 +41,16 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[])
> >>    ptrdiff_t argc = 0;
> >>    while (argv[argc++] != NULL)
> > 
> > This loop is broken.  It calculates the wrong value; if there are
> > three arguments, argc will be 4. (See patch below).
> 
> As Andreas pointed out this is intended so memcpy can copy the
> terminating null pointer.

This is simply not true.  The memcpy already adds +1 to argv and
already takes care of the trailing NULL pointer that way.

  memcpy (new_argv + 2, argv + 1, argc * sizeof(char *));
                            ^^^^^

Even if it were true, the variable name should be new_argc not
argc, similar to new_argv vs. argv.  Every normally thinking
programmer expects argc to be the length of argv not the length of
argv plus one.  With the patch you proposed here:

> [1] https://sourceware.org/ml/libc-alpha/2016-11/msg00721.html.

The loop actually calculates the "argc + 1", then subracts one
when using it.  So, the patched code calculates some value that is
not the one needed in *any* place where it is used:

  char *new_argv[argc + 1];
                 ^^^^^^^^
                     |_______ not the calculated argc anyway
  new_argv[0] = (char *) _PATH_BSHELL;
  new_argv[1] = (char *) file;
  if (argc > 1)
           ^^^^
             |_______________ could be simply "argc > 0" or "argc"
                              with the "real" value
+   memcpy (new_argv + 2, argv + 1, (argc - 1) * sizeof(char *));
                                    ^^^^^^^^^^
                                         |
                              not the calculated value, but could
                              be simply "argc" with the "real" value.
  else
    new_argv[2] = NULL;
    ^^^^^^^^^^^^^^^^^^^

Also, the patch doesnt' take care of the "else".  When the current
argc is one, there is no new_argv[2].

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany