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

[Eric Blake]

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

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

> +++ 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);

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.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120117/74cb903f/attachment-0001.sig>