diff mbox series

[v2,5/5] posix: Use posix_spawn for wordexp

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

Commit Message

Adhemerval Zanella Netto July 31, 2019, 6:31 p.m. UTC
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(-)

-- 
2.17.1

Comments

Adhemerval Zanella Netto Aug. 28, 2019, 2:09 p.m. UTC | #1
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 Netto Oct. 7, 2019, 5:51 p.m. UTC | #2
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);

>>  		}

>>  

>>
Florian Weimer Oct. 7, 2019, 7:33 p.m. UTC | #3
* 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
Carlos O'Donell Oct. 7, 2019, 9:04 p.m. UTC | #4
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.
Florian Weimer Oct. 8, 2019, 9:58 a.m. UTC | #5
* 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
Adhemerval Zanella Netto Oct. 8, 2019, 5:41 p.m. UTC | #6
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);
 		}
Florian Weimer Oct. 9, 2019, 9:11 a.m. UTC | #7
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
Adhemerval Zanella Netto Oct. 9, 2019, 12:18 p.m. UTC | #8
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 mbox series

Patch

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);
 		}