Re: [libvirt PATCH] qemu: fix potential resource leak
On 10/21/20 5:50 PM, Jonathon Jongsma wrote:
Coverity reported a potential resource leak. While it's probably
not
a real-world scenario, the code could technically jump to cleanup
between the time that vdpafd is opened and when it is used. Ensure that
it gets cleaned up in that case.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/qemu/qemu_command.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 5c4e37bd9e..cbe7a6e331 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8135,6 +8135,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
addfdarg = g_strdup_printf("%s,opaque=%s", fdset,
net->data.vdpa.devicepath);
virCommandAddArgList(cmd, "-add-fd", addfdarg, NULL);
+ vdpafd = -1;
}
if (chardev)
@@ -8204,6 +8205,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
VIR_FREE(tapfdName);
VIR_FREE(vhostfd);
VIR_FREE(tapfd);
+ if (vdpafd >= 0)
+ VIR_FORCE_CLOSE(vdpafd);
VIR_FORCE_CLOSE() ==>virForceCloseHelper() ==> virFileClose()
and virFileClose() is a NOP if fd < 0, so this doesn't need the conditional.
I *was* going to say "With that change,
Reviewed-by: Laine Stump <laine(a)redhat.com>
"
but this got me looking at the code of the entire function rather than
just the diffs in the patch, and I've got a question - is there any
reason that you only ope n the vdpa device inside the switch, and save
everything else related to it until later (at the "if (vdpafd)")? You
could then device vpafd only within that case of the switch, and make it
VIR_AUTOCLOSE vpafd = -1; Then just set it back to -1 immediately after
calling virCommandPassFD (because once it is in the set of fd's being
passed to qemu, it will be closed by virCommand* functions in the
libvirtd process, whether qemu is successfully run or not).
Does that make sense?
return ret;
}