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

[Stefan Liebler]

On 11/22/2016 02:28 PM, Adhemerval Zanella wrote:
>
>
> On 22/11/2016 08:17, Dominik Vogt wrote:
>> On Mon, Nov 21, 2016 at 09:46:22PM +0100, Andreas Schwab wrote:
>>> On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>>>
>>>> With this change are you ok to push this in?
>>>
>>> Yes, this is ok.
>>
>> No!  The patch writes past the array bounds in the else branch.
>>
>> Ciao
>>
>> Dominik ^_^  ^_^
>>
>
> Since I made a mistake to push this patch, which I apologize, I think
> your previous suggestions is indeed the correct one (patch below).
> Reading again, it indeed seems simpler to just account for arguments
> and not the final 'NULL' since the 'argv + 1' will indeed ignore
> the script name.
>
> 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;
>
>

Hi Adhemerval,

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.

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.

Bye
Stefan