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

[Eric Blake]

According to Laine Stump on 2/23/2010 12:54 PM:

Minor nits (I have not validated the overall patch in context; this is
just from a quick high-level review)

> 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

s/. the parnet/.  The parent/

> libvirtd process will use the other end of the pipe as its fd, then
> reap the child process after its done reading.

s/its/it's/

> 
> This makes domain restore work on a root-squash NFS share that is only
> visible to the qemu user.
> ---
>  src/qemu/qemu_driver.c |  158 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 155 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ac6ef6a..f909a59 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4659,6 +4659,135 @@ cleanup:
>      return ret;
>  }
>  
> +/* qemudOpenAsUID() - pipe/fork/setuid/open a file, and return the
> +   pipe fd to caller, so that it can read from the file. Also return
> +   the pid of the child process, so the caller can wait for it to exit
> +   after it's finished reading (to avoid a zombie, if nothing
> +   else). */
> +
> +static int qemudOpenAsUID(const char *path, uid_t uid, pid_t *childpid) {
> +    int pipefd[2] = {-1, -1};

Why the explicit initialization...

> +    int fd = -1;
> +
> +    *childpid = -1;
> +
> +    if (pipe(pipefd) < 0) {

...since I see nothing in POSIX that guarantees that pipe() will preserve
your initialized values across failure.  I think it's better to explicitly
set things to -1 on the failure path, rather than initializing that way.

> +        virReportSystemError(errno,
> +                             _("failed to create pipe to read '%s'"),
> +                             path);
> +        goto parentcleanup;
> +    }
> +
> +    int forkRet = virFork(childpid);

Unrelated to your patch, but we should probably start using pipe2 and
atomically marking pipes as CLOEXEC, so that use of libvirt in a
multithreaded environment does not have a race window where unrelated
threads can leak libvirt's fds across an exec (well, for new enough
kernels anyway; at least gnulib has a pipe2 fallback for older kernels).

...
> +    /* 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;
> +    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);

Is that the right uid to be reporting in the error message?

Nothing else jumped out at me as suspicious.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 320 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100223/b1923ac9/attachment-0001.sig>