Thanks for the quick response! On 02/23/2010 03:32 PM, Eric Blake wrote:
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/
Ah, yes. Those terrible typos that never seem worth the effort to fix after they're already checked in. Thanks for nipping it in the bud!
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...
Mostly because I was following the example of util.c:__virExec(), which does the same thing. Following examples seems to get me into nearly as much trouble as ignoring them ;-)
+ 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.
Interesting point. If we can't rely on that, then __virExec() should be changed too. Of course this makes maintenance a bit more error prone, since we would have to make sure that *every* jump to the cleanup that happened prior to creating the pipe would set the pipe fds to -1 (or just leave the initialization in, and re-set them to -1 when pipe failed; that's probably the safest thing to do).
+ 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).
...
I think I just heard someone volunteering! ;-)
+ /* 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?
What's the proper emoticon for blushing in embarrassment? Bonehead typo ;-) Thanks for pointing it out.