[PATCH 4/7] virfile: Adjust error path for virFileOpenForked

[John Ferlan <jferlan@xxxxxxxxxx>]

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
if waitpid succeeded, but the child reported failure, use that
regardless of recvfd status
if waitpid succeeded and reported success, but recvfd failed with
anything other than EACCES or ENOTCONN, use recvfd's result
otherwise, waitpid and recvfd succeeded, return 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@xxxxxxxxxx>
---
 src/util/virfile.c | 67 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 39 insertions(+), 28 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 4024f3d..cf6819f 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;
 
-    /* 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,42 @@ 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 the child reported failure, use that
+     * regardless of recvfd status
+     */
+    if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
+        ret = WEXITSTATUS(status);
+        virReportSystemError(ret,
+                             _("child failed to create '%s'"),
+                             path);
         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;
-        }
+        return -ret;
+    }
+
+    /* if waitpid succeeded and reported success, but recvfd failed with
+     * anything other than EACCES or ENOTCONN, use recvfd's result
+     */
+    if (WIFEXITED(status) && WEXITSTATUS(status) == 0 && fd < 0 &&
+        !(recvfd_errno == EACCES || recvfd_errno == ENOTCONN)) {
+        virReportSystemError(recvfd_errno,
+                             _("failed recvfd for child creating '%s'"),
+                             path);
+        return -recvfd_errno;
+    }
+
+    /* Some sort of abnormal child termination */
+    if (!WIFEXITED(status) || fd == -1) {
+        VIR_FORCE_CLOSE(fd);
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("child exited abnormally, failed to create '%s'"),
+                       path);
+        return -EACCES;
     }
+
+    /* otherwise, waitpid and recvfd succeeded, return the fd
+     */
     return fd;
 }
 
-- 
2.1.0

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list