Re: [libvirt] [PATCH v2 1/2] virfile: Adjust error path for virFileOpenForked

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(a)redhat.com&gt; --- 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.

- /* 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; }