| Message ID | b45e848c-b47c-fa9e-6873-7b191efb982e@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 11/22/2016 02:28 PM, Adhemerval Zanella wrote: > > > On 22/11/2016 08:17, Dominik Vogt wrote: >> 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 ^_^ ^_^ >> > > Since I made a mistake to push this patch, which I apologize, I think > your previous suggestions is indeed the correct one (patch below). > Reading again, it indeed seems simpler to just account for arguments > and not the final 'NULL' since the 'argv + 1' will indeed ignore > the script name. > > diff --git a/posix/execvpe.c b/posix/execvpe.c > index 7cdb06a..cf167d0 100644 > --- a/posix/execvpe.c > +++ b/posix/execvpe.c > @@ -38,8 +38,8 @@ > 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) > { > @@ -50,11 +50,11 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[]) > > /* 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]; > + char *new_argv[2 + argc]; > new_argv[0] = (char *) _PATH_BSHELL; > new_argv[1] = (char *) file; > if (argc > 1) > - memcpy (new_argv + 2, argv + 1, (argc - 1) * sizeof(char *)); > + memcpy (new_argv + 2, argv + 1, argc * sizeof(char *)); > else > new_argv[2] = NULL; > > Hi Adhemerval, there is an additional special case. If someone passes an argv array with argv[0] = NULL, then argc is zero and new_argv contains two elements but the else path writes NULL to new_argv[2], which is past the allocated array bounds. I don't know if this case is allowed at all. According to the man-page: The first argument, by convention, should point to the filename associated with the file being executed. Bye Stefan
On Wed, Nov 23, 2016 at 01:28:16PM +0100, Stefan Liebler wrote: > On 11/22/2016 02:28 PM, Adhemerval Zanella wrote: > >diff --git a/posix/execvpe.c b/posix/execvpe.c > >index 7cdb06a..cf167d0 100644 > >--- a/posix/execvpe.c > >+++ b/posix/execvpe.c > >@@ -38,8 +38,8 @@ > > 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) > > { > >@@ -50,11 +50,11 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[]) > > > > /* 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]; > >+ char *new_argv[2 + argc]; > > new_argv[0] = (char *) _PATH_BSHELL; > > new_argv[1] = (char *) file; > > if (argc > 1) > >- memcpy (new_argv + 2, argv + 1, (argc - 1) * sizeof(char *)); > >+ memcpy (new_argv + 2, argv + 1, argc * sizeof(char *)); > > else > > new_argv[2] = NULL; > > > > > there is an additional special case. > If someone passes an argv array with argv[0] = NULL, then argc is > zero and new_argv contains two elements but the else path writes > NULL to new_argv[2], which is past the allocated array bounds. Sigh, seems to be virtually impossible to get this right. > I don't know if this case is allowed at all. > According to the man-page: > The first argument, by convention, should point to the > filename associated with the file being executed. Stefan and me tried to understand the Posix spec for the exec function family, and it's not exactly clear to us whether a NULL argument list is a usage error or not. However, since it's possible to support that case, and funny applications may rely on it, so __execvpe should probably accept that case. Maybe fix it with char *new_argv[(argc > 0) ? 2 + argc: 3]; ? Ciao Dominik ^_^ ^_^ -- Dominik Vogt IBM Germany
On 24/11/2016 09:10, Dominik Vogt wrote: > On Wed, Nov 23, 2016 at 01:28:16PM +0100, Stefan Liebler wrote: >> On 11/22/2016 02:28 PM, Adhemerval Zanella wrote: >>> diff --git a/posix/execvpe.c b/posix/execvpe.c >>> index 7cdb06a..cf167d0 100644 >>> --- a/posix/execvpe.c >>> +++ b/posix/execvpe.c >>> @@ -38,8 +38,8 @@ >>> 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) >>> { >>> @@ -50,11 +50,11 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[]) >>> >>> /* 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]; >>> + char *new_argv[2 + argc]; >>> new_argv[0] = (char *) _PATH_BSHELL; >>> new_argv[1] = (char *) file; >>> if (argc > 1) >>> - memcpy (new_argv + 2, argv + 1, (argc - 1) * sizeof(char *)); >>> + memcpy (new_argv + 2, argv + 1, argc * sizeof(char *)); >>> else >>> new_argv[2] = NULL; >>> >>> > >> there is an additional special case. >> If someone passes an argv array with argv[0] = NULL, then argc is >> zero and new_argv contains two elements but the else path writes >> NULL to new_argv[2], which is past the allocated array bounds. > > Sigh, seems to be virtually impossible to get this right. > >> I don't know if this case is allowed at all. >> According to the man-page: >> The first argument, by convention, should point to the >> filename associated with the file being executed. > > Stefan and me tried to understand the Posix spec for the exec > function family, and it's not exactly clear to us whether a NULL > argument list is a usage error or not. However, since it's > possible to support that case, and funny applications may rely on > it, so __execvpe should probably accept that case. > > Maybe fix it with > > char *new_argv[(argc > 0) ? 2 + argc: 3]; > > ? This is pretty much what I have proposed in my last patch iteration [1] (I used your suggestion on accounting the arguments). POSIX is indeed not specific, but since old execvp{e} implementation used to support it, I think we should keep supporting it due compatibility. [1] https://sourceware.org/ml/libc-alpha/2016-11/msg00832.html
diff --git a/posix/execvpe.c b/posix/execvpe.c index 7cdb06a..cf167d0 100644 --- a/posix/execvpe.c +++ b/posix/execvpe.c @@ -38,8 +38,8 @@ 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) { @@ -50,11 +50,11 @@ maybe_script_execute (const char *file, char *const argv[], char *const envp[]) /* 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]; + char *new_argv[2 + argc]; new_argv[0] = (char *) _PATH_BSHELL; new_argv[1] = (char *) file; if (argc > 1) - memcpy (new_argv + 2, argv + 1, (argc - 1) * sizeof(char *)); + memcpy (new_argv + 2, argv + 1, argc * sizeof(char *)); else new_argv[2] = NULL;