diff mbox

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

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

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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
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 ^_^  ^_^

-- 

Dominik Vogt
IBM Germany
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;