On 30.01.2015 18:52, John Ferlan wrote:

Rather than have a dummy waitpid loop and return of the failure status from recvfd, adjust the logic to save the recvfd error & fd and then in priority order:

- if waitpid failed, use that errno value - waitpid succeeded, but if the child exited abnormally, report failure (use EACCES to report as return failure, since either EACCES or EPERM is what caused us to fall into the fork+setuid path) - waitpid succeeded, but if the child reported non-zero status, report failure (use the errno value that the child encoded into exit status) - waitpid succeeded, but if recvfd failed, report recvfd_errno - waitpid and recvfd succeeded, use the fd

NOTE: Original logic to retry the open and force owner mode was "documented" as only being attempted if we had already tried opening with the fork+setuid, but checked flags vs. VIR_FILE_OPEN_NOFORK which is counter to how we would get to that point. So that code was removed.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virfile.c | 65 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c index 4024f3d..e6e13bc 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2034,6 +2034,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, { pid_t pid; int waitret, status, ret = 0; + int recvfd_errno; int fd = -1; int pair[2] = { -1, -1 }; gid_t *groups; @@ -2124,15 +2125,11 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, fd = recvfd(pair[0], 0); } while (fd < 0 && errno == EINTR); VIR_FORCE_CLOSE(pair[0]); /* NB: this preserves errno */ + recvfd_errno = errno;

From recvfd() source, it seems like errno is set iff an error occurred. Upon success, errno is left untouched. Therefore we may set recvfd_errno to 'random' value. Therefore I think you should set recvfd_errno iff fd < 0. If you do this, you need to initialize recvfd_errno too. This may look like overkill, since there is no visible path, how an errno could be set, and the control could still reach this point. But rather be safe than sorry.

- /* gnulib will return ENOTCONN in certain instances */ - if (fd < 0 && !(errno == EACCES || errno == ENOTCONN)) { - ret = -errno; - while (waitpid(pid, NULL, 0) == -1 && errno == EINTR); - return ret; - } - - /* wait for child to complete, and retrieve its exit code */ + /* wait for child to complete, and retrieve its exit code + * if waitpid fails, use that status + */ while ((waitret = waitpid(pid, &status, 0)) == -1 && errno == EINTR); if (waitret == -1) { ret = -errno; @@ -2142,28 +2139,40 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, VIR_FORCE_CLOSE(fd); return ret; } - if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES || - fd == -1) { - /* fall back to the simpler method, which works better in - * some cases */ + + /* + * If waitpid succeeded, but if the child exited abnormally or + * reported non-zero status, report failure. + */ + if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) { + char *msg = virProcessTranslateStatus(status); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("child failed to create '%s': %s"), + path, msg); VIR_FORCE_CLOSE(fd); - if (flags & VIR_FILE_OPEN_NOFORK) { - /* If we had already tried opening w/o fork+setuid and - * failed, no sense trying again. Just set return the - * original errno that we got at that time (by - * definition, always either EACCES or EPERM - EACCES - * is close enough). - */ - return -EACCES; - } - if ((fd = open(path, openflags, mode)) < 0) - return -errno; - ret = virFileOpenForceOwnerMode(path, fd, mode, uid, gid, flags); - if (ret < 0) { - VIR_FORCE_CLOSE(fd); - return ret; - } + VIR_FREE(msg); + /* Use child exit status if possible; otherwise, + * just use -EACCES, since by our original failure in + * the non fork+setuid path would have been EACCES or + * EPERM by definition, but EACCES is close enough. + * Besides -EPERM is like returning fd == -1. + */ + if (WIFEXITED(status)) + ret = -WEXITSTATUS(status); + else + ret = -EACCES; + return ret; } + + /* if waitpid succeeded, but recvfd failed, report recvfd_errno */ + if (recvfd_errno != 0) { + virReportSystemError(recvfd_errno, + _("failed recvfd for child creating '%s'"), + path); + return -recvfd_errno; + } + + /* otherwise, waitpid and recvfd succeeded, return the fd */ return fd; }

ACK with that fixed. Michal

On 30.01.2015 18:52, John Ferlan wrote:

In the event we're falling into the code that tries to create the file in a forked environment (VIR_FILE_OPEN_FORK) we pass different mode bits, but those are never set because the virFileOpenForceOwnerMode has a check if the OPEN_FORCE_MODE bit is set before attempting to change the mode.

Since this is a special case it seems reasonable to set u+rw,g+rw,o

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 59a9593..892d804 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2965,6 +2965,9 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid,

/* Retry creating the file as qemu user */

+ /* Since we're passing different modes... */ + vfoflags |= VIR_FILE_OPEN_FORCE_MODE; + if ((fd = virFileOpenAs(path, oflags, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, fallback_uid, fallback_gid,

ACK Michal