[Libguestfs] [PATCH] daemon: always provide stdin when running chroot commands (RHBZ#1280029)
Pino Toscano
ptoscano at redhat.com
Tue Dec 1 15:16:57 UTC 2015
On Tuesday 01 December 2015 15:59:56 Mateusz Guzik 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.
>
> 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.
Both these leaks need to be fixed, thanks for reporting them.
> > + 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.
Correct, so O_CLOEXEC could be dropped in this case.
> So, chances are the chroot did have /dev/null, but the fd simply got closed.
No, the actual issue was that there was nothing at all in the /dev of
guest, so open failed.
> 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.
We do not assume anything about guests, which could be Linux with a
static /dev (rare these days), but also non-Linux guests, including
Windows.
> 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.
That could be another way to make the command* functions in the daemon
safer, which wouldn't solve the issue of this email thread: even if you
fork and then chroot, the guest has nothing in /dev, so you cannot
open /dev/null at that point. This means you still need to carry on an
open /dev/null from the appliance.
> 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.
See above.
> 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;
This is done only in the main "while (quit < 2)" loop, and only on
error there, which will return with -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.
The code for flag_copy_stdin & flag_copy_fd is unrelated to the bit
quoted above, see above for the explanation.
> In case an error was returned, the code fails to waitpid() for the child.
This needs to be fixed indeed.
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/20151201/0c65bd54/attachment.sig>
More information about the Libguestfs
mailing list