Re: [Libguestfs] [PATCH] daemon: improve internal commandrvf

- 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.

--- 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

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

} 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 ("/"));

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