src/qemu/qemu_command.c | 10 ++++++---- src/util/virpolkit.c | 1 + tests/commandtest.c | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-)
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>
---
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;
}
if (chardev)
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;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@redhat.com> Jano > } > > if (chardev) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.