From patchwork Tue Oct 8 17:41:04 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 34882 Received: (qmail 111250 invoked by alias); 8 Oct 2019 17:41:12 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 111236 invoked by uid 89); 8 Oct 2019 17:41:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.2 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=pickup, pick-up X-HELO: mail-qt1-f195.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:openpgp:autocrypt:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=PUpjdhIV9gixJvB8a9cGu6FoXkzZnwH8/3Qv1WCGs+o=; b=wjIgIFTEfI0NvrLrNNaVgiGFUVG4NcPLEB5bRvGA8Jf+D2cCIfww+7JRtaBCugZ0ae 9q5ystc6hPDt6L/Kkn2LmhDZt/6uRbiYD/OTVNN9rx9EDZzQ1aQjk+qzCWrBXL7WQWAI gOLTDx/32iJHXfVVpN0qtMU+IKkevlQ2aHGNUajzjMZ/djJAtPd7nIKrHi+7S+4HlJP+ dOiVXY334ukRmNrCHwWVy5GUG//8VCNbSGSWK4MBXZgnPLvEkX0YZXRTjQOA2eKZm4g3 yY8YhG9xCHw/iFhlvkCZfqIzUwZpRGy89aSgwvP9/JUEsyhjlfQa0eHVbfaPoRWTCNao t//A== Return-Path: Subject: Re: [PATCH v2 5/5] posix: Use posix_spawn for wordexp To: Florian Weimer , Carlos O'Donell Cc: libc-alpha@sourceware.org References: <20190731183136.21545-1-adhemerval.zanella@linaro.org> <20190731183136.21545-5-adhemerval.zanella@linaro.org> <87d0f8e4fu.fsf@oldenburg2.str.redhat.com> From: Adhemerval Zanella Openpgp: preference=signencrypt Message-ID: <3cd9e4da-d5e2-df89-81ea-54835b449661@linaro.org> Date: Tue, 8 Oct 2019 14:41:04 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <87d0f8e4fu.fsf@oldenburg2.str.redhat.com> 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 . */ -#include -#include -#include +#include +#include #include -#include #include -#include -#include #include #include -#include +#include + #include -#include +#include +#include +#include #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 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 #include #include -#include #include #include -#include #include #include -#include -#include -#include -#include #include #include -#include #include -#include +#include #include - -#include #include <_itoa.h> - -/* Undefine the following line for the production version. */ -/* #define NDEBUG 1 */ #include -/* Get some device information. */ -#include - /* * 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 + /* 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); }