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