[Libguestfs] [PATCH] daemon: improve internal commandrvf

[Mateusz Guzik]

On Wed, Dec 02, 2015 at 02:00:57PM +0100, Pino Toscano wrote:
> - add a flag to request chroot for the process, which is done only as
>   very last (before chdir) operation before exec'ing the process in the
>   child: this avoids using CHROOT_IN & CHROOT_OUT around command*
>   invocations, and reduces the code spent in chroot mode
> - add failure checks for dup2 and open done in child, not proceeding to
>   executing the process if they fail
> - open /dev/null without O_CLOEXEC, so it stays available for the
>   exec'ed process, and thus we don't need to provide an own fd for stdin
> 
> Followup of commit fd2f175ee79d29df101d353e2f380db27b19553a, thanks also
> to the notes and hints provided by Mateusz Guzik.

Looks good, thanks. I only have optional nits.

> ---
>  daemon/command.c  | 17 ++---------------
>  daemon/daemon.h   |  1 +
>  daemon/guestfsd.c | 39 +++++++++++++++++++++++++++++++--------
>  3 files changed, 34 insertions(+), 23 deletions(-)
> 
> diff --git a/daemon/command.c b/daemon/command.c
> index 27a4d0c..c4efa5b 100644
> --- a/daemon/command.c
> +++ b/daemon/command.c
> @@ -244,7 +244,7 @@ do_command (char *const *argv)
>  {
>    char *out;
>    CLEANUP_FREE char *err = NULL;
> -  int r, dev_null_fd, flags;
> +  int r, flags;
>    CLEANUP_BIND_STATE struct bind_state bind_state = { .mounted = false };
>    CLEANUP_RESOLVER_STATE struct resolver_state resolver_state =
>      { .mounted = false };
> @@ -261,17 +261,6 @@ do_command (char *const *argv)
>      return NULL;
>    }
>  
> -  /* Provide /dev/null as stdin for the command, since we want
> -   * to make sure processes have an open stdin, and it is not
> -   * possible to rely on the guest to provide it (Linux guests
> -   * get /dev dynamically populated at runtime by udev).
> -   */
> -  dev_null_fd = open ("/dev/null", O_RDONLY|O_CLOEXEC);
> -  if (dev_null_fd == -1) {
> -    reply_with_perror ("/dev/null");
> -    return NULL;
> -  }
> -
>    if (bind_mount (&bind_state) == -1)
>      return NULL;
>    if (enable_network) {
> @@ -279,11 +268,9 @@ do_command (char *const *argv)
>        return NULL;
>    }
>  
> -  flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | dev_null_fd;
> +  flags = COMMAND_FLAG_DO_CHROOT;
>  
> -  CHROOT_IN;
>    r = commandvf (&out, &err, flags, (const char * const *) argv);
> -  CHROOT_OUT;
>  
>    free_bind_state (&bind_state);
>    free_resolver_state (&resolver_state);
> diff --git a/daemon/daemon.h b/daemon/daemon.h
> index 7fbb2a2..af6f68c 100644
> --- a/daemon/daemon.h
> +++ b/daemon/daemon.h
> @@ -128,6 +128,7 @@ extern char **empty_list (void);
>  #define COMMAND_FLAG_FD_MASK                   (1024-1)
>  #define COMMAND_FLAG_FOLD_STDOUT_ON_STDERR     1024
>  #define COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN 2048
> +#define COMMAND_FLAG_DO_CHROOT                 4096
>  

Any eason for having this decimal? The standard thing is to use hex.

>  extern int commandf (char **stdoutput, char **stderror, int flags,
>                       const char *name, ...) __attribute__((sentinel));
> diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
> index 0a29aa6..47245f7 100644
> --- a/daemon/guestfsd.c
> +++ b/daemon/guestfsd.c
> @@ -932,21 +932,44 @@ commandrvf (char **stdoutput, char **stderror, int flags,
>      signal (SIGPIPE, SIG_DFL);
>      close (0);
>      if (flag_copy_stdin) {
> -      dup2 (flag_copy_fd, STDIN_FILENO);
> +      if (dup2 (flag_copy_fd, STDIN_FILENO) == -1) {
> +        perror ("dup2/flag_copy_fd");
> +        _exit (EXIT_FAILURE);
> +      }

close(0) explicitly assumes that stdin is 0, which is fine, but dup2
uses STDIN_FILENO (which itself is also fine), but you should stick to
one scheme.

>      } else {
> -      /* Set stdin to /dev/null (ignore failure) */
> -      ignore_value (open ("/dev/null", O_RDONLY|O_CLOEXEC));
> +      /* Set stdin to /dev/null. */
> +      if (open ("/dev/null", O_RDONLY) == -1) {
> +        perror ("open(/dev/null)");
> +        _exit (EXIT_FAILURE);
> +      }
>      }
>      close (so_fd[PIPE_READ]);
>      close (se_fd[PIPE_READ]);
> -    if (!(flags & COMMAND_FLAG_FOLD_STDOUT_ON_STDERR))
> -      dup2 (so_fd[PIPE_WRITE], STDOUT_FILENO);
> -    else
> -      dup2 (se_fd[PIPE_WRITE], STDOUT_FILENO);
> -    dup2 (se_fd[PIPE_WRITE], STDERR_FILENO);
> +    if (!(flags & COMMAND_FLAG_FOLD_STDOUT_ON_STDERR)) {
> +      if (dup2 (so_fd[PIPE_WRITE], STDOUT_FILENO) == -1) {
> +        perror ("dup2/so_fd[PIPE_WRITE]");
> +        _exit (EXIT_FAILURE);
> +      }
> +    } else {
> +      if (dup2 (se_fd[PIPE_WRITE], STDOUT_FILENO) == -1) {
> +        perror ("dup2/se_fd[PIPE_WRITE]");
> +        _exit (EXIT_FAILURE);
> +      }
> +    }
> +    if (dup2 (se_fd[PIPE_WRITE], STDERR_FILENO) == -1) {
> +      perror ("dup2/se_fd[PIPE_WRITE]");
> +      _exit (EXIT_FAILURE);
> +    }
>      close (so_fd[PIPE_WRITE]);
>      close (se_fd[PIPE_WRITE]);
>  
> +    if (flags & COMMAND_FLAG_DO_CHROOT && sysroot_len > 0) {
> +      if (chroot (sysroot) == -1) {
> +        perror ("chroot in sysroot");
> +        _exit (EXIT_FAILURE);
> +      }
> +    }
> +
>      ignore_value (chdir ("/"));
>  

This could also be error-checked.

>      execvp (argv[0], (void *) argv);
> -- 
> 2.1.0
> 

-- 
Mateusz Guzik