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

[Ján Tomko]

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 at redhat.com>
>---
> 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 at redhat.com>

Jano

>     }
>
>     if (chardev)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190517/df0ad226/attachment-0001.sig>