| Message ID | 1479908285-7461-1-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
Ping. On 23/11/2016 11:38, Adhemerval Zanella wrote: > Commit 6c9e1be87a37bf wrongly fixes BZ#20847 by lefting the else branch > on maybe_script_execute to still being able to invalid write on stack > allocated buffer. It happens if execvp{e} is executed with an empty > arguments list ({ NULL }) and although manual states first argument > should be the script name itself, by convention, old and current > implementation allows it. > > This patch fixes the issue by just account for arguments and not the > final 'NULL' (since the 'argv + 1' will indeed ignored the script name). > The empty argument list is handled in a special case with a minimum > allocated size. The patch also adds extra tests for such case in > tst-vfork3. > > Tested on x86_64. > > [BZ #20847] > * posix/execvpe.c (maybe_script_execute): Remove write past allocated > array bounds for else branch. > (__execvpe): Style fixes. > * posix/tst-vfork3.c (run_script): New function. > (create_script): Likewise. > (do_test): Use run_script internal function. > (do_prepare): Use create_script internal function. > --- > ChangeLog | 12 ++++ > posix/execvpe.c | 19 ++++-- > posix/tst-vfork3.c | 185 +++++++++++++++++++---------------------------------- > 3 files changed, 91 insertions(+), 125 deletions(-) > > diff --git a/posix/execvpe.c b/posix/execvpe.c > index 7cdb06a..a2d0145 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) > { > @@ -48,13 +48,18 @@ 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]; > + /* Construct an argument list for the shell based on original arguments: > + 1. Empty list (argv = { NULL }, argc = 1 }: new argv will contain 3 > + arguments - default shell, script to execute, and ending NULL. > + 2. Non empty argument list (argc = { ..., NULL }, argc > 1}: new argv > + will contain also the default shell and the script to execute. It > + will also skip the script name in arguments and only copy script > + arguments. */ > + char *new_argv[argc > 1 ? 2 + argc : 3]; > 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; > > @@ -96,7 +101,7 @@ __execvpe (const char *file, char *const argv[], char *const envp[]) > size_t path_len = __strnlen (path, PATH_MAX - 1) + 1; > > /* NAME_MAX does not include the terminating null character. */ > - if (((file_len-1) > NAME_MAX) > + if ((file_len - 1 > NAME_MAX) > || !__libc_alloca_cutoff (path_len + file_len + 1)) > { > errno = ENAMETOOLONG; > diff --git a/posix/tst-vfork3.c b/posix/tst-vfork3.c > index 05edc5a..f5fe5c3 100644 > --- a/posix/tst-vfork3.c > +++ b/posix/tst-vfork3.c > @@ -33,84 +33,64 @@ char *tmpdirname; > #define PREPARE(argc, argv) do_prepare () > #include "../test-skeleton.c" > > -static int > -do_test (void) > +static void > +run_script (const char *script, char *const argv[]) > { > - mtrace (); > - > - const char *path = getenv ("PATH"); > - if (path == NULL) > - path = "/bin"; > - char pathbuf[strlen (tmpdirname) + 1 + strlen (path) + 1]; > - strcpy (stpcpy (stpcpy (pathbuf, tmpdirname), ":"), path); > - if (setenv ("PATH", pathbuf, 1) < 0) > - { > - puts ("setenv failed"); > - return 1; > - } > - > - size_t i; > - char *argv[3] = { (char *) "script1.sh", (char *) "1", NULL }; > - for (i = 0; i < 5; i++) > + for (size_t i = 0; i < 5; i++) > { > pid_t pid = vfork (); > if (pid < 0) > - { > - printf ("vfork failed: %m\n"); > - return 1; > - } > + FAIL_EXIT1 ("vfork failed: %m"); > else if (pid == 0) > { > - execvp ("script1.sh", argv); > + execvp (script, argv); > _exit (errno); > } > + > int status; > if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid) > - { > - puts ("waitpid failed"); > - return 1; > - } > + FAIL_EXIT1 ("waitpid failed"); > else if (status != 0) > { > if (WIFEXITED (status)) > - printf ("script1.sh failed with status %d\n", > - WEXITSTATUS (status)); > + FAIL_EXIT1 ("%s failed with status %d\n", script, > + WEXITSTATUS (status)); > else > - printf ("script1.sh kill by signal %d\n", > - WTERMSIG (status)); > - return 1; > + FAIL_EXIT1 ("%s killed by signal %d\n", script, > + WTERMSIG (status)); > } > } > +} > > - argv[0] = (char *) "script2.sh"; > - argv[1] = (char *) "2"; > - for (i = 0; i < 5; i++) > +static int > +do_test (void) > +{ > + mtrace (); > + > + const char *path = getenv ("PATH"); > + if (path == NULL) > + path = "/bin"; > + char pathbuf[strlen (tmpdirname) + 1 + strlen (path) + 1]; > + strcpy (stpcpy (stpcpy (pathbuf, tmpdirname), ":"), path); > + if (setenv ("PATH", pathbuf, 1) < 0) > { > - pid_t pid = vfork (); > - if (pid < 0) > - { > - printf ("vfork failed: %m\n"); > - return 1; > - } > - else if (pid == 0) > - { > - execvp ("script2.sh", argv); > - _exit (errno); > - } > - int status; > - if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid) > - { > - puts ("waitpid failed"); > - return 1; > - } > - else if (status != 0) > - { > - printf ("script2.sh failed with status %d\n", status); > - return 1; > - } > + puts ("setenv failed"); > + return 1; > } > > - for (i = 0; i < 5; i++) > + char *argv00[] = { NULL }; > + run_script ("script0.sh", argv00); > + char *argv01[] = { (char*) "script0.sh", NULL }; > + run_script ("script0.sh", argv01); > + > + char *argv1[] = { (char *) "script1.sh", (char *) "1", NULL }; > + run_script ("script1.sh", argv1); > + > + char *argv2[] = { (char *) "script2.sh", (char *) "2", NULL }; > + run_script ("script2.sh", argv2); > + > + /* Same as before but with execlp. */ > + for (size_t i = 0; i < 5; i++) > { > pid_t pid = vfork (); > if (pid < 0) > @@ -137,87 +117,56 @@ do_test (void) > } > > unsetenv ("PATH"); > - argv[0] = (char *) "echo"; > - argv[1] = (char *) "script 4"; > - for (i = 0; i < 5; i++) > - { > - pid_t pid = vfork (); > - if (pid < 0) > - { > - printf ("vfork failed: %m\n"); > - return 1; > - } > - else if (pid == 0) > - { > - execvp ("echo", argv); > - _exit (errno); > - } > - int status; > - if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid) > - { > - puts ("waitpid failed"); > - return 1; > - } > - else if (status != 0) > - { > - printf ("echo failed with status %d\n", status); > - return 1; > - } > - } > + char *argv4[] = { (char *) "echo", (char *) "script 4", NULL }; > + run_script ("echo", argv4); > > return 0; > } > > static void > +create_script (const char *script, const char *contents, size_t size) > +{ > + int fd = open (script, O_WRONLY | O_CREAT, 0700); > + if (fd < 0 > + || TEMP_FAILURE_RETRY (write (fd, contents, size)) != size > + || fchmod (fd, S_IRUSR | S_IXUSR) < 0) > + FAIL_EXIT1 ("could not write %s\n", script); > + close (fd); > +} > + > +static void > do_prepare (void) > { > size_t len = strlen (test_dir) + sizeof ("/tst-vfork3.XXXXXX"); > tmpdirname = malloc (len); > - char *script1 = malloc (len + sizeof "/script1.sh"); > - char *script2 = malloc (len + sizeof "/script2.sh"); > - if (tmpdirname == NULL || script1 == NULL || script2 == NULL) > - { > - puts ("out of memory"); > - exit (1); > - } > + if (tmpdirname == NULL) > + FAIL_EXIT1 ("out of memory"); > strcpy (stpcpy (tmpdirname, test_dir), "/tst-vfork3.XXXXXX"); > > tmpdirname = mkdtemp (tmpdirname); > if (tmpdirname == NULL) > - { > - puts ("could not create temporary directory"); > - exit (1); > - } > + FAIL_EXIT1 ("could not create temporary directory"); > + > + char script0[len + sizeof "/script0.sh"]; > + char script1[len + sizeof "/script1.sh"]; > + char script2[len + sizeof "/script2.sh"]; > > + strcpy (stpcpy (script0, tmpdirname), "/script0.sh"); > strcpy (stpcpy (script1, tmpdirname), "/script1.sh"); > strcpy (stpcpy (script2, tmpdirname), "/script2.sh"); > > - /* Need to make sure tmpdirname is at the end of the linked list. */ > + add_temp_file (script0); > add_temp_file (script1); > - add_temp_file (tmpdirname); > add_temp_file (script2); > + /* Need to make sure tmpdirname is at the end of the linked list. */ > + add_temp_file (tmpdirname); > + > + const char content0[] = "#!/bin/sh\necho empty\n"; > + create_script (script0, content0, sizeof content0); > > const char content1[] = "#!/bin/sh\necho script $1\n"; > - int fd = open (script1, O_WRONLY | O_CREAT, 0700); > - if (fd < 0 > - || TEMP_FAILURE_RETRY (write (fd, content1, sizeof content1)) > - != sizeof content1 > - || fchmod (fd, S_IRUSR | S_IXUSR) < 0) > - { > - printf ("Could not write %s\n", script1); > - exit (1); > - } > - close (fd); > + create_script (script1, content1, sizeof content1); > > const char content2[] = "echo script $1\n"; > - fd = open (script2, O_WRONLY | O_CREAT, 0700); > - if (fd < 0 > - || TEMP_FAILURE_RETRY (write (fd, content2, sizeof content2)) > - != sizeof content2 > - || fchmod (fd, S_IRUSR | S_IXUSR) < 0) > - { > - printf ("Could not write %s\n", script2); > - exit (1); > - } > - close (fd); > + create_script (script2, content2, sizeof content2); > } >
Ping, I think it should be safe to commit this revision since it is what Dominik has suggested also [1]. I would just like some ack for the test changes. [1] https://sourceware.org/ml/libc-alpha/2016-11/msg00887.html On 28/11/2016 15:56, Adhemerval Zanella wrote: > Ping. > > On 23/11/2016 11:38, Adhemerval Zanella wrote: >> Commit 6c9e1be87a37bf wrongly fixes BZ#20847 by lefting the else branch >> on maybe_script_execute to still being able to invalid write on stack >> allocated buffer. It happens if execvp{e} is executed with an empty >> arguments list ({ NULL }) and although manual states first argument >> should be the script name itself, by convention, old and current >> implementation allows it. >> >> This patch fixes the issue by just account for arguments and not the >> final 'NULL' (since the 'argv + 1' will indeed ignored the script name). >> The empty argument list is handled in a special case with a minimum >> allocated size. The patch also adds extra tests for such case in >> tst-vfork3. >> >> Tested on x86_64. >> >> [BZ #20847] >> * posix/execvpe.c (maybe_script_execute): Remove write past allocated >> array bounds for else branch. >> (__execvpe): Style fixes. >> * posix/tst-vfork3.c (run_script): New function. >> (create_script): Likewise. >> (do_test): Use run_script internal function. >> (do_prepare): Use create_script internal function. >> --- >> ChangeLog | 12 ++++ >> posix/execvpe.c | 19 ++++-- >> posix/tst-vfork3.c | 185 +++++++++++++++++++---------------------------------- >> 3 files changed, 91 insertions(+), 125 deletions(-) >> >> diff --git a/posix/execvpe.c b/posix/execvpe.c >> index 7cdb06a..a2d0145 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) >> { >> @@ -48,13 +48,18 @@ 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]; >> + /* Construct an argument list for the shell based on original arguments: >> + 1. Empty list (argv = { NULL }, argc = 1 }: new argv will contain 3 >> + arguments - default shell, script to execute, and ending NULL. >> + 2. Non empty argument list (argc = { ..., NULL }, argc > 1}: new argv >> + will contain also the default shell and the script to execute. It >> + will also skip the script name in arguments and only copy script >> + arguments. */ >> + char *new_argv[argc > 1 ? 2 + argc : 3]; >> 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; >> >> @@ -96,7 +101,7 @@ __execvpe (const char *file, char *const argv[], char *const envp[]) >> size_t path_len = __strnlen (path, PATH_MAX - 1) + 1; >> >> /* NAME_MAX does not include the terminating null character. */ >> - if (((file_len-1) > NAME_MAX) >> + if ((file_len - 1 > NAME_MAX) >> || !__libc_alloca_cutoff (path_len + file_len + 1)) >> { >> errno = ENAMETOOLONG; >> diff --git a/posix/tst-vfork3.c b/posix/tst-vfork3.c >> index 05edc5a..f5fe5c3 100644 >> --- a/posix/tst-vfork3.c >> +++ b/posix/tst-vfork3.c >> @@ -33,84 +33,64 @@ char *tmpdirname; >> #define PREPARE(argc, argv) do_prepare () >> #include "../test-skeleton.c" >> >> -static int >> -do_test (void) >> +static void >> +run_script (const char *script, char *const argv[]) >> { >> - mtrace (); >> - >> - const char *path = getenv ("PATH"); >> - if (path == NULL) >> - path = "/bin"; >> - char pathbuf[strlen (tmpdirname) + 1 + strlen (path) + 1]; >> - strcpy (stpcpy (stpcpy (pathbuf, tmpdirname), ":"), path); >> - if (setenv ("PATH", pathbuf, 1) < 0) >> - { >> - puts ("setenv failed"); >> - return 1; >> - } >> - >> - size_t i; >> - char *argv[3] = { (char *) "script1.sh", (char *) "1", NULL }; >> - for (i = 0; i < 5; i++) >> + for (size_t i = 0; i < 5; i++) >> { >> pid_t pid = vfork (); >> if (pid < 0) >> - { >> - printf ("vfork failed: %m\n"); >> - return 1; >> - } >> + FAIL_EXIT1 ("vfork failed: %m"); >> else if (pid == 0) >> { >> - execvp ("script1.sh", argv); >> + execvp (script, argv); >> _exit (errno); >> } >> + >> int status; >> if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid) >> - { >> - puts ("waitpid failed"); >> - return 1; >> - } >> + FAIL_EXIT1 ("waitpid failed"); >> else if (status != 0) >> { >> if (WIFEXITED (status)) >> - printf ("script1.sh failed with status %d\n", >> - WEXITSTATUS (status)); >> + FAIL_EXIT1 ("%s failed with status %d\n", script, >> + WEXITSTATUS (status)); >> else >> - printf ("script1.sh kill by signal %d\n", >> - WTERMSIG (status)); >> - return 1; >> + FAIL_EXIT1 ("%s killed by signal %d\n", script, >> + WTERMSIG (status)); >> } >> } >> +} >> >> - argv[0] = (char *) "script2.sh"; >> - argv[1] = (char *) "2"; >> - for (i = 0; i < 5; i++) >> +static int >> +do_test (void) >> +{ >> + mtrace (); >> + >> + const char *path = getenv ("PATH"); >> + if (path == NULL) >> + path = "/bin"; >> + char pathbuf[strlen (tmpdirname) + 1 + strlen (path) + 1]; >> + strcpy (stpcpy (stpcpy (pathbuf, tmpdirname), ":"), path); >> + if (setenv ("PATH", pathbuf, 1) < 0) >> { >> - pid_t pid = vfork (); >> - if (pid < 0) >> - { >> - printf ("vfork failed: %m\n"); >> - return 1; >> - } >> - else if (pid == 0) >> - { >> - execvp ("script2.sh", argv); >> - _exit (errno); >> - } >> - int status; >> - if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid) >> - { >> - puts ("waitpid failed"); >> - return 1; >> - } >> - else if (status != 0) >> - { >> - printf ("script2.sh failed with status %d\n", status); >> - return 1; >> - } >> + puts ("setenv failed"); >> + return 1; >> } >> >> - for (i = 0; i < 5; i++) >> + char *argv00[] = { NULL }; >> + run_script ("script0.sh", argv00); >> + char *argv01[] = { (char*) "script0.sh", NULL }; >> + run_script ("script0.sh", argv01); >> + >> + char *argv1[] = { (char *) "script1.sh", (char *) "1", NULL }; >> + run_script ("script1.sh", argv1); >> + >> + char *argv2[] = { (char *) "script2.sh", (char *) "2", NULL }; >> + run_script ("script2.sh", argv2); >> + >> + /* Same as before but with execlp. */ >> + for (size_t i = 0; i < 5; i++) >> { >> pid_t pid = vfork (); >> if (pid < 0) >> @@ -137,87 +117,56 @@ do_test (void) >> } >> >> unsetenv ("PATH"); >> - argv[0] = (char *) "echo"; >> - argv[1] = (char *) "script 4"; >> - for (i = 0; i < 5; i++) >> - { >> - pid_t pid = vfork (); >> - if (pid < 0) >> - { >> - printf ("vfork failed: %m\n"); >> - return 1; >> - } >> - else if (pid == 0) >> - { >> - execvp ("echo", argv); >> - _exit (errno); >> - } >> - int status; >> - if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid) >> - { >> - puts ("waitpid failed"); >> - return 1; >> - } >> - else if (status != 0) >> - { >> - printf ("echo failed with status %d\n", status); >> - return 1; >> - } >> - } >> + char *argv4[] = { (char *) "echo", (char *) "script 4", NULL }; >> + run_script ("echo", argv4); >> >> return 0; >> } >> >> static void >> +create_script (const char *script, const char *contents, size_t size) >> +{ >> + int fd = open (script, O_WRONLY | O_CREAT, 0700); >> + if (fd < 0 >> + || TEMP_FAILURE_RETRY (write (fd, contents, size)) != size >> + || fchmod (fd, S_IRUSR | S_IXUSR) < 0) >> + FAIL_EXIT1 ("could not write %s\n", script); >> + close (fd); >> +} >> + >> +static void >> do_prepare (void) >> { >> size_t len = strlen (test_dir) + sizeof ("/tst-vfork3.XXXXXX"); >> tmpdirname = malloc (len); >> - char *script1 = malloc (len + sizeof "/script1.sh"); >> - char *script2 = malloc (len + sizeof "/script2.sh"); >> - if (tmpdirname == NULL || script1 == NULL || script2 == NULL) >> - { >> - puts ("out of memory"); >> - exit (1); >> - } >> + if (tmpdirname == NULL) >> + FAIL_EXIT1 ("out of memory"); >> strcpy (stpcpy (tmpdirname, test_dir), "/tst-vfork3.XXXXXX"); >> >> tmpdirname = mkdtemp (tmpdirname); >> if (tmpdirname == NULL) >> - { >> - puts ("could not create temporary directory"); >> - exit (1); >> - } >> + FAIL_EXIT1 ("could not create temporary directory"); >> + >> + char script0[len + sizeof "/script0.sh"]; >> + char script1[len + sizeof "/script1.sh"]; >> + char script2[len + sizeof "/script2.sh"]; >> >> + strcpy (stpcpy (script0, tmpdirname), "/script0.sh"); >> strcpy (stpcpy (script1, tmpdirname), "/script1.sh"); >> strcpy (stpcpy (script2, tmpdirname), "/script2.sh"); >> >> - /* Need to make sure tmpdirname is at the end of the linked list. */ >> + add_temp_file (script0); >> add_temp_file (script1); >> - add_temp_file (tmpdirname); >> add_temp_file (script2); >> + /* Need to make sure tmpdirname is at the end of the linked list. */ >> + add_temp_file (tmpdirname); >> + >> + const char content0[] = "#!/bin/sh\necho empty\n"; >> + create_script (script0, content0, sizeof content0); >> >> const char content1[] = "#!/bin/sh\necho script $1\n"; >> - int fd = open (script1, O_WRONLY | O_CREAT, 0700); >> - if (fd < 0 >> - || TEMP_FAILURE_RETRY (write (fd, content1, sizeof content1)) >> - != sizeof content1 >> - || fchmod (fd, S_IRUSR | S_IXUSR) < 0) >> - { >> - printf ("Could not write %s\n", script1); >> - exit (1); >> - } >> - close (fd); >> + create_script (script1, content1, sizeof content1); >> >> const char content2[] = "echo script $1\n"; >> - fd = open (script2, O_WRONLY | O_CREAT, 0700); >> - if (fd < 0 >> - || TEMP_FAILURE_RETRY (write (fd, content2, sizeof content2)) >> - != sizeof content2 >> - || fchmod (fd, S_IRUSR | S_IXUSR) < 0) >> - { >> - printf ("Could not write %s\n", script2); >> - exit (1); >> - } >> - close (fd); >> + create_script (script2, content2, sizeof content2); >> } >>
If no one opposes it I will commit it shortly. On 02/12/2016 16:01, Adhemerval Zanella wrote: > Ping, I think it should be safe to commit this revision since it is what > Dominik has suggested also [1]. I would just like some ack for the test > changes. > > [1] https://sourceware.org/ml/libc-alpha/2016-11/msg00887.html > > On 28/11/2016 15:56, Adhemerval Zanella wrote: >> Ping. >> >> On 23/11/2016 11:38, Adhemerval Zanella wrote: >>> Commit 6c9e1be87a37bf wrongly fixes BZ#20847 by lefting the else branch >>> on maybe_script_execute to still being able to invalid write on stack >>> allocated buffer. It happens if execvp{e} is executed with an empty >>> arguments list ({ NULL }) and although manual states first argument >>> should be the script name itself, by convention, old and current >>> implementation allows it. >>> >>> This patch fixes the issue by just account for arguments and not the >>> final 'NULL' (since the 'argv + 1' will indeed ignored the script name). >>> The empty argument list is handled in a special case with a minimum >>> allocated size. The patch also adds extra tests for such case in >>> tst-vfork3. >>> >>> Tested on x86_64. >>> >>> [BZ #20847] >>> * posix/execvpe.c (maybe_script_execute): Remove write past allocated >>> array bounds for else branch. >>> (__execvpe): Style fixes. >>> * posix/tst-vfork3.c (run_script): New function. >>> (create_script): Likewise. >>> (do_test): Use run_script internal function. >>> (do_prepare): Use create_script internal function. >>> --- >>> ChangeLog | 12 ++++ >>> posix/execvpe.c | 19 ++++-- >>> posix/tst-vfork3.c | 185 +++++++++++++++++++---------------------------------- >>> 3 files changed, 91 insertions(+), 125 deletions(-) >>> >>> diff --git a/posix/execvpe.c b/posix/execvpe.c >>> index 7cdb06a..a2d0145 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) >>> { >>> @@ -48,13 +48,18 @@ 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]; >>> + /* Construct an argument list for the shell based on original arguments: >>> + 1. Empty list (argv = { NULL }, argc = 1 }: new argv will contain 3 >>> + arguments - default shell, script to execute, and ending NULL. >>> + 2. Non empty argument list (argc = { ..., NULL }, argc > 1}: new argv >>> + will contain also the default shell and the script to execute. It >>> + will also skip the script name in arguments and only copy script >>> + arguments. */ >>> + char *new_argv[argc > 1 ? 2 + argc : 3]; >>> 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; >>> >>> @@ -96,7 +101,7 @@ __execvpe (const char *file, char *const argv[], char *const envp[]) >>> size_t path_len = __strnlen (path, PATH_MAX - 1) + 1; >>> >>> /* NAME_MAX does not include the terminating null character. */ >>> - if (((file_len-1) > NAME_MAX) >>> + if ((file_len - 1 > NAME_MAX) >>> || !__libc_alloca_cutoff (path_len + file_len + 1)) >>> { >>> errno = ENAMETOOLONG; >>> diff --git a/posix/tst-vfork3.c b/posix/tst-vfork3.c >>> index 05edc5a..f5fe5c3 100644 >>> --- a/posix/tst-vfork3.c >>> +++ b/posix/tst-vfork3.c >>> @@ -33,84 +33,64 @@ char *tmpdirname; >>> #define PREPARE(argc, argv) do_prepare () >>> #include "../test-skeleton.c" >>> >>> -static int >>> -do_test (void) >>> +static void >>> +run_script (const char *script, char *const argv[]) >>> { >>> - mtrace (); >>> - >>> - const char *path = getenv ("PATH"); >>> - if (path == NULL) >>> - path = "/bin"; >>> - char pathbuf[strlen (tmpdirname) + 1 + strlen (path) + 1]; >>> - strcpy (stpcpy (stpcpy (pathbuf, tmpdirname), ":"), path); >>> - if (setenv ("PATH", pathbuf, 1) < 0) >>> - { >>> - puts ("setenv failed"); >>> - return 1; >>> - } >>> - >>> - size_t i; >>> - char *argv[3] = { (char *) "script1.sh", (char *) "1", NULL }; >>> - for (i = 0; i < 5; i++) >>> + for (size_t i = 0; i < 5; i++) >>> { >>> pid_t pid = vfork (); >>> if (pid < 0) >>> - { >>> - printf ("vfork failed: %m\n"); >>> - return 1; >>> - } >>> + FAIL_EXIT1 ("vfork failed: %m"); >>> else if (pid == 0) >>> { >>> - execvp ("script1.sh", argv); >>> + execvp (script, argv); >>> _exit (errno); >>> } >>> + >>> int status; >>> if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid) >>> - { >>> - puts ("waitpid failed"); >>> - return 1; >>> - } >>> + FAIL_EXIT1 ("waitpid failed"); >>> else if (status != 0) >>> { >>> if (WIFEXITED (status)) >>> - printf ("script1.sh failed with status %d\n", >>> - WEXITSTATUS (status)); >>> + FAIL_EXIT1 ("%s failed with status %d\n", script, >>> + WEXITSTATUS (status)); >>> else >>> - printf ("script1.sh kill by signal %d\n", >>> - WTERMSIG (status)); >>> - return 1; >>> + FAIL_EXIT1 ("%s killed by signal %d\n", script, >>> + WTERMSIG (status)); >>> } >>> } >>> +} >>> >>> - argv[0] = (char *) "script2.sh"; >>> - argv[1] = (char *) "2"; >>> - for (i = 0; i < 5; i++) >>> +static int >>> +do_test (void) >>> +{ >>> + mtrace (); >>> + >>> + const char *path = getenv ("PATH"); >>> + if (path == NULL) >>> + path = "/bin"; >>> + char pathbuf[strlen (tmpdirname) + 1 + strlen (path) + 1]; >>> + strcpy (stpcpy (stpcpy (pathbuf, tmpdirname), ":"), path); >>> + if (setenv ("PATH", pathbuf, 1) < 0) >>> { >>> - pid_t pid = vfork (); >>> - if (pid < 0) >>> - { >>> - printf ("vfork failed: %m\n"); >>> - return 1; >>> - } >>> - else if (pid == 0) >>> - { >>> - execvp ("script2.sh", argv); >>> - _exit (errno); >>> - } >>> - int status; >>> - if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid) >>> - { >>> - puts ("waitpid failed"); >>> - return 1; >>> - } >>> - else if (status != 0) >>> - { >>> - printf ("script2.sh failed with status %d\n", status); >>> - return 1; >>> - } >>> + puts ("setenv failed"); >>> + return 1; >>> } >>> >>> - for (i = 0; i < 5; i++) >>> + char *argv00[] = { NULL }; >>> + run_script ("script0.sh", argv00); >>> + char *argv01[] = { (char*) "script0.sh", NULL }; >>> + run_script ("script0.sh", argv01); >>> + >>> + char *argv1[] = { (char *) "script1.sh", (char *) "1", NULL }; >>> + run_script ("script1.sh", argv1); >>> + >>> + char *argv2[] = { (char *) "script2.sh", (char *) "2", NULL }; >>> + run_script ("script2.sh", argv2); >>> + >>> + /* Same as before but with execlp. */ >>> + for (size_t i = 0; i < 5; i++) >>> { >>> pid_t pid = vfork (); >>> if (pid < 0) >>> @@ -137,87 +117,56 @@ do_test (void) >>> } >>> >>> unsetenv ("PATH"); >>> - argv[0] = (char *) "echo"; >>> - argv[1] = (char *) "script 4"; >>> - for (i = 0; i < 5; i++) >>> - { >>> - pid_t pid = vfork (); >>> - if (pid < 0) >>> - { >>> - printf ("vfork failed: %m\n"); >>> - return 1; >>> - } >>> - else if (pid == 0) >>> - { >>> - execvp ("echo", argv); >>> - _exit (errno); >>> - } >>> - int status; >>> - if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid) >>> - { >>> - puts ("waitpid failed"); >>> - return 1; >>> - } >>> - else if (status != 0) >>> - { >>> - printf ("echo failed with status %d\n", status); >>> - return 1; >>> - } >>> - } >>> + char *argv4[] = { (char *) "echo", (char *) "script 4", NULL }; >>> + run_script ("echo", argv4); >>> >>> return 0; >>> } >>> >>> static void >>> +create_script (const char *script, const char *contents, size_t size) >>> +{ >>> + int fd = open (script, O_WRONLY | O_CREAT, 0700); >>> + if (fd < 0 >>> + || TEMP_FAILURE_RETRY (write (fd, contents, size)) != size >>> + || fchmod (fd, S_IRUSR | S_IXUSR) < 0) >>> + FAIL_EXIT1 ("could not write %s\n", script); >>> + close (fd); >>> +} >>> + >>> +static void >>> do_prepare (void) >>> { >>> size_t len = strlen (test_dir) + sizeof ("/tst-vfork3.XXXXXX"); >>> tmpdirname = malloc (len); >>> - char *script1 = malloc (len + sizeof "/script1.sh"); >>> - char *script2 = malloc (len + sizeof "/script2.sh"); >>> - if (tmpdirname == NULL || script1 == NULL || script2 == NULL) >>> - { >>> - puts ("out of memory"); >>> - exit (1); >>> - } >>> + if (tmpdirname == NULL) >>> + FAIL_EXIT1 ("out of memory"); >>> strcpy (stpcpy (tmpdirname, test_dir), "/tst-vfork3.XXXXXX"); >>> >>> tmpdirname = mkdtemp (tmpdirname); >>> if (tmpdirname == NULL) >>> - { >>> - puts ("could not create temporary directory"); >>> - exit (1); >>> - } >>> + FAIL_EXIT1 ("could not create temporary directory"); >>> + >>> + char script0[len + sizeof "/script0.sh"]; >>> + char script1[len + sizeof "/script1.sh"]; >>> + char script2[len + sizeof "/script2.sh"]; >>> >>> + strcpy (stpcpy (script0, tmpdirname), "/script0.sh"); >>> strcpy (stpcpy (script1, tmpdirname), "/script1.sh"); >>> strcpy (stpcpy (script2, tmpdirname), "/script2.sh"); >>> >>> - /* Need to make sure tmpdirname is at the end of the linked list. */ >>> + add_temp_file (script0); >>> add_temp_file (script1); >>> - add_temp_file (tmpdirname); >>> add_temp_file (script2); >>> + /* Need to make sure tmpdirname is at the end of the linked list. */ >>> + add_temp_file (tmpdirname); >>> + >>> + const char content0[] = "#!/bin/sh\necho empty\n"; >>> + create_script (script0, content0, sizeof content0); >>> >>> const char content1[] = "#!/bin/sh\necho script $1\n"; >>> - int fd = open (script1, O_WRONLY | O_CREAT, 0700); >>> - if (fd < 0 >>> - || TEMP_FAILURE_RETRY (write (fd, content1, sizeof content1)) >>> - != sizeof content1 >>> - || fchmod (fd, S_IRUSR | S_IXUSR) < 0) >>> - { >>> - printf ("Could not write %s\n", script1); >>> - exit (1); >>> - } >>> - close (fd); >>> + create_script (script1, content1, sizeof content1); >>> >>> const char content2[] = "echo script $1\n"; >>> - fd = open (script2, O_WRONLY | O_CREAT, 0700); >>> - if (fd < 0 >>> - || TEMP_FAILURE_RETRY (write (fd, content2, sizeof content2)) >>> - != sizeof content2 >>> - || fchmod (fd, S_IRUSR | S_IXUSR) < 0) >>> - { >>> - printf ("Could not write %s\n", script2); >>> - exit (1); >>> - } >>> - close (fd); >>> + create_script (script2, content2, sizeof content2); >>> } >>>
diff --git a/posix/execvpe.c b/posix/execvpe.c index 7cdb06a..a2d0145 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) { @@ -48,13 +48,18 @@ 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]; + /* Construct an argument list for the shell based on original arguments: + 1. Empty list (argv = { NULL }, argc = 1 }: new argv will contain 3 + arguments - default shell, script to execute, and ending NULL. + 2. Non empty argument list (argc = { ..., NULL }, argc > 1}: new argv + will contain also the default shell and the script to execute. It + will also skip the script name in arguments and only copy script + arguments. */ + char *new_argv[argc > 1 ? 2 + argc : 3]; 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; @@ -96,7 +101,7 @@ __execvpe (const char *file, char *const argv[], char *const envp[]) size_t path_len = __strnlen (path, PATH_MAX - 1) + 1; /* NAME_MAX does not include the terminating null character. */ - if (((file_len-1) > NAME_MAX) + if ((file_len - 1 > NAME_MAX) || !__libc_alloca_cutoff (path_len + file_len + 1)) { errno = ENAMETOOLONG; diff --git a/posix/tst-vfork3.c b/posix/tst-vfork3.c index 05edc5a..f5fe5c3 100644 --- a/posix/tst-vfork3.c +++ b/posix/tst-vfork3.c @@ -33,84 +33,64 @@ char *tmpdirname; #define PREPARE(argc, argv) do_prepare () #include "../test-skeleton.c" -static int -do_test (void) +static void +run_script (const char *script, char *const argv[]) { - mtrace (); - - const char *path = getenv ("PATH"); - if (path == NULL) - path = "/bin"; - char pathbuf[strlen (tmpdirname) + 1 + strlen (path) + 1]; - strcpy (stpcpy (stpcpy (pathbuf, tmpdirname), ":"), path); - if (setenv ("PATH", pathbuf, 1) < 0) - { - puts ("setenv failed"); - return 1; - } - - size_t i; - char *argv[3] = { (char *) "script1.sh", (char *) "1", NULL }; - for (i = 0; i < 5; i++) + for (size_t i = 0; i < 5; i++) { pid_t pid = vfork (); if (pid < 0) - { - printf ("vfork failed: %m\n"); - return 1; - } + FAIL_EXIT1 ("vfork failed: %m"); else if (pid == 0) { - execvp ("script1.sh", argv); + execvp (script, argv); _exit (errno); } + int status; if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid) - { - puts ("waitpid failed"); - return 1; - } + FAIL_EXIT1 ("waitpid failed"); else if (status != 0) { if (WIFEXITED (status)) - printf ("script1.sh failed with status %d\n", - WEXITSTATUS (status)); + FAIL_EXIT1 ("%s failed with status %d\n", script, + WEXITSTATUS (status)); else - printf ("script1.sh kill by signal %d\n", - WTERMSIG (status)); - return 1; + FAIL_EXIT1 ("%s killed by signal %d\n", script, + WTERMSIG (status)); } } +} - argv[0] = (char *) "script2.sh"; - argv[1] = (char *) "2"; - for (i = 0; i < 5; i++) +static int +do_test (void) +{ + mtrace (); + + const char *path = getenv ("PATH"); + if (path == NULL) + path = "/bin"; + char pathbuf[strlen (tmpdirname) + 1 + strlen (path) + 1]; + strcpy (stpcpy (stpcpy (pathbuf, tmpdirname), ":"), path); + if (setenv ("PATH", pathbuf, 1) < 0) { - pid_t pid = vfork (); - if (pid < 0) - { - printf ("vfork failed: %m\n"); - return 1; - } - else if (pid == 0) - { - execvp ("script2.sh", argv); - _exit (errno); - } - int status; - if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid) - { - puts ("waitpid failed"); - return 1; - } - else if (status != 0) - { - printf ("script2.sh failed with status %d\n", status); - return 1; - } + puts ("setenv failed"); + return 1; } - for (i = 0; i < 5; i++) + char *argv00[] = { NULL }; + run_script ("script0.sh", argv00); + char *argv01[] = { (char*) "script0.sh", NULL }; + run_script ("script0.sh", argv01); + + char *argv1[] = { (char *) "script1.sh", (char *) "1", NULL }; + run_script ("script1.sh", argv1); + + char *argv2[] = { (char *) "script2.sh", (char *) "2", NULL }; + run_script ("script2.sh", argv2); + + /* Same as before but with execlp. */ + for (size_t i = 0; i < 5; i++) { pid_t pid = vfork (); if (pid < 0) @@ -137,87 +117,56 @@ do_test (void) } unsetenv ("PATH"); - argv[0] = (char *) "echo"; - argv[1] = (char *) "script 4"; - for (i = 0; i < 5; i++) - { - pid_t pid = vfork (); - if (pid < 0) - { - printf ("vfork failed: %m\n"); - return 1; - } - else if (pid == 0) - { - execvp ("echo", argv); - _exit (errno); - } - int status; - if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid) - { - puts ("waitpid failed"); - return 1; - } - else if (status != 0) - { - printf ("echo failed with status %d\n", status); - return 1; - } - } + char *argv4[] = { (char *) "echo", (char *) "script 4", NULL }; + run_script ("echo", argv4); return 0; } static void +create_script (const char *script, const char *contents, size_t size) +{ + int fd = open (script, O_WRONLY | O_CREAT, 0700); + if (fd < 0 + || TEMP_FAILURE_RETRY (write (fd, contents, size)) != size + || fchmod (fd, S_IRUSR | S_IXUSR) < 0) + FAIL_EXIT1 ("could not write %s\n", script); + close (fd); +} + +static void do_prepare (void) { size_t len = strlen (test_dir) + sizeof ("/tst-vfork3.XXXXXX"); tmpdirname = malloc (len); - char *script1 = malloc (len + sizeof "/script1.sh"); - char *script2 = malloc (len + sizeof "/script2.sh"); - if (tmpdirname == NULL || script1 == NULL || script2 == NULL) - { - puts ("out of memory"); - exit (1); - } + if (tmpdirname == NULL) + FAIL_EXIT1 ("out of memory"); strcpy (stpcpy (tmpdirname, test_dir), "/tst-vfork3.XXXXXX"); tmpdirname = mkdtemp (tmpdirname); if (tmpdirname == NULL) - { - puts ("could not create temporary directory"); - exit (1); - } + FAIL_EXIT1 ("could not create temporary directory"); + + char script0[len + sizeof "/script0.sh"]; + char script1[len + sizeof "/script1.sh"]; + char script2[len + sizeof "/script2.sh"]; + strcpy (stpcpy (script0, tmpdirname), "/script0.sh"); strcpy (stpcpy (script1, tmpdirname), "/script1.sh"); strcpy (stpcpy (script2, tmpdirname), "/script2.sh"); - /* Need to make sure tmpdirname is at the end of the linked list. */ + add_temp_file (script0); add_temp_file (script1); - add_temp_file (tmpdirname); add_temp_file (script2); + /* Need to make sure tmpdirname is at the end of the linked list. */ + add_temp_file (tmpdirname); + + const char content0[] = "#!/bin/sh\necho empty\n"; + create_script (script0, content0, sizeof content0); const char content1[] = "#!/bin/sh\necho script $1\n"; - int fd = open (script1, O_WRONLY | O_CREAT, 0700); - if (fd < 0 - || TEMP_FAILURE_RETRY (write (fd, content1, sizeof content1)) - != sizeof content1 - || fchmod (fd, S_IRUSR | S_IXUSR) < 0) - { - printf ("Could not write %s\n", script1); - exit (1); - } - close (fd); + create_script (script1, content1, sizeof content1); const char content2[] = "echo script $1\n"; - fd = open (script2, O_WRONLY | O_CREAT, 0700); - if (fd < 0 - || TEMP_FAILURE_RETRY (write (fd, content2, sizeof content2)) - != sizeof content2 - || fchmod (fd, S_IRUSR | S_IXUSR) < 0) - { - printf ("Could not write %s\n", script2); - exit (1); - } - close (fd); + create_script (script2, content2, sizeof content2); }
Commit 6c9e1be87a37bf wrongly fixes BZ#20847 by lefting the else branch on maybe_script_execute to still being able to invalid write on stack allocated buffer. It happens if execvp{e} is executed with an empty arguments list ({ NULL }) and although manual states first argument should be the script name itself, by convention, old and current implementation allows it. This patch fixes the issue by just account for arguments and not the final 'NULL' (since the 'argv + 1' will indeed ignored the script name). The empty argument list is handled in a special case with a minimum allocated size. The patch also adds extra tests for such case in tst-vfork3. Tested on x86_64. [BZ #20847] * posix/execvpe.c (maybe_script_execute): Remove write past allocated array bounds for else branch. (__execvpe): Style fixes. * posix/tst-vfork3.c (run_script): New function. (create_script): Likewise. (do_test): Use run_script internal function. (do_prepare): Use create_script internal function. --- ChangeLog | 12 ++++ posix/execvpe.c | 19 ++++-- posix/tst-vfork3.c | 185 +++++++++++++++++++---------------------------------- 3 files changed, 91 insertions(+), 125 deletions(-) -- 2.7.4