[libvirt] [PATCH] remote: avoid memory leak on successful path
From: Alex Jia <ajia@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@redhat.com> --- src/remote/remote_driver.c | 7 +------ 1 files changed, 1 insertions(+), 6 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index e28840b..cfbcd0f 100644 --- a/src/remote/remote_driver.c +++ 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); return rv; } -- 1.7.1
On 01/17/2012 02:59 AM, ajia@redhat.com wrote:
From: Alex Jia <ajia@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@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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/18/2012 05:32 AM, Eric Blake wrote:
On 01/17/2012 02:59 AM, ajia@redhat.com wrote:
From: Alex Jia<ajia@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@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.
participants (3)
-
ajia＠redhat.com -
Alex Jia
-
Eric Blake