Re: [PATCH] lib: Avoid double free when passing FDs with virCommandPassFD()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Apr 30, 2019 at 11:47:51AM +0200, Michal Privoznik wrote:
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@xxxxxxxxxx>
---
src/qemu/qemu_command.c | 10 ++++++----
src/util/virpolkit.c    |  1 +
tests/commandtest.c     |  2 +-
3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bf1fb539b1..92bd1524db 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8978,17 +8978,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;

This won't play nicely with the cleanup section, where we stop the
cleanup on the first -1 encountered:
   for (i = 0; vhostfd && i < vhostfdSize && vhostfd[i] >= 0; i++) {
   for (i = 0; tapfd && i < tapfdSize && tapfd[i] >= 0; i++) {

But it seems it's just a micro-optimization.

With the >= 0 conditions removed:
Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

    }

    if (chardev)

Attachment: signature.asc
Description: PGP signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux