diff mbox

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

Message ID 1479734322-28206-1-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella Netto Nov. 21, 2016, 1:18 p.m. UTC 
This patch fixes an invalid write out or stack allocated buffer in
2 places at execvpe implementation:

  1. On 'maybe_script_execute' function where it allocates the new
     argument list and it does not account that a minimum of argc
     plus 3 elements (default shell path, script name, arguments,
     and ending null pointer) should be considered.  The straightforward
     fix is just to take account of the correct list size.

  2. On '__execvpe' where the executable file name lenght may not
     account for ending '\0' and thus subsequent path creation may
     write past array bounds because it requires to add the terminating
     null.  The fix is to change how to calculate the executable name
     size to add the final '\0' and adjust the rest of the code
     accordingly.

As described in GCC bug report 78433 [1], these issues were masked off by
GCC because it allocated several bytes more than necessary so that many
off-by-one bugs went unnoticed.

Checked on x86_64 with a latest GCC (7.0.0 20161121) with -O3 on CFLAGS.

	* posix/execvpe.c (maybe_script_execute): Remove write past allocated
	array bounds.
	(__execvpe): Likewise.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78433
---
 ChangeLog       |  7 +++++++
 posix/execvpe.c | 17 +++++++++++------
 2 files changed, 18 insertions(+), 6 deletions(-)

Comments

Andreas Schwab Nov. 21, 2016, 1:33 p.m. UTC | #1 
On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> This patch fixes an invalid write out or stack allocated buffer in
> 2 places at execvpe implementation:
>
>   1. On 'maybe_script_execute' function where it allocates the new
>      argument list and it does not account that a minimum of argc
>      plus 3 elements (default shell path, script name, arguments,
>      and ending null pointer) should be considered.  The straightforward
>      fix is just to take account of the correct list size.
>
>   2. On '__execvpe' where the executable file name lenght may not
>      account for ending '\0' and thus subsequent path creation may
>      write past array bounds because it requires to add the terminating
>      null.  The fix is to change how to calculate the executable name
>      size to add the final '\0' and adjust the rest of the code
>      accordingly.
>
> As described in GCC bug report 78433 [1], these issues were masked off by
> GCC because it allocated several bytes more than necessary so that many
> off-by-one bugs went unnoticed.

Did the bugs already exist before commit 1eb8930608?

