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.
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