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

[Pino Toscano]

On Friday 20 November 2015 11:58:25 Richard W.M. Jones wrote:
> 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?

Makes sense, pushed with the suggested correction.

Thanks,
-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20151120/48b8e328/attachment.sig>