> +  if (((file_len-1) > NAME_MAX)

Spaces around operator and remove the redundant parens.

Andreas.
Adhemerval Zanella Netto Nov. 21, 2016, 2 p.m. UTC | #2 
On 21/11/2016 11:33, Andreas Schwab wrote:
> On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> 
>> This patch fixes an invalid write out or stack allocated buffer in
>> 2 places at execvpe implementation:
>>
>>   1. On 'maybe_script_execute' function where it allocates the new
>>      argument list and it does not account that a minimum of argc
>>      plus 3 elements (default shell path, script name, arguments,
>>      and ending null pointer) should be considered.  The straightforward
>>      fix is just to take account of the correct list size.
>>
>>   2. On '__execvpe' where the executable file name lenght may not
>>      account for ending '\0' and thus subsequent path creation may
>>      write past array bounds because it requires to add the terminating
>>      null.  The fix is to change how to calculate the executable name
>>      size to add the final '\0' and adjust the rest of the code
>>      accordingly.
>>
>> As described in GCC bug report 78433 [1], these issues were masked off by
>> GCC because it allocated several bytes more than necessary so that many
>> off-by-one bugs went unnoticed.
> 
> Did the bugs already exist before commit 1eb8930608?

For first issue I see so, since it allocates the argument list as:

 64           /* Count the arguments.  */
 65           int argc = 0;
 66           while (argv[argc++])
 67             ;
 68           size_t len = (argc + 1) * sizeof (char *);
 69           char **script_argv;
 70           void *ptr = NULL;
 71           if (__libc_use_alloca (len))
 72             script_argv = alloca (len);
 73           else
 74             script_argv = ptr = malloc (len);

Taking in consideration only argument list plus one but then writing
argument list plus 2 position on 'scripts_argv'.  The old implementation
does not fail on tst-vfork3 with newer GCC and I did not investigate why.

For second issue, old implementation seems to get the correct size:

 87       size_t pathlen;
 88       size_t alloclen = 0;
 89       char *path = getenv ("PATH");
 90       if (path == NULL)
 91         {
 92           pathlen = confstr (_CS_PATH, (char *) NULL, 0);
 93           alloclen = pathlen + 1;
 94         }
 95       else
 96         pathlen = strlen (path);
 97 
 98       size_t len = strlen (file) + 1;
 99       alloclen += pathlen + len + 1;
100 
101       char *name;
102       char *path_malloc = NULL;
103       if (__libc_use_alloca (alloclen))
104         name = alloca (alloclen);
105       else
106         {
107           path_malloc = name = malloc (alloclen);
108           if (name == NULL)
109             return -1;
110         }

It calculates the final buffer to pass on execve as path length (without
'\0') plus executable length plus 1 for final '\0' and an extra 1 for
'/'.

> 
>> +  if (((file_len-1) > NAME_MAX)
> 
> Spaces around operator and remove the redundant parens.

Ack, I will change it commit.
Stefan Liebler Nov. 21, 2016, 2:16 p.m. UTC | #3 
On 11/21/2016 02:18 PM, Adhemerval Zanella wrote:
> This patch fixes an invalid write out or stack allocated buffer in
> 2 places at execvpe implementation:
>
>   1. On 'maybe_script_execute' function where it allocates the new
>      argument list and it does not account that a minimum of argc
>      plus 3 elements (default shell path, script name, arguments,
>      and ending null pointer) should be considered.  The straightforward
>      fix is just to take account of the correct list size.
>
>   2. On '__execvpe' where the executable file name lenght may not
>      account for ending '\0' and thus subsequent path creation may
>      write past array bounds because it requires to add the terminating
>      null.  The fix is to change how to calculate the executable name
>      size to add the final '\0' and adjust the rest of the code
>      accordingly.
>
> As described in GCC bug report 78433 [1], these issues were masked off by
> GCC because it allocated several bytes more than necessary so that many
> off-by-one bugs went unnoticed.
>
> Checked on x86_64 with a latest GCC (7.0.0 20161121) with -O3 on CFLAGS.
>
> 	* posix/execvpe.c (maybe_script_execute): Remove write past allocated
> 	array bounds.
> 	(__execvpe): Likewise.
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78433
> ---
>  ChangeLog       |  7 +++++++
>  posix/execvpe.c | 17 +++++++++++------
>  2 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/posix/execvpe.c b/posix/execvpe.c
> index d933f9c..bd535b1 100644
> --- a/posix/execvpe.c
> +++ b/posix/execvpe.c
> @@ -41,15 +41,16 @@ 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];
>    new_argv[0] = (char *) _PATH_BSHELL;
>    new_argv[1] = (char *) file;
>    if (argc > 1)
Is this fix correct?
memcpy reads behind NULL in argv!
Assume:
argv[0]="tst.sh"; argv[1]="abc"; argv[2]=NULL;
=> argc=3
char *new_argv[3 + 2];
memcpy (new_argv + 2, argv + 1, 3 * sizeof(char *))
=>
new_argv[0]=_PATH_BSHELL
new_argv[1]=file
new_argv[2]=argv[1]; /* "abc"  */
new_argv[3]=argv[2]; /* NULL  */
new_argv[4]=argv[3]; /* => reads from behind NULL-element in argv!  */
Andreas Schwab Nov. 21, 2016, 2:17 p.m. UTC | #4 
On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> For first issue I see so, since it allocates the argument list as:
>
>  64           /* Count the arguments.  */
>  65           int argc = 0;
>  66           while (argv[argc++])
>  67             ;
>  68           size_t len = (argc + 1) * sizeof (char *);
>  69           char **script_argv;
>  70           void *ptr = NULL;
>  71           if (__libc_use_alloca (len))
>  72             script_argv = alloca (len);
>  73           else
>  74             script_argv = ptr = malloc (len);
>
> Taking in consideration only argument list plus one but then writing
> argument list plus 2 position on 'scripts_argv'.

But the old scripts_argv never writes to new_argv[argc+1].  Here, argc
is already including the NULL in the old argv, and scripts_argv only has
to prepend one new argument (and replace the old argv[0]).

Andreas.
Adhemerval Zanella Netto Nov. 21, 2016, 2:43 p.m. UTC | #5 
On 21/11/2016 12:17, Andreas Schwab wrote:
> On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> 
>> For first issue I see so, since it allocates the argument list as:
>>
>>  64           /* Count the arguments.  */
>>  65           int argc = 0;
>>  66           while (argv[argc++])
>>  67             ;
>>  68           size_t len = (argc + 1) * sizeof (char *);
>>  69           char **script_argv;
>>  70           void *ptr = NULL;
>>  71           if (__libc_use_alloca (len))
>>  72             script_argv = alloca (len);
>>  73           else
>>  74             script_argv = ptr = malloc (len);
>>
>> Taking in consideration only argument list plus one but then writing
>> argument list plus 2 position on 'scripts_argv'.
> 
> But the old scripts_argv never writes to new_argv[argc+1].  Here, argc
> is already including the NULL in the old argv, and scripts_argv only has
> to prepend one new argument (and replace the old argv[0]).

Right, but then I think it incur in another issue where the resulting new
argument variable would not contain a final NULL.
Andreas Schwab Nov. 21, 2016, 2:48 p.m. UTC | #6 
On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> On 21/11/2016 12:17, Andreas Schwab wrote:
>> On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>> 
>>> For first issue I see so, since it allocates the argument list as:
>>>
>>>  64           /* Count the arguments.  */
>>>  65           int argc = 0;
>>>  66           while (argv[argc++])
>>>  67             ;
>>>  68           size_t len = (argc + 1) * sizeof (char *);
>>>  69           char **script_argv;
>>>  70           void *ptr = NULL;
>>>  71           if (__libc_use_alloca (len))
>>>  72             script_argv = alloca (len);
>>>  73           else
>>>  74             script_argv = ptr = malloc (len);
>>>
>>> Taking in consideration only argument list plus one but then writing
>>> argument list plus 2 position on 'scripts_argv'.
>> 
>> But the old scripts_argv never writes to new_argv[argc+1].  Here, argc
>> is already including the NULL in the old argv, and scripts_argv only has
>> to prepend one new argument (and replace the old argv[0]).
>
> Right, but then I think it incur in another issue where the resulting new
> argument variable would not contain a final NULL.

scripts_argv first copies argv[argc-1], which is the final NULL.

Andreas.
Adhemerval Zanella Netto Nov. 21, 2016, 2:50 p.m. UTC | #7 
On 21/11/2016 12:48, Andreas Schwab wrote:
> On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> 
>> On 21/11/2016 12:17, Andreas Schwab wrote:
>>> On Nov 21 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>>>
>>>> For first issue I see so, since it allocates the argument list as:
>>>>
>>>>  64           /* Count the arguments.  */
>>>>  65           int argc = 0;
>>>>  66           while (argv[argc++])
>>>>  67             ;
>>>>  68           size_t len = (argc + 1) * sizeof (char *);
>>>>  69           char **script_argv;
>>>>  70           void *ptr = NULL;
>>>>  71           if (__libc_use_alloca (len))
>>>>  72             script_argv = alloca (len);
>>>>  73           else
>>>>  74             script_argv = ptr = malloc (len);
>>>>
>>>> Taking in consideration only argument list plus one but then writing
>>>> argument list plus 2 position on 'scripts_argv'.
>>>
>>> But the old scripts_argv never writes to new_argv[argc+1].  Here, argc
>>> is already including the NULL in the old argv, and scripts_argv only has
>>> to prepend one new argument (and replace the old argv[0]).
>>
>> Right, but then I think it incur in another issue where the resulting new
>> argument variable would not contain a final NULL.
> 
> scripts_argv first copies argv[argc-1], which is the final NULL.

Indeed, nevermind my previous comments then.
Dominik Vogt Nov. 21, 2016, 3:51 p.m. UTC | #8 
On Mon, Nov 21, 2016 at 11:18:42AM -0200, Adhemerval Zanella wrote:
> This patch fixes an invalid write out or stack allocated buffer in
> 2 places at execvpe implementation:
> 
>   1. On 'maybe_script_execute' function where it allocates the new
>      argument list and it does not account that a minimum of argc
>      plus 3 elements (default shell path, script name, arguments,
>      and ending null pointer) should be considered.  The straightforward
>      fix is just to take account of the correct list size.
> 
>   2. On '__execvpe' where the executable file name lenght may not
>      account for ending '\0' and thus subsequent path creation may
>      write past array bounds because it requires to add the terminating
>      null.  The fix is to change how to calculate the executable name
>      size to add the final '\0' and adjust the rest of the code
>      accordingly.
> 
> As described in GCC bug report 78433 [1], these issues were masked off by
> GCC because it allocated several bytes more than necessary so that many
> off-by-one bugs went unnoticed.
> 
> Checked on x86_64 with a latest GCC (7.0.0 20161121) with -O3 on CFLAGS.
> 
> 	* posix/execvpe.c (maybe_script_execute): Remove write past allocated
> 	array bounds.
> 	(__execvpe): Likewise.
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78433
> ---
>  ChangeLog       |  7 +++++++
>  posix/execvpe.c | 17 +++++++++++------
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/posix/execvpe.c b/posix/execvpe.c
> index d933f9c..bd535b1 100644
> --- a/posix/execvpe.c
> +++ b/posix/execvpe.c
> @@ -41,15 +41,16 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[])
>    ptrdiff_t argc = 0;
>    while (argv[argc++] != NULL)

