[libvirt PATCH] qemu: fix potential resource leak

[Laine Stump]

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 at 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 at 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;
>   }
>