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

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.

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