Re: [PATCH v2 5/5] posix: Use posix_spawn for wordexp

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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