| Message ID | 20190731183136.21545-5-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/5] mips: Do not malloc on getdents64 fallback | expand |
Ping. On 31/07/2019 15:31, Adhemerval Zanella wrote: > Change from previous version: > > - Use libsupport and remove atfork usage on posix/wordexp-test.c. > > -- > > This patch replaces the fork+exec by posix_spawn on wordexp, which > allows a better scability on Linux and simplifies the thread > cancellation handling. > > The only change which can not be implemented with posix_spawn the > /dev/null check to certify it is indeed the expected device. I am > not sure how effetive this check is since /dev/null tampering means > something very wrong with the system and this is the least of the > issues. My view is the tests is really out of the place and the > hardening provided is minimum. > > If the idea is still to provide such check, I think a possibilty > would be to open /dev/null, check it, add a dup2 file action, and > close the file descriptor. > > Checked on powerpc64le-linux-gnu and x86_64-linux-gnu. > > * include/spawn.h (__posix_spawn_file_actions_addopen): New > prototype. > * posix/spawn_faction_addopen.c (posix_spawn_file_actions_addopen): > Add internal alias. > * posix/wordexp.c (create_environment, free_environment): New > functions. > (exec_comm_child, exec_comm): Use posix_spawn instead of fork+exec. > * posix/wordexp-test.c: Use libsupport and remove atfork usage. > --- > include/spawn.h | 3 + > posix/spawn_faction_addopen.c | 8 +- > posix/wordexp-test.c | 142 +++++++++-------------------- > posix/wordexp.c | 167 ++++++++++++++++------------------ > 4 files changed, 129 insertions(+), 191 deletions(-) > > diff --git a/include/spawn.h b/include/spawn.h > index 7fdd965bd7..4a0b1849da 100644 > --- a/include/spawn.h > +++ b/include/spawn.h > @@ -11,6 +11,9 @@ __typeof (posix_spawn_file_actions_addclose) > __typeof (posix_spawn_file_actions_adddup2) > __posix_spawn_file_actions_adddup2 attribute_hidden; > > +__typeof (posix_spawn_file_actions_addopen) > + __posix_spawn_file_actions_addopen attribute_hidden; > + > __typeof (posix_spawn_file_actions_destroy) > __posix_spawn_file_actions_destroy attribute_hidden; > > diff --git a/posix/spawn_faction_addopen.c b/posix/spawn_faction_addopen.c > index 742eb9526d..2e598de300 100644 > --- a/posix/spawn_faction_addopen.c > +++ b/posix/spawn_faction_addopen.c > @@ -25,9 +25,9 @@ > /* Add an action to FILE-ACTIONS which tells the implementation to call > `open' for the given file during the `spawn' call. */ > int > -posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, > - int fd, const char *path, int oflag, > - mode_t mode) > +__posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, > + int fd, const char *path, int oflag, > + mode_t mode) > { > struct __spawn_action *rec; > > @@ -60,3 +60,5 @@ posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, > > return 0; > } > +weak_alias (__posix_spawn_file_actions_addopen, > + posix_spawn_file_actions_addopen) > diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c > index 10a0768a6b..ef780b0a65 100644 > --- a/posix/wordexp-test.c > +++ b/posix/wordexp-test.c > @@ -15,39 +15,21 @@ > License along with the GNU C Library; if not, see > <http://www.gnu.org/licenses/>. */ > > -#include <sys/stat.h> > -#include <sys/types.h> > -#include <sys/mman.h> > +#include <wordexp.h> > +#include <stdio.h> > #include <fcntl.h> > -#include <unistd.h> > #include <pwd.h> > -#include <stdio.h> > -#include <stdint.h> > #include <stdlib.h> > #include <string.h> > -#include <wordexp.h> > +#include <sys/mman.h> > + > #include <libc-pointer-arith.h> > -#include <dso_handle.h> > +#include <array_length.h> > +#include <support/xunistd.h> > +#include <support/check.h> > > #define IFS " \n\t" > > -extern int __register_atfork (void (*) (void), void (*) (void), void (*) (void), void *); > - > -static int __app_register_atfork (void (*prepare) (void), void (*parent) (void), void (*child) (void)) > -{ > - return __register_atfork (prepare, parent, child, __dso_handle); > -} > - > -/* Number of forks seen. */ > -static int registered_forks; > - > -/* For each fork increment the fork count. */ > -static void > -register_fork (void) > -{ > - registered_forks++; > -} > - > struct test_case_struct > { > int retval; > @@ -57,7 +39,7 @@ struct test_case_struct > size_t wordc; > const char *wordv[10]; > const char *ifs; > -} test_case[] = > +} static test_case[] = > { > /* Simple word- and field-splitting */ > { 0, NULL, "one", 0, 1, { "one", }, IFS }, > @@ -238,8 +220,6 @@ struct test_case_struct > { WRDE_SYNTAX, NULL, "${", 0, 0, { NULL, }, IFS }, /* BZ 18043 */ > { WRDE_SYNTAX, NULL, "L${a:", 0, 0, { NULL, }, IFS }, /* BZ 18043#c4 */ > { WRDE_SYNTAX, NULL, "$[1/0]", WRDE_NOCMD, 0, {NULL, }, IFS }, /* BZ 18100 */ > - > - { -1, NULL, NULL, 0, 0, { NULL, }, IFS }, > }; > > static int testit (struct test_case_struct *tc); > @@ -256,16 +236,14 @@ command_line_test (const char *words) > printf ("we_wordv[%d] = \"%s\"\n", i, we.we_wordv[i]); > } > > -int > -main (int argc, char *argv[]) > +static int > +do_test (int argc, char *argv[]) > { > - const char *globfile[] = { "one", "two", "three", NULL }; > + const char *globfile[] = { "one", "two", "three" }; > char tmpdir[32]; > struct passwd *pw; > const char *cwd; > int test; > - int fail = 0; > - int i; > struct test_case_struct ts; > > if (argc > 1) > @@ -278,30 +256,18 @@ main (int argc, char *argv[]) > > /* Set up arena for pathname expansion */ > tmpnam (tmpdir); > - if (mkdir (tmpdir, S_IRWXU) || chdir (tmpdir)) > - return -1; > - else > - { > - int fd; > + xmkdir (tmpdir, S_IRWXU); > + TEST_VERIFY_EXIT (chdir (tmpdir) == 0); > > - for (i = 0; globfile[i]; ++i) > - if ((fd = creat (globfile[i], S_IRUSR | S_IWUSR)) == -1 > - || close (fd)) > - return -1; > - } > - > - /* If we are not allowed to do command substitution, we install > - fork handlers to verify that no forks happened. No forks should > - happen at all if command substitution is disabled. */ > - if (__app_register_atfork (register_fork, NULL, NULL) != 0) > + for (int i = 0; i < array_length (globfile); ++i) > { > - printf ("Failed to register fork handler.\n"); > - return -1; > + int fd = xopen (globfile[i], O_WRONLY|O_CREAT|O_TRUNC, > + S_IRUSR | S_IWUSR); > + xclose (fd); > } > > - for (test = 0; test_case[test].retval != -1; test++) > - if (testit (&test_case[test])) > - ++fail; > + for (test = 0; test < array_length (test_case); test++) > + TEST_COMPARE (testit (&test_case[test]), 0); > > /* Tilde-expansion tests. */ > pw = getpwnam ("root"); > @@ -315,8 +281,7 @@ main (int argc, char *argv[]) > ts.wordv[0] = pw->pw_dir; > ts.ifs = IFS; > > - if (testit (&ts)) > - ++fail; > + TEST_COMPARE (testit (&ts), 0); > > ts.retval = 0; > ts.env = pw->pw_dir; > @@ -326,8 +291,7 @@ main (int argc, char *argv[]) > ts.wordv[0] = "x"; > ts.ifs = IFS; > > - if (testit (&ts)) > - ++fail; > + TEST_COMPARE (testit (&ts), 0); > } > > /* "~" expands to value of $HOME when HOME is set */ > @@ -342,8 +306,7 @@ main (int argc, char *argv[]) > ts.wordv[1] = "/dummy/home/foo"; > ts.ifs = IFS; > > - if (testit (&ts)) > - ++fail; > + TEST_COMPARE (testit (&ts), 0); > > /* "~" expands to home dir from passwd file if HOME is not set */ > > @@ -359,8 +322,7 @@ main (int argc, char *argv[]) > ts.wordv[0] = pw->pw_dir; > ts.ifs = IFS; > > - if (testit (&ts)) > - ++fail; > + TEST_COMPARE (testit (&ts), 0); > } > > /* Integer overflow in division. */ > @@ -375,37 +337,32 @@ main (int argc, char *argv[]) > "18446744073709551616", > "170141183460469231731687303715884105728", > "340282366920938463463374607431768211456", > - NULL > }; > > - for (const char *const *num = numbers; *num; ++num) > + for (int i = 0; i < array_length (numbers); i++) > { > wordexp_t p; > char pattern[256]; > - snprintf (pattern, sizeof (pattern), "$[(-%s)/(-1)]", *num); > + snprintf (pattern, sizeof (pattern), "$[(-%s)/(-1)]", numbers[i]); > int ret = wordexp (pattern, &p, WRDE_NOCMD); > if (ret == 0) > { > - if (p.we_wordc != 1 || strcmp (p.we_wordv[0], *num) != 0) > - { > - printf ("Integer overflow for \"%s\" failed", pattern); > - ++fail; > - } > + TEST_COMPARE (p.we_wordc, 1); > + TEST_COMPARE (strcmp (p.we_wordv[0], numbers[i]), 0); > wordfree (&p); > } > - else if (ret != WRDE_SYNTAX) > + else > { > - printf ("Integer overflow for \"%s\" failed with %d", > - pattern, ret); > - ++fail; > + TEST_COMPARE (ret, WRDE_SYNTAX); > + if (ret != WRDE_SYNTAX) > + printf ("Integer overflow for \"%s\" failed with %d", > + pattern, ret); > } > } > } > > - puts ("tests completed, now cleaning up"); > - > /* Clean up */ > - for (i = 0; globfile[i]; ++i) > + for (int i = 0; i < array_length (globfile); ++i) > remove (globfile[i]); > > if (cwd == NULL) > @@ -414,26 +371,17 @@ main (int argc, char *argv[]) > chdir (cwd); > rmdir (tmpdir); > > - printf ("tests failed: %d\n", fail); > - > - return fail != 0; > + return 0; > } > > static const char * > at_page_end (const char *words) > { > const int pagesize = getpagesize (); > - char *start = mmap (0, 2 * pagesize, PROT_READ|PROT_WRITE, > - MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); > + char *start = xmmap (0, 2 * pagesize, PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANONYMOUS, -1); > > - if (start == MAP_FAILED) > - return start; > - > - if (mprotect (start + pagesize, pagesize, PROT_NONE)) > - { > - munmap (start, 2 * pagesize); > - return MAP_FAILED; > - } > + xmprotect (start + pagesize, pagesize, PROT_NONE); > > /* Includes terminating NUL. */ > const size_t words_size = strlen (words) + 1; > @@ -472,9 +420,6 @@ testit (struct test_case_struct *tc) > fflush (NULL); > const char *words = at_page_end (tc->words); > > - if (tc->flags & WRDE_NOCMD) > - registered_forks = 0; > - > if (tc->flags & WRDE_APPEND) > { > /* initial wordexp() call, to be appended to */ > @@ -486,13 +431,6 @@ testit (struct test_case_struct *tc) > } > retval = wordexp (words, &we, tc->flags); > > - if ((tc->flags & WRDE_NOCMD) > - && (registered_forks > 0)) > - { > - printf ("FAILED fork called for WRDE_NOCMD\n"); > - return 1; > - } > - > if (tc->flags & WRDE_DOOFFS) > start_offs = sav_we.we_offs; > > @@ -551,9 +489,11 @@ testit (struct test_case_struct *tc) > const int page_size = getpagesize (); > char *start = (char *) PTR_ALIGN_DOWN (words, page_size); > > - if (munmap (start, 2 * page_size) != 0) > - return 1; > + xmunmap (start, 2 * page_size); > > fflush (NULL); > return bzzzt; > } > + > +#define TEST_FUNCTION_ARGV do_test > +#include <support/test-driver.c> > diff --git a/posix/wordexp.c b/posix/wordexp.c > index 22c6d18a9c..e1aafcaceb 100644 > --- a/posix/wordexp.c > +++ b/posix/wordexp.c > @@ -25,33 +25,18 @@ > #include <libintl.h> > #include <paths.h> > #include <pwd.h> > -#include <signal.h> > #include <stdbool.h> > #include <stdio.h> > -#include <stdlib.h> > #include <string.h> > #include <sys/param.h> > -#include <sys/stat.h> > -#include <sys/time.h> > -#include <sys/types.h> > -#include <sys/types.h> > #include <sys/wait.h> > #include <unistd.h> > -#include <wchar.h> > #include <wordexp.h> > -#include <kernel-features.h> > +#include <spawn.h> > #include <scratch_buffer.h> > - > -#include <libc-lock.h> > #include <_itoa.h> > - > -/* Undefine the following line for the production version. */ > -/* #define NDEBUG 1 */ > #include <assert.h> > > -/* Get some device information. */ > -#include <device-nrs.h> > - > /* > * This is a recursive-descent-style word expansion routine. > */ > @@ -812,61 +797,90 @@ parse_arith (char **word, size_t *word_length, size_t *max_length, > return WRDE_SYNTAX; > } > > +static char ** > +create_environment (void) > +{ > + size_t s = 0; > + > + /* Calculate total environment size, including 'IFS' if is present. */ > + for (char **ep = __environ; *ep != NULL; ep++, s++); > + > + /* Include final NULL pointer. */ > + char **newenviron = malloc (s * sizeof (char*)); > + if (newenviron == NULL) > + return NULL; > + > + /* Copy current environment excluding 'IFS', to make sure the subshell > + doesn't field-split on our behalf. */ > + size_t i, j; > + for (i = 0, j = 0; i < s; i++) > + if (strncmp (__environ[i], "IFS=", sizeof ("IFS=")-1) != 0) > + newenviron[j++] = __strdup (__environ[i]); > + newenviron[j] = NULL; > + > + return newenviron; > +} > + > +static void > +free_environment (char **environ) > +{ > + for (char **ep = environ; *ep != NULL; ep++) > + free (*ep); > + free (environ); > +} > + > /* Function called by child process in exec_comm() */ > -static inline void > -__attribute__ ((always_inline)) > -exec_comm_child (char *comm, int *fildes, int showerr, int noexec) > +static pid_t > +exec_comm_child (char *comm, int *fildes, bool showerr, bool noexec) > { > - const char *args[4] = { _PATH_BSHELL, "-c", comm, NULL }; > + pid_t pid = -1; > > - /* Execute the command, or just check syntax? */ > - if (noexec) > - args[1] = "-nc"; > + /* Execute the command, or just check syntax? */ > + const char *args[] = { _PATH_BSHELL, noexec ? "-nc" : "-c", comm, NULL }; > > - /* Redirect output. */ > - if (__glibc_likely (fildes[1] != STDOUT_FILENO)) > - { > - __dup2 (fildes[1], STDOUT_FILENO); > - __close (fildes[1]); > - } > - else > - /* Reset the close-on-exec flag (if necessary). */ > - __fcntl (fildes[1], F_SETFD, 0); > + posix_spawn_file_actions_t fa; > + /* posix_spawn_file_actions_init does not fail. */ > + __posix_spawn_file_actions_init (&fa); > > - /* Redirect stderr to /dev/null if we have to. */ > - if (showerr == 0) > + /* Redirect output. For check syntax only (noexec being true), exec_comm > + explicits sets fildes[1] to -1, so check its value to avoid a failure in > + __posix_spawn_file_actions_adddup2. */ > + if (fildes[1] != -1) > { > - struct stat64 st; > - int fd; > - __close (STDERR_FILENO); > - fd = __open (_PATH_DEVNULL, O_WRONLY); > - if (fd >= 0 && fd != STDERR_FILENO) > + if (__glibc_likely (fildes[1] != STDOUT_FILENO)) > { > - __dup2 (fd, STDERR_FILENO); > - __close (fd); > + if (__posix_spawn_file_actions_adddup2 (&fa, fildes[1], > + STDOUT_FILENO) != 0 > + || __posix_spawn_file_actions_addclose (&fa, fildes[1]) != 0) > + goto out; > } > - /* Be paranoid. Check that we actually opened the /dev/null > - device. */ > - if (__builtin_expect (__fxstat64 (_STAT_VER, STDERR_FILENO, &st), 0) != 0 > - || __builtin_expect (S_ISCHR (st.st_mode), 1) == 0 > -#if defined DEV_NULL_MAJOR && defined DEV_NULL_MINOR > - || st.st_rdev != __gnu_dev_makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR) > -#endif > - ) > - /* It's not the /dev/null device. Stop right here. The > - problem is: how do we stop? We use _exit() with an > - hopefully unusual exit code. */ > - _exit (90); > + else > + /* Reset the close-on-exec flag (if necessary). */ > + if (__posix_spawn_file_actions_adddup2 (&fa, fildes[1], fildes[1]) > + != 0) > + goto out; > } > > - /* Make sure the subshell doesn't field-split on our behalf. */ > - __unsetenv ("IFS"); > + /* Redirect stderr to /dev/null if we have to. */ > + if (!showerr) > + if (__posix_spawn_file_actions_addopen (&fa, STDERR_FILENO, _PATH_DEVNULL, > + O_WRONLY, 0) != 0) > + goto out; > + > + char **newenv = create_environment (); > + if (newenv == NULL) > + goto out; > > - __close (fildes[0]); > - __execve (_PATH_BSHELL, (char *const *) args, __environ); > + /* pid is unset if posix_spawn fails, so it keep the original value > + of -1. */ > + __posix_spawn (&pid, _PATH_BSHELL, &fa, NULL, (char *const *) args, newenv); > > - /* Bad. What now? */ > - abort (); > + free_environment (newenv); > + > +out: > + __posix_spawn_file_actions_destroy (&fa); > + > + return pid; > } > > /* Function to execute a command and retrieve the results */ > @@ -884,13 +898,13 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, > size_t maxnewlines = 0; > char buffer[bufsize]; > pid_t pid; > - int noexec = 0; > + bool noexec = false; > > /* Do nothing if command substitution should not succeed. */ > if (flags & WRDE_NOCMD) > return WRDE_CMDSUB; > > - /* Don't fork() unless necessary */ > + /* Don't posix_spawn() unless necessary */ > if (!comm || !*comm) > return 0; > > @@ -898,19 +912,15 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, > return WRDE_NOSPACE; > > again: > - if ((pid = __fork ()) < 0) > + pid = exec_comm_child (comm, fildes, noexec ? false : flags & WRDE_SHOWERR, > + noexec); > + if (pid < 0) > { > - /* Bad */ > __close (fildes[0]); > __close (fildes[1]); > return WRDE_NOSPACE; > } > > - if (pid == 0) > - exec_comm_child (comm, fildes, noexec ? 0 : flags & WRDE_SHOWERR, noexec); > - > - /* Parent */ > - > /* If we are just testing the syntax, only wait. */ > if (noexec) > return (TEMP_FAILURE_RETRY (__waitpid (pid, &status, 0)) == pid > @@ -1091,7 +1101,7 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, > /* Check for syntax error (re-execute but with "-n" flag) */ > if (buflen < 1 && status != 0) > { > - noexec = 1; > + noexec = true; > goto again; > } > > @@ -1143,26 +1153,9 @@ parse_comm (char **word, size_t *word_length, size_t *max_length, > /* Go -- give script to the shell */ > if (comm) > { > -#ifdef __libc_ptf_call > - /* We do not want the exec_comm call to be cut short > - by a thread cancellation since cleanup is very > - ugly. Therefore disable cancellation for > - now. */ > - // XXX Ideally we do want the thread being cancelable. > - // XXX If demand is there we'll change it. > - int state = PTHREAD_CANCEL_ENABLE; > - __libc_ptf_call (__pthread_setcancelstate, > - (PTHREAD_CANCEL_DISABLE, &state), 0); > -#endif > - > + /* posix_spawn already handles thread cancellation. */ > error = exec_comm (comm, word, word_length, max_length, > flags, pwordexp, ifs, ifs_white); > - > -#ifdef __libc_ptf_call > - __libc_ptf_call (__pthread_setcancelstate, > - (state, NULL), 0); > -#endif > - > free (comm); > } > >
Ping (x2). On 28/08/2019 11:09, Adhemerval Zanella wrote: > Ping. > > On 31/07/2019 15:31, Adhemerval Zanella wrote: >> Change from previous version: >> >> - Use libsupport and remove atfork usage on posix/wordexp-test.c. >> >> -- >> >> This patch replaces the fork+exec by posix_spawn on wordexp, which >> allows a better scability on Linux and simplifies the thread >> cancellation handling. >> >> The only change which can not be implemented with posix_spawn the >> /dev/null check to certify it is indeed the expected device. I am >> not sure how effetive this check is since /dev/null tampering means >> something very wrong with the system and this is the least of the >> issues. My view is the tests is really out of the place and the >> hardening provided is minimum. >> >> If the idea is still to provide such check, I think a possibilty >> would be to open /dev/null, check it, add a dup2 file action, and >> close the file descriptor. >> >> Checked on powerpc64le-linux-gnu and x86_64-linux-gnu. >> >> * include/spawn.h (__posix_spawn_file_actions_addopen): New >> prototype. >> * posix/spawn_faction_addopen.c (posix_spawn_file_actions_addopen): >> Add internal alias. >> * posix/wordexp.c (create_environment, free_environment): New >> functions. >> (exec_comm_child, exec_comm): Use posix_spawn instead of fork+exec. >> * posix/wordexp-test.c: Use libsupport and remove atfork usage. >> --- >> include/spawn.h | 3 + >> posix/spawn_faction_addopen.c | 8 +- >> posix/wordexp-test.c | 142 +++++++++-------------------- >> posix/wordexp.c | 167 ++++++++++++++++------------------ >> 4 files changed, 129 insertions(+), 191 deletions(-) >> >> diff --git a/include/spawn.h b/include/spawn.h >> index 7fdd965bd7..4a0b1849da 100644 >> --- a/include/spawn.h >> +++ b/include/spawn.h >> @@ -11,6 +11,9 @@ __typeof (posix_spawn_file_actions_addclose) >> __typeof (posix_spawn_file_actions_adddup2) >> __posix_spawn_file_actions_adddup2 attribute_hidden; >> >> +__typeof (posix_spawn_file_actions_addopen) >> + __posix_spawn_file_actions_addopen attribute_hidden; >> + >> __typeof (posix_spawn_file_actions_destroy) >> __posix_spawn_file_actions_destroy attribute_hidden; >> >> diff --git a/posix/spawn_faction_addopen.c b/posix/spawn_faction_addopen.c >> index 742eb9526d..2e598de300 100644 >> --- a/posix/spawn_faction_addopen.c >> +++ b/posix/spawn_faction_addopen.c >> @@ -25,9 +25,9 @@ >> /* Add an action to FILE-ACTIONS which tells the implementation to call >> `open' for the given file during the `spawn' call. */ >> int >> -posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, >> - int fd, const char *path, int oflag, >> - mode_t mode) >> +__posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, >> + int fd, const char *path, int oflag, >> + mode_t mode) >> { >> struct __spawn_action *rec; >> >> @@ -60,3 +60,5 @@ posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, >> >> return 0; >> } >> +weak_alias (__posix_spawn_file_actions_addopen, >> + posix_spawn_file_actions_addopen) >> diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c >> index 10a0768a6b..ef780b0a65 100644 >> --- a/posix/wordexp-test.c >> +++ b/posix/wordexp-test.c >> @@ -15,39 +15,21 @@ >> License along with the GNU C Library; if not, see >> <http://www.gnu.org/licenses/>. */ >> >> -#include <sys/stat.h> >> -#include <sys/types.h> >> -#include <sys/mman.h> >> +#include <wordexp.h> >> +#include <stdio.h> >> #include <fcntl.h> >> -#include <unistd.h> >> #include <pwd.h> >> -#include <stdio.h> >> -#include <stdint.h> >> #include <stdlib.h> >> #include <string.h> >> -#include <wordexp.h> >> +#include <sys/mman.h> >> + >> #include <libc-pointer-arith.h> >> -#include <dso_handle.h> >> +#include <array_length.h> >> +#include <support/xunistd.h> >> +#include <support/check.h> >> >> #define IFS " \n\t" >> >> -extern int __register_atfork (void (*) (void), void (*) (void), void (*) (void), void *); >> - >> -static int __app_register_atfork (void (*prepare) (void), void (*parent) (void), void (*child) (void)) >> -{ >> - return __register_atfork (prepare, parent, child, __dso_handle); >> -} >> - >> -/* Number of forks seen. */ >> -static int registered_forks; >> - >> -/* For each fork increment the fork count. */ >> -static void >> -register_fork (void) >> -{ >> - registered_forks++; >> -} >> - >> struct test_case_struct >> { >> int retval; >> @@ -57,7 +39,7 @@ struct test_case_struct >> size_t wordc; >> const char *wordv[10]; >> const char *ifs; >> -} test_case[] = >> +} static test_case[] = >> { >> /* Simple word- and field-splitting */ >> { 0, NULL, "one", 0, 1, { "one", }, IFS }, >> @@ -238,8 +220,6 @@ struct test_case_struct >> { WRDE_SYNTAX, NULL, "${", 0, 0, { NULL, }, IFS }, /* BZ 18043 */ >> { WRDE_SYNTAX, NULL, "L${a:", 0, 0, { NULL, }, IFS }, /* BZ 18043#c4 */ >> { WRDE_SYNTAX, NULL, "$[1/0]", WRDE_NOCMD, 0, {NULL, }, IFS }, /* BZ 18100 */ >> - >> - { -1, NULL, NULL, 0, 0, { NULL, }, IFS }, >> }; >> >> static int testit (struct test_case_struct *tc); >> @@ -256,16 +236,14 @@ command_line_test (const char *words) >> printf ("we_wordv[%d] = \"%s\"\n", i, we.we_wordv[i]); >> } >> >> -int >> -main (int argc, char *argv[]) >> +static int >> +do_test (int argc, char *argv[]) >> { >> - const char *globfile[] = { "one", "two", "three", NULL }; >> + const char *globfile[] = { "one", "two", "three" }; >> char tmpdir[32]; >> struct passwd *pw; >> const char *cwd; >> int test; >> - int fail = 0; >> - int i; >> struct test_case_struct ts; >> >> if (argc > 1) >> @@ -278,30 +256,18 @@ main (int argc, char *argv[]) >> >> /* Set up arena for pathname expansion */ >> tmpnam (tmpdir); >> - if (mkdir (tmpdir, S_IRWXU) || chdir (tmpdir)) >> - return -1; >> - else >> - { >> - int fd; >> + xmkdir (tmpdir, S_IRWXU); >> + TEST_VERIFY_EXIT (chdir (tmpdir) == 0); >> >> - for (i = 0; globfile[i]; ++i) >> - if ((fd = creat (globfile[i], S_IRUSR | S_IWUSR)) == -1 >> - || close (fd)) >> - return -1; >> - } >> - >> - /* If we are not allowed to do command substitution, we install >> - fork handlers to verify that no forks happened. No forks should >> - happen at all if command substitution is disabled. */ >> - if (__app_register_atfork (register_fork, NULL, NULL) != 0) >> + for (int i = 0; i < array_length (globfile); ++i) >> { >> - printf ("Failed to register fork handler.\n"); >> - return -1; >> + int fd = xopen (globfile[i], O_WRONLY|O_CREAT|O_TRUNC, >> + S_IRUSR | S_IWUSR); >> + xclose (fd); >> } >> >> - for (test = 0; test_case[test].retval != -1; test++) >> - if (testit (&test_case[test])) >> - ++fail; >> + for (test = 0; test < array_length (test_case); test++) >> + TEST_COMPARE (testit (&test_case[test]), 0); >> >> /* Tilde-expansion tests. */ >> pw = getpwnam ("root"); >> @@ -315,8 +281,7 @@ main (int argc, char *argv[]) >> ts.wordv[0] = pw->pw_dir; >> ts.ifs = IFS; >> >> - if (testit (&ts)) >> - ++fail; >> + TEST_COMPARE (testit (&ts), 0); >> >> ts.retval = 0; >> ts.env = pw->pw_dir; >> @@ -326,8 +291,7 @@ main (int argc, char *argv[]) >> ts.wordv[0] = "x"; >> ts.ifs = IFS; >> >> - if (testit (&ts)) >> - ++fail; >> + TEST_COMPARE (testit (&ts), 0); >> } >> >> /* "~" expands to value of $HOME when HOME is set */ >> @@ -342,8 +306,7 @@ main (int argc, char *argv[]) >> ts.wordv[1] = "/dummy/home/foo"; >> ts.ifs = IFS; >> >> - if (testit (&ts)) >> - ++fail; >> + TEST_COMPARE (testit (&ts), 0); >> >> /* "~" expands to home dir from passwd file if HOME is not set */ >> >> @@ -359,8 +322,7 @@ main (int argc, char *argv[]) >> ts.wordv[0] = pw->pw_dir; >> ts.ifs = IFS; >> >> - if (testit (&ts)) >> - ++fail; >> + TEST_COMPARE (testit (&ts), 0); >> } >> >> /* Integer overflow in division. */ >> @@ -375,37 +337,32 @@ main (int argc, char *argv[]) >> "18446744073709551616", >> "170141183460469231731687303715884105728", >> "340282366920938463463374607431768211456", >> - NULL >> }; >> >> - for (const char *const *num = numbers; *num; ++num) >> + for (int i = 0; i < array_length (numbers); i++) >> { >> wordexp_t p; >> char pattern[256]; >> - snprintf (pattern, sizeof (pattern), "$[(-%s)/(-1)]", *num); >> + snprintf (pattern, sizeof (pattern), "$[(-%s)/(-1)]", numbers[i]); >> int ret = wordexp (pattern, &p, WRDE_NOCMD); >> if (ret == 0) >> { >> - if (p.we_wordc != 1 || strcmp (p.we_wordv[0], *num) != 0) >> - { >> - printf ("Integer overflow for \"%s\" failed", pattern); >> - ++fail; >> - } >> + TEST_COMPARE (p.we_wordc, 1); >> + TEST_COMPARE (strcmp (p.we_wordv[0], numbers[i]), 0); >> wordfree (&p); >> } >> - else if (ret != WRDE_SYNTAX) >> + else >> { >> - printf ("Integer overflow for \"%s\" failed with %d", >> - pattern, ret); >> - ++fail; >> + TEST_COMPARE (ret, WRDE_SYNTAX); >> + if (ret != WRDE_SYNTAX) >> + printf ("Integer overflow for \"%s\" failed with %d", >> + pattern, ret); >> } >> } >> } >> >> - puts ("tests completed, now cleaning up"); >> - >> /* Clean up */ >> - for (i = 0; globfile[i]; ++i) >> + for (int i = 0; i < array_length (globfile); ++i) >> remove (globfile[i]); >> >> if (cwd == NULL) >> @@ -414,26 +371,17 @@ main (int argc, char *argv[]) >> chdir (cwd); >> rmdir (tmpdir); >> >> - printf ("tests failed: %d\n", fail); >> - >> - return fail != 0; >> + return 0; >> } >> >> static const char * >> at_page_end (const char *words) >> { >> const int pagesize = getpagesize (); >> - char *start = mmap (0, 2 * pagesize, PROT_READ|PROT_WRITE, >> - MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); >> + char *start = xmmap (0, 2 * pagesize, PROT_READ | PROT_WRITE, >> + MAP_PRIVATE | MAP_ANONYMOUS, -1); >> >> - if (start == MAP_FAILED) >> - return start; >> - >> - if (mprotect (start + pagesize, pagesize, PROT_NONE)) >> - { >> - munmap (start, 2 * pagesize); >> - return MAP_FAILED; >> - } >> + xmprotect (start + pagesize, pagesize, PROT_NONE); >> >> /* Includes terminating NUL. */ >> const size_t words_size = strlen (words) + 1; >> @@ -472,9 +420,6 @@ testit (struct test_case_struct *tc) >> fflush (NULL); >> const char *words = at_page_end (tc->words); >> >> - if (tc->flags & WRDE_NOCMD) >> - registered_forks = 0; >> - >> if (tc->flags & WRDE_APPEND) >> { >> /* initial wordexp() call, to be appended to */ >> @@ -486,13 +431,6 @@ testit (struct test_case_struct *tc) >> } >> retval = wordexp (words, &we, tc->flags); >> >> - if ((tc->flags & WRDE_NOCMD) >> - && (registered_forks > 0)) >> - { >> - printf ("FAILED fork called for WRDE_NOCMD\n"); >> - return 1; >> - } >> - >> if (tc->flags & WRDE_DOOFFS) >> start_offs = sav_we.we_offs; >> >> @@ -551,9 +489,11 @@ testit (struct test_case_struct *tc) >> const int page_size = getpagesize (); >> char *start = (char *) PTR_ALIGN_DOWN (words, page_size); >> >> - if (munmap (start, 2 * page_size) != 0) >> - return 1; >> + xmunmap (start, 2 * page_size); >> >> fflush (NULL); >> return bzzzt; >> } >> + >> +#define TEST_FUNCTION_ARGV do_test >> +#include <support/test-driver.c> >> diff --git a/posix/wordexp.c b/posix/wordexp.c >> index 22c6d18a9c..e1aafcaceb 100644 >> --- a/posix/wordexp.c >> +++ b/posix/wordexp.c >> @@ -25,33 +25,18 @@ >> #include <libintl.h> >> #include <paths.h> >> #include <pwd.h> >> -#include <signal.h> >> #include <stdbool.h> >> #include <stdio.h> >> -#include <stdlib.h> >> #include <string.h> >> #include <sys/param.h> >> -#include <sys/stat.h> >> -#include <sys/time.h> >> -#include <sys/types.h> >> -#include <sys/types.h> >> #include <sys/wait.h> >> #include <unistd.h> >> -#include <wchar.h> >> #include <wordexp.h> >> -#include <kernel-features.h> >> +#include <spawn.h> >> #include <scratch_buffer.h> >> - >> -#include <libc-lock.h> >> #include <_itoa.h> >> - >> -/* Undefine the following line for the production version. */ >> -/* #define NDEBUG 1 */ >> #include <assert.h> >> >> -/* Get some device information. */ >> -#include <device-nrs.h> >> - >> /* >> * This is a recursive-descent-style word expansion routine. >> */ >> @@ -812,61 +797,90 @@ parse_arith (char **word, size_t *word_length, size_t *max_length, >> return WRDE_SYNTAX; >> } >> >> +static char ** >> +create_environment (void) >> +{ >> + size_t s = 0; >> + >> + /* Calculate total environment size, including 'IFS' if is present. */ >> + for (char **ep = __environ; *ep != NULL; ep++, s++); >> + >> + /* Include final NULL pointer. */ >> + char **newenviron = malloc (s * sizeof (char*)); >> + if (newenviron == NULL) >> + return NULL; >> + >> + /* Copy current environment excluding 'IFS', to make sure the subshell >> + doesn't field-split on our behalf. */ >> + size_t i, j; >> + for (i = 0, j = 0; i < s; i++) >> + if (strncmp (__environ[i], "IFS=", sizeof ("IFS=")-1) != 0) >> + newenviron[j++] = __strdup (__environ[i]); >> + newenviron[j] = NULL; >> + >> + return newenviron; >> +} >> + >> +static void >> +free_environment (char **environ) >> +{ >> + for (char **ep = environ; *ep != NULL; ep++) >> + free (*ep); >> + free (environ); >> +} >> + >> /* Function called by child process in exec_comm() */ >> -static inline void >> -__attribute__ ((always_inline)) >> -exec_comm_child (char *comm, int *fildes, int showerr, int noexec) >> +static pid_t >> +exec_comm_child (char *comm, int *fildes, bool showerr, bool noexec) >> { >> - const char *args[4] = { _PATH_BSHELL, "-c", comm, NULL }; >> + pid_t pid = -1; >> >> - /* Execute the command, or just check syntax? */ >> - if (noexec) >> - args[1] = "-nc"; >> + /* Execute the command, or just check syntax? */ >> + const char *args[] = { _PATH_BSHELL, noexec ? "-nc" : "-c", comm, NULL }; >> >> - /* Redirect output. */ >> - if (__glibc_likely (fildes[1] != STDOUT_FILENO)) >> - { >> - __dup2 (fildes[1], STDOUT_FILENO); >> - __close (fildes[1]); >> - } >> - else >> - /* Reset the close-on-exec flag (if necessary). */ >> - __fcntl (fildes[1], F_SETFD, 0); >> + posix_spawn_file_actions_t fa; >> + /* posix_spawn_file_actions_init does not fail. */ >> + __posix_spawn_file_actions_init (&fa); >> >> - /* Redirect stderr to /dev/null if we have to. */ >> - if (showerr == 0) >> + /* Redirect output. For check syntax only (noexec being true), exec_comm >> + explicits sets fildes[1] to -1, so check its value to avoid a failure in >> + __posix_spawn_file_actions_adddup2. */ >> + if (fildes[1] != -1) >> { >> - struct stat64 st; >> - int fd; >> - __close (STDERR_FILENO); >> - fd = __open (_PATH_DEVNULL, O_WRONLY); >> - if (fd >= 0 && fd != STDERR_FILENO) >> + if (__glibc_likely (fildes[1] != STDOUT_FILENO)) >> { >> - __dup2 (fd, STDERR_FILENO); >> - __close (fd); >> + if (__posix_spawn_file_actions_adddup2 (&fa, fildes[1], >> + STDOUT_FILENO) != 0 >> + || __posix_spawn_file_actions_addclose (&fa, fildes[1]) != 0) >> + goto out; >> } >> - /* Be paranoid. Check that we actually opened the /dev/null >> - device. */ >> - if (__builtin_expect (__fxstat64 (_STAT_VER, STDERR_FILENO, &st), 0) != 0 >> - || __builtin_expect (S_ISCHR (st.st_mode), 1) == 0 >> -#if defined DEV_NULL_MAJOR && defined DEV_NULL_MINOR >> - || st.st_rdev != __gnu_dev_makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR) >> -#endif >> - ) >> - /* It's not the /dev/null device. Stop right here. The >> - problem is: how do we stop? We use _exit() with an >> - hopefully unusual exit code. */ >> - _exit (90); >> + else >> + /* Reset the close-on-exec flag (if necessary). */ >> + if (__posix_spawn_file_actions_adddup2 (&fa, fildes[1], fildes[1]) >> + != 0) >> + goto out; >> } >> >> - /* Make sure the subshell doesn't field-split on our behalf. */ >> - __unsetenv ("IFS"); >> + /* Redirect stderr to /dev/null if we have to. */ >> + if (!showerr) >> + if (__posix_spawn_file_actions_addopen (&fa, STDERR_FILENO, _PATH_DEVNULL, >> + O_WRONLY, 0) != 0) >> + goto out; >> + >> + char **newenv = create_environment (); >> + if (newenv == NULL) >> + goto out; >> >> - __close (fildes[0]); >> - __execve (_PATH_BSHELL, (char *const *) args, __environ); >> + /* pid is unset if posix_spawn fails, so it keep the original value >> + of -1. */ >> + __posix_spawn (&pid, _PATH_BSHELL, &fa, NULL, (char *const *) args, newenv); >> >> - /* Bad. What now? */ >> - abort (); >> + free_environment (newenv); >> + >> +out: >> + __posix_spawn_file_actions_destroy (&fa); >> + >> + return pid; >> } >> >> /* Function to execute a command and retrieve the results */ >> @@ -884,13 +898,13 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, >> size_t maxnewlines = 0; >> char buffer[bufsize]; >> pid_t pid; >> - int noexec = 0; >> + bool noexec = false; >> >> /* Do nothing if command substitution should not succeed. */ >> if (flags & WRDE_NOCMD) >> return WRDE_CMDSUB; >> >> - /* Don't fork() unless necessary */ >> + /* Don't posix_spawn() unless necessary */ >> if (!comm || !*comm) >> return 0; >> >> @@ -898,19 +912,15 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, >> return WRDE_NOSPACE; >> >> again: >> - if ((pid = __fork ()) < 0) >> + pid = exec_comm_child (comm, fildes, noexec ? false : flags & WRDE_SHOWERR, >> + noexec); >> + if (pid < 0) >> { >> - /* Bad */ >> __close (fildes[0]); >> __close (fildes[1]); >> return WRDE_NOSPACE; >> } >> >> - if (pid == 0) >> - exec_comm_child (comm, fildes, noexec ? 0 : flags & WRDE_SHOWERR, noexec); >> - >> - /* Parent */ >> - >> /* If we are just testing the syntax, only wait. */ >> if (noexec) >> return (TEMP_FAILURE_RETRY (__waitpid (pid, &status, 0)) == pid >> @@ -1091,7 +1101,7 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, >> /* Check for syntax error (re-execute but with "-n" flag) */ >> if (buflen < 1 && status != 0) >> { >> - noexec = 1; >> + noexec = true; >> goto again; >> } >> >> @@ -1143,26 +1153,9 @@ parse_comm (char **word, size_t *word_length, size_t *max_length, >> /* Go -- give script to the shell */ >> if (comm) >> { >> -#ifdef __libc_ptf_call >> - /* We do not want the exec_comm call to be cut short >> - by a thread cancellation since cleanup is very >> - ugly. Therefore disable cancellation for >> - now. */ >> - // XXX Ideally we do want the thread being cancelable. >> - // XXX If demand is there we'll change it. >> - int state = PTHREAD_CANCEL_ENABLE; >> - __libc_ptf_call (__pthread_setcancelstate, >> - (PTHREAD_CANCEL_DISABLE, &state), 0); >> -#endif >> - >> + /* posix_spawn already handles thread cancellation. */ >> error = exec_comm (comm, word, word_length, max_length, >> flags, pwordexp, ifs, ifs_white); >> - >> -#ifdef __libc_ptf_call >> - __libc_ptf_call (__pthread_setcancelstate, >> - (state, NULL), 0); >> -#endif >> - >> free (comm); >> } >> >>
* Adhemerval Zanella: > diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c > index 10a0768a6b..ef780b0a65 100644 > --- a/posix/wordexp-test.c > +++ b/posix/wordexp-test.c > -/* For each fork increment the fork count. */ > -static void > -register_fork (void) > -{ > - registered_forks++; > -} It's a bit sad to see this testing go away. It was originally added to catch command execution with WRDE_NOCMD. On Linux, could you enter a PID namespace instead and check that the next PID has the expected value? Carlos, you added this testing. Do you have an opinion here? > diff --git a/posix/wordexp.c b/posix/wordexp.c > index 22c6d18a9c..e1aafcaceb 100644 > --- a/posix/wordexp.c > +++ b/posix/wordexp.c > +static char ** > +create_environment (void) > +{ > + size_t s = 0; > + > + /* Calculate total environment size, including 'IFS' if is present. */ > + for (char **ep = __environ; *ep != NULL; ep++, s++); I would put s++ into the body of the for loop, for clarity. Or give ep a wider scope and use s = ep -- __environ. > + /* Include final NULL pointer. */ > + char **newenviron = malloc (s * sizeof (char*)); > + if (newenviron == NULL) > + return NULL; char* should be char *. I don't see how this includes the final NULL? Should we do all this work only if IFS= is actually present? That is, skip all this for getenv ("IFS) == NULL? > + /* Copy current environment excluding 'IFS', to make sure the subshell > + doesn't field-split on our behalf. */ That comment should apply to the entire function, I think. > @@ -884,13 +898,13 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, > size_t maxnewlines = 0; > char buffer[bufsize]; > pid_t pid; > - int noexec = 0; > + bool noexec = false; > > /* Do nothing if command substitution should not succeed. */ > if (flags & WRDE_NOCMD) > return WRDE_CMDSUB; > > - /* Don't fork() unless necessary */ > + /* Don't posix_spawn() unless necessary */ GNU style doesn't use () after function names. Thanks, Florian
On 10/7/19 3:33 PM, Florian Weimer wrote: > * Adhemerval Zanella: > >> diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c >> index 10a0768a6b..ef780b0a65 100644 >> --- a/posix/wordexp-test.c >> +++ b/posix/wordexp-test.c > >> -/* For each fork increment the fork count. */ >> -static void >> -register_fork (void) >> -{ >> - registered_forks++; >> -} > > It's a bit sad to see this testing go away. It was originally added to > catch command execution with WRDE_NOCMD. > > On Linux, could you enter a PID namespace instead and check that the > next PID has the expected value? > > Carlos, you added this testing. Do you have an opinion here? We should not regress testing WRDE_NOCMD, because doing so is what lead to CVE-2014-7817 :-( We should expend some effort here to provide robust testing for WRDE_NOCMD. All 3 tests I added rely on registered_forks testing to verify correct operation of WRDE_NOCMD. Is there anything we can do about this Adhemerval? -- Cheers, Carlos.
* Carlos O'Donell: > On 10/7/19 3:33 PM, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c >>> index 10a0768a6b..ef780b0a65 100644 >>> --- a/posix/wordexp-test.c >>> +++ b/posix/wordexp-test.c >> >>> -/* For each fork increment the fork count. */ >>> -static void >>> -register_fork (void) >>> -{ >>> - registered_forks++; >>> -} >> >> It's a bit sad to see this testing go away. It was originally added to >> catch command execution with WRDE_NOCMD. >> >> On Linux, could you enter a PID namespace instead and check that the >> next PID has the expected value? >> >> Carlos, you added this testing. Do you have an opinion here? > > We should not regress testing WRDE_NOCMD, because doing so is what > lead to CVE-2014-7817 :-( > > We should expend some effort here to provide robust testing for > WRDE_NOCMD. I'm working on it. Thanks, Florian
On 07/10/2019 16:33, Florian Weimer wrote: > * Adhemerval Zanella: > >> diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c >> index 10a0768a6b..ef780b0a65 100644 >> --- a/posix/wordexp-test.c >> +++ b/posix/wordexp-test.c > >> -/* For each fork increment the fork count. */ >> -static void >> -register_fork (void) >> -{ >> - registered_forks++; >> -} > > It's a bit sad to see this testing go away. It was originally added to > catch command execution with WRDE_NOCMD. > > On Linux, could you enter a PID namespace instead and check that the > next PID has the expected value? > > Carlos, you added this testing. Do you have an opinion here? > >> diff --git a/posix/wordexp.c b/posix/wordexp.c >> index 22c6d18a9c..e1aafcaceb 100644 >> --- a/posix/wordexp.c >> +++ b/posix/wordexp.c > >> +static char ** >> +create_environment (void) >> +{ >> + size_t s = 0; >> + >> + /* Calculate total environment size, including 'IFS' if is present. */ >> + for (char **ep = __environ; *ep != NULL; ep++, s++); > > I would put s++ into the body of the for loop, for clarity. Or give ep > a wider scope and use s = ep -- __environ. Ack, I moved s++ to main loop. > >> + /* Include final NULL pointer. */ >> + char **newenviron = malloc (s * sizeof (char*)); >> + if (newenviron == NULL) >> + return NULL; > > char* should be char *. I don't see how this includes the final NULL? > > Should we do all this work only if IFS= is actually present? That is, > skip all this for getenv ("IFS) == NULL? Ack. > >> + /* Copy current environment excluding 'IFS', to make sure the subshell >> + doesn't field-split on our behalf. */ > > That comment should apply to the entire function, I think. Ack. > >> @@ -884,13 +898,13 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, >> size_t maxnewlines = 0; >> char buffer[bufsize]; >> pid_t pid; >> - int noexec = 0; >> + bool noexec = false; >> >> /* Do nothing if command substitution should not succeed. */ >> if (flags & WRDE_NOCMD) >> return WRDE_CMDSUB; >> >> - /* Don't fork() unless necessary */ >> + /* Don't posix_spawn() unless necessary */ > > GNU style doesn't use () after function names. Ack. I fixed your remarks, rebased against master to pick-up your changed to wordexp tests and used dynarray to construct the new environment (it simplifies a bit the creation on a new environment for the IFS case). * include/spawn.h (__posix_spawn_file_actions_addopen): New prototype. * posix/spawn_faction_addopen.c (posix_spawn_file_actions_addopen): Add internal alias. * posix/wordexp.c (create_environment, free_environment): New functions. (exec_comm_child, exec_comm): Use posix_spawn instead of fork+exec. * posix/wordexp-test.c: Use libsupport and remove atfork usage. --- diff --git a/include/spawn.h b/include/spawn.h index 7fdd965bd7..4a0b1849da 100644 --- a/include/spawn.h +++ b/include/spawn.h @@ -11,6 +11,9 @@ __typeof (posix_spawn_file_actions_addclose) __typeof (posix_spawn_file_actions_adddup2) __posix_spawn_file_actions_adddup2 attribute_hidden; +__typeof (posix_spawn_file_actions_addopen) + __posix_spawn_file_actions_addopen attribute_hidden; + __typeof (posix_spawn_file_actions_destroy) __posix_spawn_file_actions_destroy attribute_hidden; diff --git a/posix/spawn_faction_addopen.c b/posix/spawn_faction_addopen.c index d5694ee4d7..4fd64bb005 100644 --- a/posix/spawn_faction_addopen.c +++ b/posix/spawn_faction_addopen.c @@ -25,9 +25,9 @@ /* Add an action to FILE-ACTIONS which tells the implementation to call `open' for the given file during the `spawn' call. */ int -posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, - int fd, const char *path, int oflag, - mode_t mode) +__posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, + int fd, const char *path, int oflag, + mode_t mode) { struct __spawn_action *rec; @@ -60,3 +60,5 @@ posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, return 0; } +weak_alias (__posix_spawn_file_actions_addopen, + posix_spawn_file_actions_addopen) diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c index a4d8bcf1da..34836f6240 100644 --- a/posix/wordexp-test.c +++ b/posix/wordexp-test.c @@ -15,19 +15,18 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ -#include <sys/stat.h> -#include <sys/types.h> -#include <sys/mman.h> +#include <wordexp.h> +#include <stdio.h> #include <fcntl.h> -#include <unistd.h> #include <pwd.h> -#include <stdio.h> -#include <stdint.h> #include <stdlib.h> #include <string.h> -#include <wordexp.h> +#include <sys/mman.h> + #include <libc-pointer-arith.h> -#include <dso_handle.h> +#include <array_length.h> +#include <support/xunistd.h> +#include <support/check.h> #define IFS " \n\t" @@ -40,7 +39,7 @@ struct test_case_struct size_t wordc; const char *wordv[10]; const char *ifs; -} test_case[] = +} static test_case[] = { /* Simple word- and field-splitting */ { 0, NULL, "one", 0, 1, { "one", }, IFS }, @@ -213,8 +212,6 @@ struct test_case_struct { WRDE_SYNTAX, NULL, "`\\", 0, 0, { NULL, }, IFS }, /* BZ 18042 */ { WRDE_SYNTAX, NULL, "${", 0, 0, { NULL, }, IFS }, /* BZ 18043 */ { WRDE_SYNTAX, NULL, "L${a:", 0, 0, { NULL, }, IFS }, /* BZ 18043#c4 */ - - { -1, NULL, NULL, 0, 0, { NULL, }, IFS }, }; static int testit (struct test_case_struct *tc); @@ -226,21 +223,19 @@ command_line_test (const char *words) wordexp_t we; int i; int retval = wordexp (words, &we, 0); - printf ("wordexp returned %d\n", retval); + printf ("info: wordexp returned %d\n", retval); for (i = 0; i < we.we_wordc; i++) - printf ("we_wordv[%d] = \"%s\"\n", i, we.we_wordv[i]); + printf ("info: we_wordv[%d] = \"%s\"\n", i, we.we_wordv[i]); } -int -main (int argc, char *argv[]) +static int +do_test (int argc, char *argv[]) { - const char *globfile[] = { "one", "two", "three", NULL }; + const char *globfile[] = { "one", "two", "three" }; char tmpdir[32]; struct passwd *pw; const char *cwd; int test; - int fail = 0; - int i; struct test_case_struct ts; if (argc > 1) @@ -253,21 +248,18 @@ main (int argc, char *argv[]) /* Set up arena for pathname expansion */ tmpnam (tmpdir); - if (mkdir (tmpdir, S_IRWXU) || chdir (tmpdir)) - return -1; - else - { - int fd; + xmkdir (tmpdir, S_IRWXU); + TEST_VERIFY_EXIT (chdir (tmpdir) == 0); - for (i = 0; globfile[i]; ++i) - if ((fd = creat (globfile[i], S_IRUSR | S_IWUSR)) == -1 - || close (fd)) - return -1; + for (int i = 0; i < array_length (globfile); ++i) + { + int fd = xopen (globfile[i], O_WRONLY|O_CREAT|O_TRUNC, + S_IRUSR | S_IWUSR); + xclose (fd); } - for (test = 0; test_case[test].retval != -1; test++) - if (testit (&test_case[test])) - ++fail; + for (test = 0; test < array_length (test_case); test++) + TEST_COMPARE (testit (&test_case[test]), 0); /* Tilde-expansion tests. */ pw = getpwnam ("root"); @@ -281,8 +273,7 @@ main (int argc, char *argv[]) ts.wordv[0] = pw->pw_dir; ts.ifs = IFS; - if (testit (&ts)) - ++fail; + TEST_COMPARE (testit (&ts), 0); ts.retval = 0; ts.env = pw->pw_dir; @@ -292,8 +283,7 @@ main (int argc, char *argv[]) ts.wordv[0] = "x"; ts.ifs = IFS; - if (testit (&ts)) - ++fail; + TEST_COMPARE (testit (&ts), 0); } /* "~" expands to value of $HOME when HOME is set */ @@ -308,8 +298,7 @@ main (int argc, char *argv[]) ts.wordv[1] = "/dummy/home/foo"; ts.ifs = IFS; - if (testit (&ts)) - ++fail; + TEST_COMPARE (testit (&ts), 0); /* "~" expands to home dir from passwd file if HOME is not set */ @@ -325,14 +314,13 @@ main (int argc, char *argv[]) ts.wordv[0] = pw->pw_dir; ts.ifs = IFS; - if (testit (&ts)) - ++fail; + TEST_COMPARE (testit (&ts), 0); } puts ("tests completed, now cleaning up"); /* Clean up */ - for (i = 0; globfile[i]; ++i) + for (int i = 0; i < array_length (globfile); ++i) remove (globfile[i]); if (cwd == NULL) @@ -341,26 +329,17 @@ main (int argc, char *argv[]) chdir (cwd); rmdir (tmpdir); - printf ("tests failed: %d\n", fail); - - return fail != 0; + return 0; } static const char * at_page_end (const char *words) { const int pagesize = getpagesize (); - char *start = mmap (0, 2 * pagesize, PROT_READ|PROT_WRITE, - MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); + char *start = xmmap (0, 2 * pagesize, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1); - if (start == MAP_FAILED) - return start; - - if (mprotect (start + pagesize, pagesize, PROT_NONE)) - { - munmap (start, 2 * pagesize); - return MAP_FAILED; - } + xmprotect (start + pagesize, pagesize, PROT_NONE); /* Includes terminating NUL. */ const size_t words_size = strlen (words) + 1; @@ -395,7 +374,7 @@ testit (struct test_case_struct *tc) sav_we.we_offs = 3; we = sav_we; - printf ("Test %d (%s): ", ++tests, tc->words); + printf ("info: test %d (%s): ", ++tests, tc->words); fflush (NULL); const char *words = at_page_end (tc->words); @@ -404,7 +383,7 @@ testit (struct test_case_struct *tc) /* initial wordexp() call, to be appended to */ if (wordexp ("pre1 pre2", &we, tc->flags & ~WRDE_APPEND) != 0) { - printf ("FAILED setup\n"); + printf ("info: FAILED setup\n"); return 1; } } @@ -436,7 +415,7 @@ testit (struct test_case_struct *tc) if (bzzzt) { printf ("FAILED\n"); - printf ("Test words: <%s>, need retval %d, wordc %Zd\n", + printf ("info: Test words: <%s>, need retval %d, wordc %Zd\n", tc->words, tc->retval, tc->wordc); if (start_offs != 0) printf ("(preceded by %d NULLs)\n", start_offs); @@ -468,9 +447,11 @@ testit (struct test_case_struct *tc) const int page_size = getpagesize (); char *start = (char *) PTR_ALIGN_DOWN (words, page_size); - if (munmap (start, 2 * page_size) != 0) - return 1; + xmunmap (start, 2 * page_size); fflush (NULL); return bzzzt; } + +#define TEST_FUNCTION_ARGV do_test +#include <support/test-driver.c> diff --git a/posix/wordexp.c b/posix/wordexp.c index 6a6e3a8e11..ea1ada1d2a 100644 --- a/posix/wordexp.c +++ b/posix/wordexp.c @@ -25,33 +25,18 @@ #include <libintl.h> #include <paths.h> #include <pwd.h> -#include <signal.h> #include <stdbool.h> #include <stdio.h> -#include <stdlib.h> #include <string.h> #include <sys/param.h> -#include <sys/stat.h> -#include <sys/time.h> -#include <sys/types.h> -#include <sys/types.h> #include <sys/wait.h> #include <unistd.h> -#include <wchar.h> #include <wordexp.h> -#include <kernel-features.h> +#include <spawn.h> #include <scratch_buffer.h> - -#include <libc-lock.h> #include <_itoa.h> - -/* Undefine the following line for the production version. */ -/* #define NDEBUG 1 */ #include <assert.h> -/* Get some device information. */ -#include <device-nrs.h> - /* * This is a recursive-descent-style word expansion routine. */ @@ -812,61 +797,76 @@ parse_arith (char **word, size_t *word_length, size_t *max_length, return WRDE_SYNTAX; } +#define DYNARRAY_STRUCT strlist +#define DYNARRAY_ELEMENT char * +#define DYNARRAY_PREFIX strlist_ +/* Allocates about 512/1024 (32/64 bit) on stack. */ +#define DYNARRAY_INITIAL_SIZE 128 +#include <malloc/dynarray-skeleton.c> + /* Function called by child process in exec_comm() */ -static inline void -__attribute__ ((always_inline)) -exec_comm_child (char *comm, int *fildes, int showerr, int noexec) +static pid_t +exec_comm_child (char *comm, int *fildes, bool showerr, bool noexec) { - const char *args[4] = { _PATH_BSHELL, "-c", comm, NULL }; + pid_t pid = -1; - /* Execute the command, or just check syntax? */ - if (noexec) - args[1] = "-nc"; + /* Execute the command, or just check syntax? */ + const char *args[] = { _PATH_BSHELL, noexec ? "-nc" : "-c", comm, NULL }; + + posix_spawn_file_actions_t fa; + /* posix_spawn_file_actions_init does not fail. */ + __posix_spawn_file_actions_init (&fa); - /* Redirect output. */ - if (__glibc_likely (fildes[1] != STDOUT_FILENO)) + /* Redirect output. For check syntax only (noexec being true), exec_comm + explicits sets fildes[1] to -1, so check its value to avoid a failure in + __posix_spawn_file_actions_adddup2. */ + if (fildes[1] != -1) { - __dup2 (fildes[1], STDOUT_FILENO); - __close (fildes[1]); + if (__glibc_likely (fildes[1] != STDOUT_FILENO)) + { + if (__posix_spawn_file_actions_adddup2 (&fa, fildes[1], + STDOUT_FILENO) != 0 + || __posix_spawn_file_actions_addclose (&fa, fildes[1]) != 0) + goto out; + } + else + /* Reset the close-on-exec flag (if necessary). */ + if (__posix_spawn_file_actions_adddup2 (&fa, fildes[1], fildes[1]) + != 0) + goto out; } - else - /* Reset the close-on-exec flag (if necessary). */ - __fcntl (fildes[1], F_SETFD, 0); /* Redirect stderr to /dev/null if we have to. */ - if (showerr == 0) + if (!showerr) + if (__posix_spawn_file_actions_addopen (&fa, STDERR_FILENO, _PATH_DEVNULL, + O_WRONLY, 0) != 0) + goto out; + + struct strlist newenv; + strlist_init (&newenv); + + bool recreate_env = getenv ("IFS") != NULL; + if (recreate_env) { - struct stat64 st; - int fd; - __close (STDERR_FILENO); - fd = __open (_PATH_DEVNULL, O_WRONLY); - if (fd >= 0 && fd != STDERR_FILENO) - { - __dup2 (fd, STDERR_FILENO); - __close (fd); - } - /* Be paranoid. Check that we actually opened the /dev/null - device. */ - if (__builtin_expect (__fxstat64 (_STAT_VER, STDERR_FILENO, &st), 0) != 0 - || __builtin_expect (S_ISCHR (st.st_mode), 1) == 0 -#if defined DEV_NULL_MAJOR && defined DEV_NULL_MINOR - || st.st_rdev != __gnu_dev_makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR) -#endif - ) - /* It's not the /dev/null device. Stop right here. The - problem is: how do we stop? We use _exit() with an - hopefully unusual exit code. */ - _exit (90); + for (char **ep = __environ; *ep != NULL; ep++) + if (strncmp (*ep, "IFS=", sizeof ("IFS=")-1) != 0) + strlist_add (&newenv, *ep); + strlist_add (&newenv, NULL); + if (strlist_has_failed (&newenv)) + goto out; } - /* Make sure the subshell doesn't field-split on our behalf. */ - __unsetenv ("IFS"); + /* pid is unset if posix_spawn fails, so it keep the original value + of -1. */ + __posix_spawn (&pid, _PATH_BSHELL, &fa, NULL, (char *const *) args, + recreate_env ? strlist_begin (&newenv) : __environ); - __close (fildes[0]); - __execve (_PATH_BSHELL, (char *const *) args, __environ); + strlist_free (&newenv); + +out: + __posix_spawn_file_actions_destroy (&fa); - /* Bad. What now? */ - abort (); + return pid; } /* Function to execute a command and retrieve the results */ @@ -884,13 +884,13 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, size_t maxnewlines = 0; char buffer[bufsize]; pid_t pid; - int noexec = 0; + bool noexec = false; /* Do nothing if command substitution should not succeed. */ if (flags & WRDE_NOCMD) return WRDE_CMDSUB; - /* Don't fork() unless necessary */ + /* Don't posix_spawn unless necessary */ if (!comm || !*comm) return 0; @@ -898,19 +898,15 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, return WRDE_NOSPACE; again: - if ((pid = __fork ()) < 0) + pid = exec_comm_child (comm, fildes, noexec ? false : flags & WRDE_SHOWERR, + noexec); + if (pid < 0) { - /* Bad */ __close (fildes[0]); __close (fildes[1]); return WRDE_NOSPACE; } - if (pid == 0) - exec_comm_child (comm, fildes, noexec ? 0 : flags & WRDE_SHOWERR, noexec); - - /* Parent */ - /* If we are just testing the syntax, only wait. */ if (noexec) return (TEMP_FAILURE_RETRY (__waitpid (pid, &status, 0)) == pid @@ -1091,7 +1087,7 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, /* Check for syntax error (re-execute but with "-n" flag) */ if (buflen < 1 && status != 0) { - noexec = 1; + noexec = true; goto again; } @@ -1143,26 +1139,9 @@ parse_comm (char **word, size_t *word_length, size_t *max_length, /* Go -- give script to the shell */ if (comm) { -#ifdef __libc_ptf_call - /* We do not want the exec_comm call to be cut short - by a thread cancellation since cleanup is very - ugly. Therefore disable cancellation for - now. */ - // XXX Ideally we do want the thread being cancelable. - // XXX If demand is there we'll change it. - int state = PTHREAD_CANCEL_ENABLE; - __libc_ptf_call (__pthread_setcancelstate, - (PTHREAD_CANCEL_DISABLE, &state), 0); -#endif - + /* posix_spawn already handles thread cancellation. */ error = exec_comm (comm, word, word_length, max_length, flags, pwordexp, ifs, ifs_white); - -#ifdef __libc_ptf_call - __libc_ptf_call (__pthread_setcancelstate, - (state, NULL), 0); -#endif - free (comm); }
Thanks for the updated patch. * Adhemerval Zanella: > static const char * > at_page_end (const char *words) > { > const int pagesize = getpagesize (); > - char *start = mmap (0, 2 * pagesize, PROT_READ|PROT_WRITE, > - MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); > + char *start = xmmap (0, 2 * pagesize, PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANONYMOUS, -1); > > - if (start == MAP_FAILED) > - return start; > - > - if (mprotect (start + pagesize, pagesize, PROT_NONE)) > - { > - munmap (start, 2 * pagesize); > - return MAP_FAILED; > - } > + xmprotect (start + pagesize, pagesize, PROT_NONE); I believe you can use <support/next_to_fault.h> for that. > + if (strncmp (*ep, "IFS=", sizeof ("IFS=")-1) != 0) Missing spaces around -. In my opinion, you should just call strlen. GCC will fold it to a constant. > /* pid is unset if posix_spawn fails, so it keep the original value “pid is not set” or “pid is not updated”, I think. Rest looks okay to me. Thanks, Florian
On 09/10/2019 06:11, Florian Weimer wrote: > Thanks for the updated patch. > > * Adhemerval Zanella: > >> static const char * >> at_page_end (const char *words) >> { >> const int pagesize = getpagesize (); >> - char *start = mmap (0, 2 * pagesize, PROT_READ|PROT_WRITE, >> - MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); >> + char *start = xmmap (0, 2 * pagesize, PROT_READ | PROT_WRITE, >> + MAP_PRIVATE | MAP_ANONYMOUS, -1); >> >> - if (start == MAP_FAILED) >> - return start; >> - >> - if (mprotect (start + pagesize, pagesize, PROT_NONE)) >> - { >> - munmap (start, 2 * pagesize); >> - return MAP_FAILED; >> - } >> + xmprotect (start + pagesize, pagesize, PROT_NONE); > > I believe you can use <support/next_to_fault.h> for that. Ack, I will change to use it. > >> + if (strncmp (*ep, "IFS=", sizeof ("IFS=")-1) != 0) > > Missing spaces around -. In my opinion, you should just call strlen. > GCC will fold it to a constant. Ack. > >> /* pid is unset if posix_spawn fails, so it keep the original value > > “pid is not set” or “pid is not updated”, I think. Ack. > > Rest looks okay to me. > > Thanks, > Florian >
diff --git a/include/spawn.h b/include/spawn.h index 7fdd965bd7..4a0b1849da 100644 --- a/include/spawn.h +++ b/include/spawn.h @@ -11,6 +11,9 @@ __typeof (posix_spawn_file_actions_addclose) __typeof (posix_spawn_file_actions_adddup2) __posix_spawn_file_actions_adddup2 attribute_hidden; +__typeof (posix_spawn_file_actions_addopen) + __posix_spawn_file_actions_addopen attribute_hidden; + __typeof (posix_spawn_file_actions_destroy) __posix_spawn_file_actions_destroy attribute_hidden; diff --git a/posix/spawn_faction_addopen.c b/posix/spawn_faction_addopen.c index 742eb9526d..2e598de300 100644 --- a/posix/spawn_faction_addopen.c +++ b/posix/spawn_faction_addopen.c @@ -25,9 +25,9 @@ /* Add an action to FILE-ACTIONS which tells the implementation to call `open' for the given file during the `spawn' call. */ int -posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, - int fd, const char *path, int oflag, - mode_t mode) +__posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, + int fd, const char *path, int oflag, + mode_t mode) { struct __spawn_action *rec; @@ -60,3 +60,5 @@ posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, return 0; } +weak_alias (__posix_spawn_file_actions_addopen, + posix_spawn_file_actions_addopen) diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c index 10a0768a6b..ef780b0a65 100644 --- a/posix/wordexp-test.c +++ b/posix/wordexp-test.c @@ -15,39 +15,21 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ -#include <sys/stat.h> -#include <sys/types.h> -#include <sys/mman.h> +#include <wordexp.h> +#include <stdio.h> #include <fcntl.h> -#include <unistd.h> #include <pwd.h> -#include <stdio.h> -#include <stdint.h> #include <stdlib.h> #include <string.h> -#include <wordexp.h> +#include <sys/mman.h> + #include <libc-pointer-arith.h> -#include <dso_handle.h> +#include <array_length.h> +#include <support/xunistd.h> +#include <support/check.h> #define IFS " \n\t" -extern int __register_atfork (void (*) (void), void (*) (void), void (*) (void), void *); - -static int __app_register_atfork (void (*prepare) (void), void (*parent) (void), void (*child) (void)) -{ - return __register_atfork (prepare, parent, child, __dso_handle); -} - -/* Number of forks seen. */ -static int registered_forks; - -/* For each fork increment the fork count. */ -static void -register_fork (void) -{ - registered_forks++; -} - struct test_case_struct { int retval; @@ -57,7 +39,7 @@ struct test_case_struct size_t wordc; const char *wordv[10]; const char *ifs; -} test_case[] = +} static test_case[] = { /* Simple word- and field-splitting */ { 0, NULL, "one", 0, 1, { "one", }, IFS }, @@ -238,8 +220,6 @@ struct test_case_struct { WRDE_SYNTAX, NULL, "${", 0, 0, { NULL, }, IFS }, /* BZ 18043 */ { WRDE_SYNTAX, NULL, "L${a:", 0, 0, { NULL, }, IFS }, /* BZ 18043#c4 */ { WRDE_SYNTAX, NULL, "$[1/0]", WRDE_NOCMD, 0, {NULL, }, IFS }, /* BZ 18100 */ - - { -1, NULL, NULL, 0, 0, { NULL, }, IFS }, }; static int testit (struct test_case_struct *tc); @@ -256,16 +236,14 @@ command_line_test (const char *words) printf ("we_wordv[%d] = \"%s\"\n", i, we.we_wordv[i]); } -int -main (int argc, char *argv[]) +static int +do_test (int argc, char *argv[]) { - const char *globfile[] = { "one", "two", "three", NULL }; + const char *globfile[] = { "one", "two", "three" }; char tmpdir[32]; struct passwd *pw; const char *cwd; int test; - int fail = 0; - int i; struct test_case_struct ts; if (argc > 1) @@ -278,30 +256,18 @@ main (int argc, char *argv[]) /* Set up arena for pathname expansion */ tmpnam (tmpdir); - if (mkdir (tmpdir, S_IRWXU) || chdir (tmpdir)) - return -1; - else - { - int fd; + xmkdir (tmpdir, S_IRWXU); + TEST_VERIFY_EXIT (chdir (tmpdir) == 0); - for (i = 0; globfile[i]; ++i) - if ((fd = creat (globfile[i], S_IRUSR | S_IWUSR)) == -1 - || close (fd)) - return -1; - } - - /* If we are not allowed to do command substitution, we install - fork handlers to verify that no forks happened. No forks should - happen at all if command substitution is disabled. */ - if (__app_register_atfork (register_fork, NULL, NULL) != 0) + for (int i = 0; i < array_length (globfile); ++i) { - printf ("Failed to register fork handler.\n"); - return -1; + int fd = xopen (globfile[i], O_WRONLY|O_CREAT|O_TRUNC, + S_IRUSR | S_IWUSR); + xclose (fd); } - for (test = 0; test_case[test].retval != -1; test++) - if (testit (&test_case[test])) - ++fail; + for (test = 0; test < array_length (test_case); test++) + TEST_COMPARE (testit (&test_case[test]), 0); /* Tilde-expansion tests. */ pw = getpwnam ("root"); @@ -315,8 +281,7 @@ main (int argc, char *argv[]) ts.wordv[0] = pw->pw_dir; ts.ifs = IFS; - if (testit (&ts)) - ++fail; + TEST_COMPARE (testit (&ts), 0); ts.retval = 0; ts.env = pw->pw_dir; @@ -326,8 +291,7 @@ main (int argc, char *argv[]) ts.wordv[0] = "x"; ts.ifs = IFS; - if (testit (&ts)) - ++fail; + TEST_COMPARE (testit (&ts), 0); } /* "~" expands to value of $HOME when HOME is set */ @@ -342,8 +306,7 @@ main (int argc, char *argv[]) ts.wordv[1] = "/dummy/home/foo"; ts.ifs = IFS; - if (testit (&ts)) - ++fail; + TEST_COMPARE (testit (&ts), 0); /* "~" expands to home dir from passwd file if HOME is not set */ @@ -359,8 +322,7 @@ main (int argc, char *argv[]) ts.wordv[0] = pw->pw_dir; ts.ifs = IFS; - if (testit (&ts)) - ++fail; + TEST_COMPARE (testit (&ts), 0); } /* Integer overflow in division. */ @@ -375,37 +337,32 @@ main (int argc, char *argv[]) "18446744073709551616", "170141183460469231731687303715884105728", "340282366920938463463374607431768211456", - NULL }; - for (const char *const *num = numbers; *num; ++num) + for (int i = 0; i < array_length (numbers); i++) { wordexp_t p; char pattern[256]; - snprintf (pattern, sizeof (pattern), "$[(-%s)/(-1)]", *num); + snprintf (pattern, sizeof (pattern), "$[(-%s)/(-1)]", numbers[i]); int ret = wordexp (pattern, &p, WRDE_NOCMD); if (ret == 0) { - if (p.we_wordc != 1 || strcmp (p.we_wordv[0], *num) != 0) - { - printf ("Integer overflow for \"%s\" failed", pattern); - ++fail; - } + TEST_COMPARE (p.we_wordc, 1); + TEST_COMPARE (strcmp (p.we_wordv[0], numbers[i]), 0); wordfree (&p); } - else if (ret != WRDE_SYNTAX) + else { - printf ("Integer overflow for \"%s\" failed with %d", - pattern, ret); - ++fail; + TEST_COMPARE (ret, WRDE_SYNTAX); + if (ret != WRDE_SYNTAX) + printf ("Integer overflow for \"%s\" failed with %d", + pattern, ret); } } } - puts ("tests completed, now cleaning up"); - /* Clean up */ - for (i = 0; globfile[i]; ++i) + for (int i = 0; i < array_length (globfile); ++i) remove (globfile[i]); if (cwd == NULL) @@ -414,26 +371,17 @@ main (int argc, char *argv[]) chdir (cwd); rmdir (tmpdir); - printf ("tests failed: %d\n", fail); - - return fail != 0; + return 0; } static const char * at_page_end (const char *words) { const int pagesize = getpagesize (); - char *start = mmap (0, 2 * pagesize, PROT_READ|PROT_WRITE, - MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); + char *start = xmmap (0, 2 * pagesize, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1); - if (start == MAP_FAILED) - return start; - - if (mprotect (start + pagesize, pagesize, PROT_NONE)) - { - munmap (start, 2 * pagesize); - return MAP_FAILED; - } + xmprotect (start + pagesize, pagesize, PROT_NONE); /* Includes terminating NUL. */ const size_t words_size = strlen (words) + 1; @@ -472,9 +420,6 @@ testit (struct test_case_struct *tc) fflush (NULL); const char *words = at_page_end (tc->words); - if (tc->flags & WRDE_NOCMD) - registered_forks = 0; - if (tc->flags & WRDE_APPEND) { /* initial wordexp() call, to be appended to */ @@ -486,13 +431,6 @@ testit (struct test_case_struct *tc) } retval = wordexp (words, &we, tc->flags); - if ((tc->flags & WRDE_NOCMD) - && (registered_forks > 0)) - { - printf ("FAILED fork called for WRDE_NOCMD\n"); - return 1; - } - if (tc->flags & WRDE_DOOFFS) start_offs = sav_we.we_offs; @@ -551,9 +489,11 @@ testit (struct test_case_struct *tc) const int page_size = getpagesize (); char *start = (char *) PTR_ALIGN_DOWN (words, page_size); - if (munmap (start, 2 * page_size) != 0) - return 1; + xmunmap (start, 2 * page_size); fflush (NULL); return bzzzt; } + +#define TEST_FUNCTION_ARGV do_test +#include <support/test-driver.c> diff --git a/posix/wordexp.c b/posix/wordexp.c index 22c6d18a9c..e1aafcaceb 100644 --- a/posix/wordexp.c +++ b/posix/wordexp.c @@ -25,33 +25,18 @@ #include <libintl.h> #include <paths.h> #include <pwd.h> -#include <signal.h> #include <stdbool.h> #include <stdio.h> -#include <stdlib.h> #include <string.h> #include <sys/param.h> -#include <sys/stat.h> -#include <sys/time.h> -#include <sys/types.h> -#include <sys/types.h> #include <sys/wait.h> #include <unistd.h> -#include <wchar.h> #include <wordexp.h> -#include <kernel-features.h> +#include <spawn.h> #include <scratch_buffer.h> - -#include <libc-lock.h> #include <_itoa.h> - -/* Undefine the following line for the production version. */ -/* #define NDEBUG 1 */ #include <assert.h> -/* Get some device information. */ -#include <device-nrs.h> - /* * This is a recursive-descent-style word expansion routine. */ @@ -812,61 +797,90 @@ parse_arith (char **word, size_t *word_length, size_t *max_length, return WRDE_SYNTAX; } +static char ** +create_environment (void) +{ + size_t s = 0; + + /* Calculate total environment size, including 'IFS' if is present. */ + for (char **ep = __environ; *ep != NULL; ep++, s++); + + /* Include final NULL pointer. */ + char **newenviron = malloc (s * sizeof (char*)); + if (newenviron == NULL) + return NULL; + + /* Copy current environment excluding 'IFS', to make sure the subshell + doesn't field-split on our behalf. */ + size_t i, j; + for (i = 0, j = 0; i < s; i++) + if (strncmp (__environ[i], "IFS=", sizeof ("IFS=")-1) != 0) + newenviron[j++] = __strdup (__environ[i]); + newenviron[j] = NULL; + + return newenviron; +} + +static void +free_environment (char **environ) +{ + for (char **ep = environ; *ep != NULL; ep++) + free (*ep); + free (environ); +} + /* Function called by child process in exec_comm() */ -static inline void -__attribute__ ((always_inline)) -exec_comm_child (char *comm, int *fildes, int showerr, int noexec) +static pid_t +exec_comm_child (char *comm, int *fildes, bool showerr, bool noexec) { - const char *args[4] = { _PATH_BSHELL, "-c", comm, NULL }; + pid_t pid = -1; - /* Execute the command, or just check syntax? */ - if (noexec) - args[1] = "-nc"; + /* Execute the command, or just check syntax? */ + const char *args[] = { _PATH_BSHELL, noexec ? "-nc" : "-c", comm, NULL }; - /* Redirect output. */ - if (__glibc_likely (fildes[1] != STDOUT_FILENO)) - { - __dup2 (fildes[1], STDOUT_FILENO); - __close (fildes[1]); - } - else - /* Reset the close-on-exec flag (if necessary). */ - __fcntl (fildes[1], F_SETFD, 0); + posix_spawn_file_actions_t fa; + /* posix_spawn_file_actions_init does not fail. */ + __posix_spawn_file_actions_init (&fa); - /* Redirect stderr to /dev/null if we have to. */ - if (showerr == 0) + /* Redirect output. For check syntax only (noexec being true), exec_comm + explicits sets fildes[1] to -1, so check its value to avoid a failure in + __posix_spawn_file_actions_adddup2. */ + if (fildes[1] != -1) { - struct stat64 st; - int fd; - __close (STDERR_FILENO); - fd = __open (_PATH_DEVNULL, O_WRONLY); - if (fd >= 0 && fd != STDERR_FILENO) + if (__glibc_likely (fildes[1] != STDOUT_FILENO)) { - __dup2 (fd, STDERR_FILENO); - __close (fd); + if (__posix_spawn_file_actions_adddup2 (&fa, fildes[1], + STDOUT_FILENO) != 0 + || __posix_spawn_file_actions_addclose (&fa, fildes[1]) != 0) + goto out; } - /* Be paranoid. Check that we actually opened the /dev/null - device. */ - if (__builtin_expect (__fxstat64 (_STAT_VER, STDERR_FILENO, &st), 0) != 0 - || __builtin_expect (S_ISCHR (st.st_mode), 1) == 0 -#if defined DEV_NULL_MAJOR && defined DEV_NULL_MINOR - || st.st_rdev != __gnu_dev_makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR) -#endif - ) - /* It's not the /dev/null device. Stop right here. The - problem is: how do we stop? We use _exit() with an - hopefully unusual exit code. */ - _exit (90); + else + /* Reset the close-on-exec flag (if necessary). */ + if (__posix_spawn_file_actions_adddup2 (&fa, fildes[1], fildes[1]) + != 0) + goto out; } - /* Make sure the subshell doesn't field-split on our behalf. */ - __unsetenv ("IFS"); + /* Redirect stderr to /dev/null if we have to. */ + if (!showerr) + if (__posix_spawn_file_actions_addopen (&fa, STDERR_FILENO, _PATH_DEVNULL, + O_WRONLY, 0) != 0) + goto out; + + char **newenv = create_environment (); + if (newenv == NULL) + goto out; - __close (fildes[0]); - __execve (_PATH_BSHELL, (char *const *) args, __environ); + /* pid is unset if posix_spawn fails, so it keep the original value + of -1. */ + __posix_spawn (&pid, _PATH_BSHELL, &fa, NULL, (char *const *) args, newenv); - /* Bad. What now? */ - abort (); + free_environment (newenv); + +out: + __posix_spawn_file_actions_destroy (&fa); + + return pid; } /* Function to execute a command and retrieve the results */ @@ -884,13 +898,13 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, size_t maxnewlines = 0; char buffer[bufsize]; pid_t pid; - int noexec = 0; + bool noexec = false; /* Do nothing if command substitution should not succeed. */ if (flags & WRDE_NOCMD) return WRDE_CMDSUB; - /* Don't fork() unless necessary */ + /* Don't posix_spawn() unless necessary */ if (!comm || !*comm) return 0; @@ -898,19 +912,15 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, return WRDE_NOSPACE; again: - if ((pid = __fork ()) < 0) + pid = exec_comm_child (comm, fildes, noexec ? false : flags & WRDE_SHOWERR, + noexec); + if (pid < 0) { - /* Bad */ __close (fildes[0]); __close (fildes[1]); return WRDE_NOSPACE; } - if (pid == 0) - exec_comm_child (comm, fildes, noexec ? 0 : flags & WRDE_SHOWERR, noexec); - - /* Parent */ - /* If we are just testing the syntax, only wait. */ if (noexec) return (TEMP_FAILURE_RETRY (__waitpid (pid, &status, 0)) == pid @@ -1091,7 +1101,7 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, /* Check for syntax error (re-execute but with "-n" flag) */ if (buflen < 1 && status != 0) { - noexec = 1; + noexec = true; goto again; } @@ -1143,26 +1153,9 @@ parse_comm (char **word, size_t *word_length, size_t *max_length, /* Go -- give script to the shell */ if (comm) { -#ifdef __libc_ptf_call - /* We do not want the exec_comm call to be cut short - by a thread cancellation since cleanup is very - ugly. Therefore disable cancellation for - now. */ - // XXX Ideally we do want the thread being cancelable. - // XXX If demand is there we'll change it. - int state = PTHREAD_CANCEL_ENABLE; - __libc_ptf_call (__pthread_setcancelstate, - (PTHREAD_CANCEL_DISABLE, &state), 0); -#endif - + /* posix_spawn already handles thread cancellation. */ error = exec_comm (comm, word, word_length, max_length, flags, pwordexp, ifs, ifs_white); - -#ifdef __libc_ptf_call - __libc_ptf_call (__pthread_setcancelstate, - (state, NULL), 0); -#endif - free (comm); }