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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
