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

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

On 21/11/2016 18:10, Andreas Schwab wrote:
> On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> 
>> diff --git a/posix/execvpe.c b/posix/execvpe.c
>> index d933f9c..96a12bf5 100644
>> --- a/posix/execvpe.c
>> +++ b/posix/execvpe.c
>> @@ -41,19 +41,20 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[])
>>    ptrdiff_t argc = 0;
>>    while (argv[argc++] != NULL)
>>      {
>> -      if (argc == INT_MAX - 1)
>> +      if (argc == INT_MAX - 2)
>>  	{
>>  	  errno = E2BIG;
>>  	  return;
>>  	}
>>      }
>>  
>> -  /* Construct an argument list for the shell.  */
>> -  char *new_argv[argc + 1];
>> +  /* 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 + 2];
> 
> The array is now always one element too big, unless execvpe was called
> with argv[0] == NULL.

You are right, I keep forgetting 'argc' in this context also contains the
script name itself.  The memcpy adjustment is indeed the only fix required
for this part:

diff --git a/posix/execvpe.c b/posix/execvpe.c
index d933f9c..7cdb06a 100644
--- a/posix/execvpe.c
+++ b/posix/execvpe.c
@@ -48,12 +48,13 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[])
        }
     }

-  /* Construct an argument list for the shell.  */
+  /* 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];
   new_argv[0] = (char *) _PATH_BSHELL;
   new_argv[1] = (char *) file;
   if (argc > 1)
-    memcpy (new_argv + 2, argv + 1, argc * sizeof(char *));
+    memcpy (new_argv + 2, argv + 1, (argc - 1) * sizeof(char *));
   else
     new_argv[2] = NULL;

With this change are you ok to push this in?