[libvirt] [PATCH] Fix domain restore for files on root-squash NFS.

[Jim Meyering]

Laine Stump wrote:
> If qemudDomainRestore fails to open the domain save file, create a
> pipe, then fork a process that does setuid(qemu_user) and opens the
> file, then reads this file and stuffs it into the pipe. the parnet
> libvirtd process will use the other end of the pipe as its fd, then
> reap the child process after its done reading.

Hi Laine,

This looks fine, modulo a few minor stylistic
and error-handling nits.  Comments below.

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
...
> +static int qemudOpenAsUID(const char *path, uid_t uid, pid_t *childpid) {
...
> +        /* caller gets the read side of the pipe */
> +        fd = pipefd[0];
> +        pipefd[0] = -1;
> +parentcleanup:

This_is_more_readable than notusingwordseparators ;-)
s/cleanup/_cleanup/

> +        if (pipefd[0] != -1)
> +            close(pipefd[0]);
> +        if (pipefd[1] != -1)
> +            close(pipefd[1]);
> +        if ((fd < 0) && (*childpid > 0)) {
> +            /* a child process was started and subsequently an error
> +               occurred in the parent, so we need to wait for it to
> +               exit, but its status is inconsequential. */
> +            while ((waitpid(*childpid, NULL, 0) == -1)
> +                   && (errno == EINTR));
> +            *childpid = -1;
> +        }
> +        return fd;
> +    }
> +
> +    /* child */
> +
> +    /* setuid to the qemu user, then open the file, read it,
> +       and stuff it into the pipe for the parent process to
> +       read */
> +    int exit_code;
> +    char *buf = NULL;
> +    int bufsize = 1024 * 1024;

It's slightly better to use size_t in place of int,
to match the prototype of VIR_ALLOC_N.

> +    int bytesread;
> +
> +    /* child doesn't need the read side of the pipe */
> +    close(pipefd[0]);
> +
> +    if (forkRet < 0) {
> +        exit_code = errno;
> +        virReportSystemError(errno,
> +                             _("failed in child after forking to read '%s'"),
> +                             path);
> +        goto childcleanup;
> +    }
> +
> +    if (setuid(uid) != 0) {
> +        exit_code = errno;
> +        virReportSystemError(errno,
> +                             _("cannot setuid(%d) to read '%s'"),
> +                             getuid(), path);
> +        goto childcleanup;
> +    }
> +    if ((fd = open(path, O_RDONLY)) < 0) {
> +        exit_code = errno;
> +        virReportSystemError(errno,
> +                             _("cannot open '%s' as uid %d"),
> +                             path, getuid());
> +        goto childcleanup;
> +    }
> +    if (VIR_ALLOC_N(buf, bufsize) < 0) {
> +        exit_code = ENOMEM;
> +        virReportOOMError();
> +        goto childcleanup;
> +    }
> +
> +    /* read from fd and write to pipefd[1] until EOF */
> +    do {
> +        if ((bytesread = saferead(fd, buf, bufsize)) < 0) {
> +            exit_code = errno;
> +            virReportSystemError(errno,
> +                                 _("child failed reading from '%s'"),
> +                                 path);
> +            goto childcleanup;
> +        }
> +        if (safewrite(pipefd[1], buf, bytesread) != bytesread) {
> +            exit_code = errno;
> +            virReportSystemError(errno, "%s",
> +                                 _("child failed writing to pipe"));
> +            goto childcleanup;
> +        }
> +    } while (bytesread > 0);
> +    exit_code = 0;
> +
> +childcleanup:

s/cleanup/_cleanup/

> +    VIR_FREE(buf);
> +    if (fd != -1)
> +        close(fd);
> +    if (pipefd[1] != -1)
> +        close(pipefd[1]);
> +    _exit(exit_code);
> +}
> +
>  /* TODO: check seclabel restore */
>  static int qemudDomainRestore(virConnectPtr conn,
>                                const char *path) {
> @@ -4666,6 +4795,7 @@ static int qemudDomainRestore(virConnectPtr conn,
>      virDomainDefPtr def = NULL;
>      virDomainObjPtr vm = NULL;
>      int fd = -1;
> +    pid_t read_pid = -1;
>      int ret = -1;
>      char *xml = NULL;
>      struct qemud_save_header header;
> @@ -4677,9 +4807,20 @@ static int qemudDomainRestore(virConnectPtr conn,
>      qemuDriverLock(driver);
>      /* Verify the header and read the XML */
>      if ((fd = open(path, O_RDONLY)) < 0) {
> -        qemuReportError(VIR_ERR_OPERATION_FAILED,
> -                        "%s", _("cannot read domain image"));
> -        goto cleanup;
> +        if ((driver->user == 0) || (getuid() != 0)) {
> +            qemuReportError(VIR_ERR_OPERATION_FAILED,
> +                            "%s", _("cannot read domain image"));
> +            goto cleanup;
> +        }
> +
> +        /* Opening as root failed, but qemu runs as a different user
> +           that might have better luck. Create a pipe, then fork a
> +           child process to run as the qemu user, which will hopefully
> +           have the necessary authority to read the file. */
> +        if ((fd = qemudOpenAsUID(path, driver->user, &read_pid)) < 0) {
> +            /* error already reported */
> +            goto cleanup;
> +        }
>      }
>
>      if (saferead(fd, &header, sizeof(header)) != sizeof(header)) {
> @@ -4769,6 +4910,12 @@ static int qemudDomainRestore(virConnectPtr conn,
>          close(intermediatefd);
>      close(fd);
>      fd = -1;
> +    if (read_pid != -1) {
> +        /* reap the process that read the file */
> +        while ((waitpid(read_pid, NULL, 0) == -1)
> +               && (errno == EINTR));

Two things.  In general, terminating an empty loop with a semicolon
is risky -- too easy to miss that syntax, and snuggling it up on the
end of the "while (cond)" line makes it even easier to miss.  If you
do this, please at least add a comment on a separate line:

    while (cond)
        /* empty */ ;

using braces might even be better:

    while (cond) {
        /* empty */
    }

You might want to diagnose a failing waitpid, when errno != EINTR.
ECHILD (no such PID) appears to be the only one we'd care about.
Obviously, we'd ignore waitpid failure during clean-up,
but in the parent, post-read, it might indicate a problem.

Also, don't you want to detect child failure in the latter?
If so, you'd pass say, &child_status as second argument to waitpid.

> +        read_pid = -1;
> +    }
>      if (ret < 0) {
>          if (!vm->persistent) {
>              if (qemuDomainObjEndJob(vm) > 0)
> @@ -4810,6 +4957,11 @@ cleanup:
>      VIR_FREE(xml);
>      if (fd != -1)
>          close(fd);
> +    if (read_pid != 0) {
> +        /* reap the process that read the file */
> +        while ((waitpid(read_pid, NULL, 0) == -1)
> +               && (errno == EINTR));
> +    }
>      if (vm)
>          virDomainObjUnlock(vm);
>      if (event)