From 5cdd5d380bab5f6f7207e0ffd0c1322a8281feca Mon Sep 17 00:00:00 2001
From: Michal Privoznik <mprivozn@redhat.com>
Date: Tue, 30 Apr 2019 11:17:22 +0200
Subject: [PATCH] lib: Avoid double close when passing FDs with
 virCommandPassFD()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If an FD is passed into a child using:

  virCommandPassFD(cmd, fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);

then the parent should refrain from touching @fd thereafter. This
is even documented in virCommandPassFD() comment. The reason is
that either at virCommandRun()/virCommandRunAsync() or
virCommandFree() time the @fd will be closed. Closing it earlier,
e.g. right after virCommandPassFD() call might result in
undesired results. Another thread might open a file and receive
the same FD which is then unexpectedly closed by virCommandFree()
or virCommandRun().

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
---
 src/qemu/qemu_command.c | 14 ++++++++------
 src/util/virpolkit.c    |  1 +
 tests/commandtest.c     |  2 +-
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9183163984..aae2f43044 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8997,17 +8997,19 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
         if (qemuSecuritySetTapFDLabel(driver->securityManager,
                                       def, tapfd[i]) < 0)
             goto cleanup;
-        virCommandPassFD(cmd, tapfd[i],
-                         VIR_COMMAND_PASS_FD_CLOSE_PARENT);
         if (virAsprintf(&tapfdName[i], "%d", tapfd[i]) < 0)
             goto cleanup;
+        virCommandPassFD(cmd, tapfd[i],
+                         VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+        tapfd[i] = -1;
     }
 
     for (i = 0; i < vhostfdSize; i++) {
-        virCommandPassFD(cmd, vhostfd[i],
-                         VIR_COMMAND_PASS_FD_CLOSE_PARENT);
         if (virAsprintf(&vhostfdName[i], "%d", vhostfd[i]) < 0)
             goto cleanup;
+        virCommandPassFD(cmd, vhostfd[i],
+                         VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+        vhostfd[i] = -1;
     }
 
     if (chardev)
@@ -9054,14 +9056,14 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
         virSetError(saved_err);
         virFreeError(saved_err);
     }
-    for (i = 0; vhostfd && i < vhostfdSize && vhostfd[i] >= 0; i++) {
+    for (i = 0; vhostfd && i < vhostfdSize; i++) {
         if (ret < 0)
             VIR_FORCE_CLOSE(vhostfd[i]);
         if (vhostfdName)
             VIR_FREE(vhostfdName[i]);
     }
     VIR_FREE(vhostfdName);
-    for (i = 0; tapfd && i < tapfdSize && tapfd[i] >= 0; i++) {
+    for (i = 0; tapfd && i < tapfdSize; i++) {
         if (ret < 0)
             VIR_FORCE_CLOSE(tapfd[i]);
         if (tapfdName)
diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
index 25eaad2c63..634b46e82b 100644
--- a/src/util/virpolkit.c
+++ b/src/util/virpolkit.c
@@ -187,6 +187,7 @@ virPolkitAgentCreate(void)
     virCommandSetOutputFD(agent->cmd, &outfd);
     virCommandSetErrorFD(agent->cmd, &errfd);
     virCommandPassFD(agent->cmd, pipe_fd[1], VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+    pipe_fd[1] = -1;
     if (virCommandRunAsync(agent->cmd, NULL) < 0)
         goto error;
 
diff --git a/tests/commandtest.c b/tests/commandtest.c
index 816a70860f..146cc4c1bf 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -1024,6 +1024,7 @@ static int test24(const void *unused ATTRIBUTE_UNUSED)
     virCommandDaemonize(cmd);
     virCommandPassFD(cmd, newfd2, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
     virCommandPassFD(cmd, newfd3, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+    newfd2 = newfd3 = -1;
     virCommandPassListenFDs(cmd);
 
     if (virCommandRun(cmd, NULL) < 0) {
@@ -1053,7 +1054,6 @@ static int test24(const void *unused ATTRIBUTE_UNUSED)
     VIR_FREE(prefix);
     virCommandFree(cmd);
     VIR_FORCE_CLOSE(newfd1);
-    /* coverity[double_close] */
     VIR_FORCE_CLOSE(newfd2);
     VIR_FORCE_CLOSE(newfd3);
     return ret;