On Thu, Nov 19, 2015 at 05:38:25PM +0100, Pino Toscano wrote: > When running commands in the mounted guest (using the "command" API, and > APIs based on it), provide the /dev/null from the appliance as open fd > for stdin. Commands usually assume stdin is open if they didn't close > it explicitly, so this should avoid crashes or misbehavings due to that. > --- > daemon/command.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/daemon/command.c b/daemon/command.c > index 1593de9..b2d84ca 100644 > --- a/daemon/command.c > +++ b/daemon/command.c > @@ -23,6 +23,8 @@ > #include <stdbool.h> > #include <string.h> > #include <sys/stat.h> > +#include <sys/types.h> > +#include <fcntl.h> > > #include "guestfs_protocol.h" > #include "daemon.h" > @@ -242,7 +244,7 @@ do_command (char *const *argv) > { > char *out; > CLEANUP_FREE char *err = NULL; > - int r; > + int r, fd, flags; > CLEANUP_BIND_STATE struct bind_state bind_state = { .mounted = false }; > CLEANUP_RESOLVER_STATE struct resolver_state resolver_state = > { .mounted = false }; > @@ -259,6 +261,17 @@ 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). > + */ > + fd = open ("/dev/null", O_RDONLY|O_CLOEXEC); > + if (fd == -1) { > + reply_with_perror ("/dev/null"); > + return NULL; > + } > + > if (bind_mount (&bind_state) == -1) > return NULL; > if (enable_network) { > @@ -266,8 +279,10 @@ do_command (char *const *argv) > return NULL; > } > > + flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | fd; > + > CHROOT_IN; > - r = commandv (&out, &err, (const char * const *) argv); > + r = commandvf (&out, &err, flags, (const char * const *) argv); > CHROOT_OUT; > > free_bind_state (&bind_state); Looks good. How about naming the variable 'dev_null_fd' or something, so we know it's not just a temporary file descriptor variable?

On Thu, Nov 19, 2015 at 05:38:25PM +0100, Pino Toscano wrote: > When running commands in the mounted guest (using the "command" API, and > APIs based on it), provide the /dev/null from the appliance as open fd > for stdin. Commands usually assume stdin is open if they didn't close > it explicitly, so this should avoid crashes or misbehavings due to that. This does not look right. > + /* 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). > + */ > + fd = open ("/dev/null", O_RDONLY|O_CLOEXEC); > + if (fd == -1) { > + reply_with_perror ("/dev/null"); > + return NULL; > + } > + I disagree with this (see below). > if (bind_mount (&bind_state) == -1) > return NULL; nit: this leaks the fd on error, but it may not matter much. > if (enable_network) { > @@ -266,8 +279,10 @@ do_command (char *const *argv) > return NULL; > } > nit: same.

> + flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | fd; > + > CHROOT_IN; > - r = commandv (&out, &err, (const char * const *) argv); > + r = commandvf (&out, &err, flags, (const char * const *) argv); > CHROOT_OUT; > > free_bind_state (&bind_state); According to the analysis in https://bugzilla.redhat.com/show_bug.cgi?id=1280029 the problem was that the target program was being executed in a chroot which did not have /dev/null populated, hence the open in commandrvf failed and the process was left without fd 0. commandrvf does the following in the child: close (0); if (flag_copy_stdin) { dup2 (flag_copy_fd, STDIN_FILENO); } else { /* Set stdin to /dev/null (ignore failure) */ ignore_value (open ("/dev/null", O_RDONLY|O_CLOEXEC)); } [..] execvp (argv[0], (void *) argv); First observation is that regardless of whether this open("/dev/null", ..) succeeds, there is no fd 0 in the process after execvp due to O_CLOEXEC.

So, chances are the chroot did have /dev/null, but the fd simply got closed.

I would argue that /dev has to be at least partially populated for anything that gets executed in the chroot. At the very least special nodes like null, zero and {u,}random are needed.

CHROOT_IN/OUT around commandvf are definitely problematic. chroot should be done in the child, which also removes the need to chroot out in the parent.

Assuming populated /dev is problematic/not feasible, at the very least the open(/dev/null) should be performed in the child just prior to chroot.

Current patch seems to work around shortcomings of the current API. Side content: if (flag_copy_stdin) close (flag_copy_fd); waitpid (pid, NULL, 0); return -1;

but some lines below there is: if (flag_copy_stdin && close (flag_copy_fd) == -1) { perror ("close"); return -1; } /* Get the exit status of the command. */ if (waitpid (pid, &r, 0) != pid) { perror ("waitpid"); return -1; } close() does not return an error unless extraterrestial circumstances occur, and even then the fd is no longer in use by the process. As such, I would argue checking for errors here is not necessary. Note that prior sample does not check.

In case an error was returned, the code fails to waitpid() for the child.