This loop is broken.  It calculates the wrong value; if there are
three arguments, argc will be 4. (See patch below).

>      {
> -      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];

This doesn't fix all problems.  The memcpy below still copies junk
from argv[1 + argc], which is past the end of argv, to new_argv.
Fixing the loop fixes this problem.

>    new_argv[0] = (char *) _PATH_BSHELL;
>    new_argv[1] = (char *) file;
>    if (argc > 1)

> @@ -91,10 +92,11 @@ __execvpe (const char *file, char *const argv[], char *const envp[])
>    /* Although GLIBC does not enforce NAME_MAX, we set it as the maximum
>       size to avoid unbounded stack allocation.  Same applies for
>       PATH_MAX.  */
> -  size_t file_len = __strnlen (file, NAME_MAX + 1);
> +  size_t file_len = __strnlen (file, NAME_MAX) + 1;

I think the existing buffer size calculation in __execvpe() is
fine after all, because of the "+ 1" in the next line:

>    size_t path_len = __strnlen (path, PATH_MAX - 1) + 1;
                                                     ^^^^

>  
> -  if ((file_len > NAME_MAX)
> +  /* NAME_MAX does not include the terminating null character.  */
> +  if (((file_len-1) > NAME_MAX)
>        || !__libc_alloca_cutoff (path_len + file_len + 1))
>      {
>        errno = ENAMETOOLONG;
> @@ -103,6 +105,9 @@ __execvpe (const char *file, char *const argv[], char *const envp[])
>  
>    const char *subp;
>    bool got_eacces = false;
> +  /* The resulting string maximum size would be potentially a entry
> +     in PATH plus '/' (path_len + 1) and then the the resulting file name
> +     plus '\0' (file_len since it already accounts for the '\0').  */
>    char buffer[path_len + file_len + 1];
>    for (const char *p = path; ; p = subp)
>      {
> @@ -123,7 +128,7 @@ __execvpe (const char *file, char *const argv[], char *const envp[])
>           execute.  */
>        char *pend = mempcpy (buffer, p, subp - p);
>        *pend = '/';
> -      memcpy (pend + (p < subp), file, file_len + 1);
> +      memcpy (pend + (p < subp), file, file_len);
>  
>        __execve (buffer, argv, envp);

Alternative change:

--- snip ---
--- a/posix/execvpe.c
+++ b/posix/execvpe.c
@@ -38,10 +38,10 @@
 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)
       	{
@@ -49,7 +49,7 @@ maybe_script_execute (const char *file, char
*const argv[], ch
     }

   /* Construct an argument list for the shell.  */
-  char *new_argv[argc + 1];
+  char *new_argv[2 + argc];
   new_argv[0] = (char *) _PATH_BSHELL;
   new_argv[1] = (char *) file;
   if (argc > 1)
--- snip ---

(Note the reversed order from "argc + 1" to "2 + argc" which is
supposed to make it clear what the "2" is for.  Or maybe even

   2 + argc - 1 + 1
  ^^^        ^^^ ^^^
   |          |   |___ terminating NULL
   |          |_______ all but one elements of argv copied
   |__________________ first two elements "_PATH_SHELL" and "file"

Ciao

Dominik ^_^  ^_^
Andreas Schwab Nov. 21, 2016, 4:05 p.m. UTC | #9 
On Nov 21 2016, Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:

>> diff --git a/posix/execvpe.c b/posix/execvpe.c
>> index d933f9c..bd535b1 100644
>> --- a/posix/execvpe.c
>> +++ b/posix/execvpe.c
>> @@ -41,15 +41,16 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[])
>>    ptrdiff_t argc = 0;
>>    while (argv[argc++] != NULL)
>
> This loop is broken.  It calculates the wrong value; if there are
> three arguments, argc will be 4. (See patch below).

This is intented, it computes the new argc.

Andreas.
Adhemerval Zanella Netto Nov. 21, 2016, 4:23 p.m. UTC | #10 
On 21/11/2016 13:51, Dominik Vogt wrote:
> On Mon, Nov 21, 2016 at 11:18:42AM -0200, Adhemerval Zanella wrote:
>> This patch fixes an invalid write out or stack allocated buffer in
>> 2 places at execvpe implementation:
>>
>>   1. On 'maybe_script_execute' function where it allocates the new
>>      argument list and it does not account that a minimum of argc
>>      plus 3 elements (default shell path, script name, arguments,
>>      and ending null pointer) should be considered.  The straightforward
>>      fix is just to take account of the correct list size.
>>
>>   2. On '__execvpe' where the executable file name lenght may not
>>      account for ending '\0' and thus subsequent path creation may
>>      write past array bounds because it requires to add the terminating
>>      null.  The fix is to change how to calculate the executable name
>>      size to add the final '\0' and adjust the rest of the code
>>      accordingly.
>>
>> As described in GCC bug report 78433 [1], these issues were masked off by
>> GCC because it allocated several bytes more than necessary so that many
>> off-by-one bugs went unnoticed.
>>
>> Checked on x86_64 with a latest GCC (7.0.0 20161121) with -O3 on CFLAGS.
>>
>> 	* posix/execvpe.c (maybe_script_execute): Remove write past allocated
>> 	array bounds.
>> 	(__execvpe): Likewise.
>>
>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78433
>> ---
>>  ChangeLog       |  7 +++++++
>>  posix/execvpe.c | 17 +++++++++++------
>>  2 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/posix/execvpe.c b/posix/execvpe.c
>> index d933f9c..bd535b1 100644
>> --- a/posix/execvpe.c
>> +++ b/posix/execvpe.c
>> @@ -41,15 +41,16 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[])
>>    ptrdiff_t argc = 0;
>>    while (argv[argc++] != NULL)
> 
> This loop is broken.  It calculates the wrong value; if there are
> three arguments, argc will be 4. (See patch below).

As Andreas pointed out this is intended so memcpy can copy the
terminating null pointer.

> 
>>      {
>> -      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];
> 
> This doesn't fix all problems.  The memcpy below still copies junk
> from argv[1 + argc], which is past the end of argv, to new_argv.
> Fixing the loop fixes this problem.

Yes and Stefan Liebler pointed out earlier and I added an
extra snippet to handle it [1]

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

> 
>>    new_argv[0] = (char *) _PATH_BSHELL;
>>    new_argv[1] = (char *) file;
>>    if (argc > 1)
> 
>> @@ -91,10 +92,11 @@ __execvpe (const char *file, char *const argv[], char *const envp[])
>>    /* Although GLIBC does not enforce NAME_MAX, we set it as the maximum
>>       size to avoid unbounded stack allocation.  Same applies for
>>       PATH_MAX.  */
>> -  size_t file_len = __strnlen (file, NAME_MAX + 1);
>> +  size_t file_len = __strnlen (file, NAME_MAX) + 1;
> 
> I think the existing buffer size calculation in __execvpe() is
> fine after all, because of the "+ 1" in the next line:
> 
>>    size_t path_len = __strnlen (path, PATH_MAX - 1) + 1;
>                                                      ^^^^
> 

This addresses only the '/' that will appended after path handling
in the loop.  The extra '\0' for the program executable name still
need to be added and '__strnlen (file, NAME_MAX + 1)' potentially
does not take it in account.

That's why I have a added a comment before the buffer allocation
explaining from where the allocation came from.

>>  
>> -  if ((file_len > NAME_MAX)
>> +  /* NAME_MAX does not include the terminating null character.  */
>> +  if (((file_len-1) > NAME_MAX)
>>        || !__libc_alloca_cutoff (path_len + file_len + 1))
>>      {
>>        errno = ENAMETOOLONG;
>> @@ -103,6 +105,9 @@ __execvpe (const char *file, char *const argv[], char *const envp[])
>>  
>>    const char *subp;
>>    bool got_eacces = false;
>> +  /* The resulting string maximum size would be potentially a entry
>> +     in PATH plus '/' (path_len + 1) and then the the resulting file name
>> +     plus '\0' (file_len since it already accounts for the '\0').  */
>>    char buffer[path_len + file_len + 1];
>>    for (const char *p = path; ; p = subp)
>>      {
>> @@ -123,7 +128,7 @@ __execvpe (const char *file, char *const argv[], char *const envp[])
>>           execute.  */
>>        char *pend = mempcpy (buffer, p, subp - p);
>>        *pend = '/';
>> -      memcpy (pend + (p < subp), file, file_len + 1);
>> +      memcpy (pend + (p < subp), file, file_len);
>>  
>>        __execve (buffer, argv, envp);
> 
> Alternative change:
> 
> --- snip ---
> --- a/posix/execvpe.c
> +++ b/posix/execvpe.c
> @@ -38,10 +38,10 @@
>  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)
>        	{
> @@ -49,7 +49,7 @@ maybe_script_execute (const char *file, char
> *const argv[], ch
>      }
> 
>    /* Construct an argument list for the shell.  */
> -  char *new_argv[argc + 1];
> +  char *new_argv[2 + argc];
>    new_argv[0] = (char *) _PATH_BSHELL;
>    new_argv[1] = (char *) file;
>    if (argc > 1)
> --- snip ---
> 
> (Note the reversed order from "argc + 1" to "2 + argc" which is
> supposed to make it clear what the "2" is for.  Or maybe even
> 
>    2 + argc - 1 + 1
>   ^^^        ^^^ ^^^
>    |          |   |___ terminating NULL
>    |          |_______ all but one elements of argv copied
>    |__________________ first two elements "_PATH_SHELL" and "file"
> 
> Ciao
> 
> Dominik ^_^  ^_^
>
Dominik Vogt Nov. 22, 2016, 9:55 a.m. UTC | #11 
On Mon, Nov 21, 2016 at 02:23:27PM -0200, Adhemerval Zanella wrote:
> 
> 
> On 21/11/2016 13:51, Dominik Vogt wrote:
> > On Mon, Nov 21, 2016 at 11:18:42AM -0200, Adhemerval Zanella wrote:
> >> @@ -41,15 +41,16 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[])
> >>    ptrdiff_t argc = 0;
> >>    while (argv[argc++] != NULL)
> > 
> > This loop is broken.  It calculates the wrong value; if there are
> > three arguments, argc will be 4. (See patch below).
> 
> As Andreas pointed out this is intended so memcpy can copy the
> terminating null pointer.

This is simply not true.  The memcpy already adds +1 to argv and
already takes care of the trailing NULL pointer that way.

  memcpy (new_argv + 2, argv + 1, argc * sizeof(char *));
                            ^^^^^

Even if it were true, the variable name should be new_argc not
argc, similar to new_argv vs. argv.  Every normally thinking
programmer expects argc to be the length of argv not the length of
argv plus one.  With the patch you proposed here:

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

The loop actually calculates the "argc + 1", then subracts one
when using it.  So, the patched code calculates some value that is
not the one needed in *any* place where it is used:

  char *new_argv[argc + 1];
                 ^^^^^^^^
                     |_______ not the calculated argc anyway
  new_argv[0] = (char *) _PATH_BSHELL;
  new_argv[1] = (char *) file;
  if (argc > 1)
           ^^^^
             |_______________ could be simply "argc > 0" or "argc"
                              with the "real" value
+   memcpy (new_argv + 2, argv + 1, (argc - 1) * sizeof(char *));
                                    ^^^^^^^^^^
                                         |
                              not the calculated value, but could
                              be simply "argc" with the "real" value.
  else
    new_argv[2] = NULL;
    ^^^^^^^^^^^^^^^^^^^

Also, the patch doesnt' take care of the "else".  When the current
argc is one, there is no new_argv[2].

Ciao

Dominik ^_^  ^_^
diff mbox

Patch

diff --git a/posix/execvpe.c b/posix/execvpe.c
index d933f9c..bd535b1 100644
--- a/posix/execvpe.c
+++ b/posix/execvpe.c
@@ -41,15 +41,16 @@  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];
   new_argv[0] = (char *) _PATH_BSHELL;
   new_argv[1] = (char *) file;
   if (argc > 1)
@@ -91,10 +92,11 @@  __execvpe (const char *file, char *const argv[], char *const envp[])
   /* Although GLIBC does not enforce NAME_MAX, we set it as the maximum
      size to avoid unbounded stack allocation.  Same applies for
      PATH_MAX.  */
-  size_t file_len = __strnlen (file, NAME_MAX + 1);
+  size_t file_len = __strnlen (file, NAME_MAX) + 1;
   size_t path_len = __strnlen (path, PATH_MAX - 1) + 1;
 
-  if ((file_len > NAME_MAX)
+  /* NAME_MAX does not include the terminating null character.  */
+  if (((file_len-1) > NAME_MAX)
       || !__libc_alloca_cutoff (path_len + file_len + 1))
     {
       errno = ENAMETOOLONG;
@@ -103,6 +105,9 @@  __execvpe (const char *file, char *const argv[], char *const envp[])
 
   const char *subp;
   bool got_eacces = false;
+  /* The resulting string maximum size would be potentially a entry
+     in PATH plus '/' (path_len + 1) and then the the resulting file name
+     plus '\0' (file_len since it already accounts for the '\0').  */
   char buffer[path_len + file_len + 1];
   for (const char *p = path; ; p = subp)
     {
@@ -123,7 +128,7 @@  __execvpe (const char *file, char *const argv[], char *const envp[])
          execute.  */
       char *pend = mempcpy (buffer, p, subp - p);
       *pend = '/';
-      memcpy (pend + (p < subp), file, file_len + 1);
+      memcpy (pend + (p < subp), file, file_len);
 
       __execve (buffer, argv, envp);