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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 24/11/2016 09:10, Dominik Vogt wrote:
> 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];
> 
> ?

This is pretty much what I have proposed in my last patch
iteration [1] (I used your suggestion on accounting the arguments).

POSIX is indeed not specific, but since old execvp{e} implementation
used to support it, I think we should keep supporting it due
compatibility.

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