qemu: fix memory leak on some paths

[PATCH] qemu: fix memory leak on some paths

Posted by luzhipeng 1 year, 7 months ago

From: lu zhipeng <luzhipeng@cestc.cn>

virTypedParamsAddString may return -1 but alloc params,
so free it.

Signed-off-by: lu zhipeng <luzhipeng@cestc.cn>
---
 src/qemu/qemu_driver.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 94b70872d4..c4501cd705 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4473,11 +4473,13 @@ qemuDomainPinVcpuLive(virDomainObj *vm,
         goto cleanup;
 
     event = virDomainEventTunableNewFromObj(vm, eventParams, eventNparams);
-
+    eventNparams = 0;
     ret = 0;
 
  cleanup:
     virObjectEventStateQueue(driver->domainEventState, event);
+    if (eventNparams)
+        virTypedParamsFree(eventParams, eventNparams);
     return ret;
 }
 
@@ -4682,6 +4684,7 @@ qemuDomainPinEmulator(virDomainPtr dom,
             goto endjob;
 
         event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams);
+        eventNparams = 0;
     }
 
     if (persistentDef) {
@@ -4700,6 +4703,8 @@ qemuDomainPinEmulator(virDomainPtr dom,
  cleanup:
     virObjectEventStateQueue(driver->domainEventState, event);
     virDomainObjEndAPI(&vm);
+    if (eventNparams)
+        virTypedParamsFree(eventParams, eventNparams);
     return ret;
 }
 
@@ -5079,6 +5084,7 @@ qemuDomainPinIOThread(virDomainPtr dom,
             goto endjob;
 
         event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams);
+        eventNparams = 0;
     }
 
     if (persistentDef) {
@@ -5106,6 +5112,8 @@ qemuDomainPinIOThread(virDomainPtr dom,
  cleanup:
     virObjectEventStateQueue(driver->domainEventState, event);
     virDomainObjEndAPI(&vm);
+    if (eventNparams)
+        virTypedParamsFree(eventParams, eventNparams);
     return ret;
 }
 
-- 
2.31.1

Re: [PATCH] qemu: fix memory leak on some paths

Posted by Jiri Denemark 1 year, 7 months ago

On Thu, Sep 22, 2022 at 21:00:38 +0800, luzhipeng wrote:
> From: lu zhipeng <luzhipeng@cestc.cn>
> 
> virTypedParamsAddString may return -1 but alloc params,
> so free it.
> 
> Signed-off-by: lu zhipeng <luzhipeng@cestc.cn>
> ---
>  src/qemu/qemu_driver.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 94b70872d4..c4501cd705 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4473,11 +4473,13 @@ qemuDomainPinVcpuLive(virDomainObj *vm,
>          goto cleanup;
>  
>      event = virDomainEventTunableNewFromObj(vm, eventParams, eventNparams);

This call consumes eventParams which should not be used afterwards. Thus
instead of

> -
> +    eventNparams = 0;

we should set eventParams = NULL here...

>      ret = 0;
>  
>   cleanup:
>      virObjectEventStateQueue(driver->domainEventState, event);

... and call virTypedParamsFree unconditionally.

> +    if (eventNparams)
> +        virTypedParamsFree(eventParams, eventNparams);
>      return ret;
>  }
>  

Alternatively, virDomainEventTunableNew could be changed to get
virTypedParameterPtr *params and set &params = NULL itself.

Jirka

Re: [PATCH] qemu: fix memory leak on some paths

Posted by Martin Kletzander 1 year, 7 months ago

On Thu, Sep 22, 2022 at 04:26:06PM +0200, Jiri Denemark wrote:
>On Thu, Sep 22, 2022 at 21:00:38 +0800, luzhipeng wrote:
>> From: lu zhipeng <luzhipeng@cestc.cn>
>>
>> virTypedParamsAddString may return -1 but alloc params,
>> so free it.
>>
>> Signed-off-by: lu zhipeng <luzhipeng@cestc.cn>
>> ---
>>  src/qemu/qemu_driver.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 94b70872d4..c4501cd705 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -4473,11 +4473,13 @@ qemuDomainPinVcpuLive(virDomainObj *vm,
>>          goto cleanup;
>>
>>      event = virDomainEventTunableNewFromObj(vm, eventParams, eventNparams);
>
>This call consumes eventParams which should not be used afterwards. Thus
>instead of
>
>> -
>> +    eventNparams = 0;
>
>we should set eventParams = NULL here...
>
>>      ret = 0;
>>
>>   cleanup:
>>      virObjectEventStateQueue(driver->domainEventState, event);
>
>... and call virTypedParamsFree unconditionally.
>
>> +    if (eventNparams)
>> +        virTypedParamsFree(eventParams, eventNparams);
>>      return ret;
>>  }
>>
>
>Alternatively, virDomainEventTunableNew could be changed to get
>virTypedParameterPtr *params and set &params = NULL itself.
>

That's what I wanted to suggest since it actually consumes the params as
it would make it more future-proof and easier to use.

>Jirka
>