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

[Mateusz Guzik]

On Tue, Dec 01, 2015 at 04:16:57PM +0100, Pino Toscano wrote:
> On Tuesday 01 December 2015 15:59:56 Mateusz Guzik wrote:
> > 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.
> 

I have to confess to ignorance about this project, I only found it after
seeing a suspicious commit in dnf.

However, your description does not look right. The code seems to assume
it is possible to execve binaries in a chroot, which for practical
purposes means linux binaries. Further, do_command sets up
/etc/resolv.conf for the "guest".

My argument is that executing linux binaries in an environment without
properly populated /dev is a recipe for trouble and will one day come
back with weird failures or improperly constructed images.

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

The issue would be solved in a less hackish manner. If the process is
not chrooted, it can open /dev/null. So you fork, open /dev/null and
only then proceed to chroot.

But as noted earlier, unpopulated /dev seems to be the real bug here.

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

The point was that cleanup is duplicated and inconsistent.

-- 
Mateusz Guzik