Message ID | 1479734322-28206-1-git-send-email-adhemerval.zanella@linaro.org |
---|---|
State | Dropped |
Headers |
Received: (qmail 12699 invoked by alias); 21 Nov 2016 13:19:01 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 12665 invoked by uid 89); 21 Nov 2016 13:19:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=103, 6, 1036, __strnlen, enforce X-HELO: mail-vk0-f52.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:subject:date:message-id; bh=BgGaKPmVG1mS2kfsc16/wXGYSXEyyXKESN1tyLw3zwk=; b=JIvIf5kDHyamq1SOaZkzDzM9qSYYJ79Zs0O0ad//WcmMivdZRkeRqfFjg2DFeu5HKF cMEEb5N/OfyAv82SX1SrqdMSY1qlO0qbUTlmFJEpBQEW4WuttpzfYqb6Glajl7O/fZfi 3xK4Zk0AmRHqYDDUlm0UjyEnpy+E8fR0OOCX8h7MA9C6xTEayndk7/adyQFfWzk8PUzB SEPjfp+WHR7OYiWpIyL7RqGluXfpNGoMqPO1f8xLeagxA/YlqJjL7BhQu2lsw3fXeGDp RaBIQSeuGN/n/oiW8Oy3yeeYGEj4mO2sR0MqjVfV4rPb2AGrQMge++nRdf0FAklm1grW QZ/A== X-Gm-Message-State: AKaTC01PEPmE0HlwcIvs1W5go+RL/QZKT5So6IP8HWdwg4xRVBIJXIEqR+f03bKzejPEzEDX X-Received: by 10.31.173.144 with SMTP id w138mr6606102vke.153.1479734328975; Mon, 21 Nov 2016 05:18:48 -0800 (PST) From: Adhemerval Zanella <adhemerval.zanella@linaro.org> To: libc-alpha@sourceware.org Subject: [PATCH] Fix writes past the allocated array bounds in execvpe (BZ# 20847) Date: Mon, 21 Nov 2016 11:18:42 -0200 Message-Id: <1479734322-28206-1-git-send-email-adhemerval.zanella@linaro.org> |
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
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.
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.
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! */
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.
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.
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.
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.
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 ^_^ ^_^
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.
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 ^_^ ^_^ >
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 --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);