[libvirt] [PATCH] remote: avoid memory leak on successful path

[Alex Jia]

On 01/18/2012 05:32 AM, Eric Blake wrote:
> On 01/17/2012 02:59 AM, ajia at redhat.com wrote:
>> From: Alex Jia<ajia at redhat.com>
>>
>> when everything is okay, 'rv' will be 0, however, 'cleanup' lable only free
>> allocated memory with 'rv<  0', so memory leak on successful path. The patch
>> uses new virTypedParameterArrayClear function to free memory instead.
>>
>> * src/remote/remote_driver.c: fix memory leak on remoteDeserializeTypedParameters.
>>
>> * Detected by valgrind:
>>
>> ==31957== 1 bytes in 1 blocks are definitely lost in loss record 1 of 2,097
>> ==31957==    at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
>> ==31957==    by 0x39CF07F6E1: strdup (in /lib64/libc-2.12.so)
>> ==31957==    by 0xD84CDA1: remoteDeserializeTypedParameters.clone.0 (remote_driver.c:1404)
>> ==31957==    by 0xD84D11B: remoteDomainGetNumaParameters (remote_driver.c:1566)
>> ==31957==    by 0xD81EBFA: virDomainGetNumaParameters (libvirt.c:3887)
>> ==31957==    by 0xD4F8E25: libvirt_virDomainGetNumaParameters (libvirt-override.c:1127)
>> ==31957==    by 0x39E1ADE7F3: PyEval_EvalFrameEx (ceval.c:3794)
>> ==31957==    by 0x39E1ADF99E: PyEval_EvalFrameEx (ceval.c:3880)
>> ==31957==    by 0x39E1AE0466: PyEval_EvalCodeEx (ceval.c:3044)
>> ==31957==    by 0x39E1AE0541: PyEval_EvalCode (ceval.c:545)
>> ==31957==    by 0x39E1AFB88B: run_mod (pythonrun.c:1351)
>> ==31957==    by 0x39E1AFB95F: PyRun_FileExFlags (pythonrun.c:1337)
>>
>>
>> Signed-off-by: Alex Jia<ajia at redhat.com>
>> ---
>>   src/remote/remote_driver.c |    7 +------
>>   1 files changed, 1 insertions(+), 6 deletions(-)
> NACK.  Since params is an incoming parameter, it is up to the caller to
> free params, including its contents, on success.
>
> The memory leak resides instead in libvirt_virDomainGetNumaParameters,
> and you already posted a patch for a similar leak once:
>
> https://www.redhat.com/archives/libvir-list/2011-December/msg01132.html
Yeah, I have ever done a similar fixing :(
> However, my problem with the patch at that time is that we have a LOT of
> coding bugs in libvirt-override.c (we are disregarding allocation
> failures, failing to propagate python errors back to the caller, and so
> forth), where we are better off doing an audit of that entire file:
>
> https://www.redhat.com/archives/libvir-list/2011-December/msg01146.html
>
In addition, in python/typewrappers.c, we have 2 wrapper functions
'libvirt_constcharPtrWrap' and 'libvirt_constcharPtrWrap', which are
calling 'PyString_FromString' to return new reference of a string, but
they haven't released original string memory, I tried to free them using
free((void *) str), however, it seems it's a wrong, I met some invalid
read/free error in valgrind, the leaks still exist, If we can't free them in
python/typewrappers.c, it means caller must to free them in
libvirt-override.c.

Moreover, I saw we increased object reference in wrapper functions
from python/typewrappers.c by calling 'Py_INCREF', I'm not sure whether
we also need a 'Py_DECREF' in libvirt-override.c when calling wrapper
functions?
>> +++ b/src/remote/remote_driver.c
>> @@ -1417,12 +1417,7 @@ remoteDeserializeTypedParameters(remote_typed_param *ret_params_val,
>>       rv = 0;
>>
>>   cleanup:
>> -    if (rv<  0) {
>> -        int j;
>> -        for (j = 0; j<  i; j++)
>> -            if (params[j].type == VIR_TYPED_PARAM_STRING)
>> -                VIR_FREE(params[j].value.s);
>> -    }
>> +    virTypedParameterArrayClear(params, *nparams);
> If you want to touch remote_driver.c, to simplify this block of code,
> you could use:
>
> if (rv<  0)
>      virTypedParameterArrayClear(params, *nparams);
Yeah, but it will be better if we can commit a patch instead of many 
small patches
to simplify all of related codes :)
> rather than open-coding the cleanup loop.  But that still won't solve
> the fact that the real leak that needs to be plugged is in
> libvirt-override.c.
>
In deed.