Re: [Libguestfs] [PATCH] daemon: always provide stdin when running chroot commands (RHBZ#1280029)

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?