[libvirt] [PATCH] qemu: fix potential memory leak

[Daniel Henrique Barboza]

On 9/19/19 5:00 AM, Xu Yandong wrote:
> function virTypedParamsAddString may return -1 but alloc params,
> so invoker should free it.
>
> Signed-off-by: Xu Yandong <xuyandong2 at huawei.com>
> ---
>   src/qemu/qemu_driver.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1e041a8bac..62ce7270ca 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5312,6 +5312,8 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
>           goto cleanup;
>   
>       event = virDomainEventTunableNewFromObj(vm, eventParams, eventNparams);
> +    eventParams = NULL;
> +    eventNparams = 0;

If I understood it right, you're doing these assignments to be able to
tell in the cleanup label if 'event' was populated, otherwise you can't
free eventParams.

You don't need to do that. You can simply test for event not being NULL
in the cleanup:

if (!event)
     virTypedParamsFree(eventParams, eventNparams);


This works because if event = NULL you either didn't reach the
virDomainEventTunableNewFromObj at all (which means that eventParams
should be freed if alloc'ed) or virDomainEvenTunableNewFromObj
returned NULL. If the later, the function will call virTypedParamsFree()
inside the 'error' label of the inner function virDomainEventTunableNew()
right before returning NULL, so no need to free eventParams again in
the cleanup too.


Same thing with the rest of the patch, given that
virDomainEventTunableNewFromDom behaves the same way if event = NULL.



Thanks,


DHB


>   
>       ret = 0;
>   
> @@ -5320,6 +5322,8 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
>       virCgroupFree(&cgroup_vcpu);
>       VIR_FREE(str);
>       virObjectEventStateQueue(driver->domainEventState, event);
> +    if (eventParams)
> +        virTypedParamsFree(eventParams, eventNparams);
>       return ret;
>   }
>   
> @@ -5527,6 +5531,8 @@ qemuDomainPinEmulator(virDomainPtr dom,
>               goto endjob;
>   
>           event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams);
> +        eventParams = NULL;
> +        eventNparams = 0;
>       }
>   
>       if (persistentDef) {
> @@ -5548,6 +5554,8 @@ qemuDomainPinEmulator(virDomainPtr dom,
>    cleanup:
>       if (cgroup_emulator)
>           virCgroupFree(&cgroup_emulator);
> +    if (eventParams)
> +        virTypedParamsFree(eventParams, eventNparams);
>       virObjectEventStateQueue(driver->domainEventState, event);
>       VIR_FREE(str);
>       virBitmapFree(pcpumap);
> @@ -6012,6 +6020,8 @@ qemuDomainPinIOThread(virDomainPtr dom,
>               goto endjob;
>   
>           event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams);
> +        eventParams = NULL;
> +        eventNparams = 0;
>       }
>   
>       if (persistentDef) {
> @@ -6043,6 +6053,8 @@ qemuDomainPinIOThread(virDomainPtr dom,
>    cleanup:
>       if (cgroup_iothread)
>           virCgroupFree(&cgroup_iothread);
> +    if (eventParams)
> +        virTypedParamsFree(eventParams, eventNparams);
>       virObjectEventStateQueue(driver->domainEventState, event);
>       VIR_FREE(str);
>       virBitmapFree(pcpumap);