diff mbox

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

Message ID ba4c0f7f-35af-a733-71c1-c035b3c3a495@linaro.org
State Dropped
Headers

Commit Message

Adhemerval Zanella Nov. 21, 2016, 8:37 p.m. UTC 
  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:

With this change are you ok to push this in?

Comments

Andreas Schwab Nov. 21, 2016, 8:46 p.m. UTC | #1 
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.

Andreas.
Dominik Vogt Nov. 22, 2016, 10:17 a.m. UTC | #2 
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 ^_^  ^_^
diff mbox

Patch

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;