Thanks for the review!
On 02/23/2010 04:57 PM, Jim Meyering wrote:
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/
Yeah, okay. I get conflicted working in libvirt, because I find bits
with both studlyCaps and underscore_separators, and can't decide which
to use, so my brain locks up and I use none.
+ 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.
Right. Done.
+ 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/
Yup.
+ 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 */
}
There I go, letting cut-paste get me into trouble again.
I also prefer using braces, and will change it to that.
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.
Yeah, I wondered about that, but wasn't sure what I could do after qemu
had already been started up and read data from the file (which
presumably would itself have failed if there was some problem in the
child that cause bad/incomplete data to show up). So I punted.
I'll spend some time in the morning trying to do something suitable if
the child sends back a failure status.
+ 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)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
