Re: [libvirt PATCH] qemu: fix potential resource leak

On Thu, 22 Oct 2020 09:01:13 +0200 Peter Krempa <pkrempa(a)redhat.com&gt; wrote: > On Wed, Oct 21, 2020 at 22:31:09 -0400, Laine Stump wrote: > > 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&gt; > > > --- > > > 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&gt; > > > > " > > > > > > 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 > > I'd like to point out that opening anything in the command line > formatters is IMO bad practice. The command line formatter should > format the commandline and nothing more. This is visible by the > necessity to have a lot of mock override functions which prevent the > command line formatter from touching resources on the host in the > first place. > > Better approach is to open resources in 'qemuProcessPrepareHost' and > store them in private data for command line formatting. Fake data for > tests are added in 'testCompareXMLToArgvCreateArgs'. > > I'm aware though that there's a lot of "prior art" in this area > though. These are good points. I fell into the trap of modeling the new code on some existing code that did similar things rather than thinking critically enough about the best way to do it. I'll look into a better approach.