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

[Mateusz Guzik]

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.

-- 
Mateusz Guzik