Mateusz Guzik
2015-Dec-01 14:59 UTC
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
Mateusz Guzik
2015-Dec-01 16:05 UTC
Re: [Libguestfs] [PATCH] daemon: always provide stdin when running chroot commands (RHBZ#1280029)
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
Seemingly Similar Threads
- Re: [PATCH] daemon: always provide stdin when running chroot commands (RHBZ#1280029)
- [PATCH] daemon: always provide stdin when running chroot commands (RHBZ#1280029)
- [PATCH] daemon: improve internal commandrvf
- Re: [PATCH] daemon: always provide stdin when running chroot commands (RHBZ#1280029)
- Re: [PATCH] daemon: always provide stdin when running chroot commands (RHBZ#1280029)