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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
