On 28.06.2016 13:48, Daniel P. Berrange wrote:
On Tue, Jun 28, 2016 at 01:43:56PM +0200, Michal Privoznik wrote:
When working with typed params calling pure free() over an array of params is not enough. Some items in the array may be type of string in which case they have a pointer to an allocated memory too. Therefore we should use virTypedParamsFree() which does all the necessary.
Moreover, some methods (set_interface_parameters() and set_block_iotune()) were missing any free at all.
==16768== 1 bytes in 1 blocks are definitely lost in loss record 2 of 7,378 ==16768== at 0x4C29F80: malloc (vg_replace_malloc.c:296) ==16768== by 0x52603B9: strdup (strdup.c:42) ==16768== by 0x66222E4: virStrdup (in /usr/lib64/libvirt.so.0.2000.0) ==16768== by 0x662B92A: virTypedParamsDeserialize (in /usr/lib64/libvirt.so.0.2000.0) ==16768== by 0x671D7F2: remoteDomainGetNumaParameters (in /usr/lib64/libvirt.so.0.2000.0) ==16768== by 0x66CA1BE: virDomainGetNumaParameters (in /usr/lib64/libvirt.so.0.2000.0) ==16768== by 0x630CF74: XS_Sys__Virt__Domain_get_numa_parameters (Virt.xs:4224) ==16768== by 0x4EF920A: Perl_pp_entersub (in /usr/lib64/libperl.so.5.20.2) ==16768== by 0x4EF1A42: Perl_runops_standard (in /usr/lib64/libperl.so.5.20.2) ==16768== by 0x4E8BE58: perl_run (in /usr/lib64/libperl.so.5.20.2) ==16768== by 0x400D9A: main (in /usr/bin/perl)
==16768== 672 bytes in 1 blocks are definitely lost in loss record 6,993 of 7,378 ==16768== at 0x4C29F80: malloc (vg_replace_malloc.c:296) ==16768== by 0x4ED6984: Perl_safesysmalloc (in /usr/lib64/libperl.so.5.20.2) ==16768== by 0x63164DF: XS_Sys__Virt__Domain_set_interface_parameters (Virt.xs:5024) ==16768== by 0x4EF920A: Perl_pp_entersub (in /usr/lib64/libperl.so.5.20.2) ==16768== by 0x4EF1A42: Perl_runops_standard (in /usr/lib64/libperl.so.5.20.2) ==16768== by 0x4E8BE58: perl_run (in /usr/lib64/libperl.so.5.20.2) ==16768== by 0x400D9A: main (in /usr/bin/perl)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Virt.xs | 90 +++++++++++++++++++++++++++++++++-------------------------------- 1 file changed, 46 insertions(+), 44 deletions(-)
Please mention which APIs had leaks fixed in the Changes file too.
diff --git a/Virt.xs b/Virt.xs index cf4e5bc..3b59f8a 100644 --- a/Virt.xs +++ b/Virt.xs @@ -2246,12 +2246,12 @@ get_node_memory_parameters(conn, flags=0) Newx(params, nparams, virTypedParameter);
if (virNodeGetMemoryParameters(conn, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams);
This change is not safe, because 'Newx' is not guaranteed to correspond to a plain 'malloc' call. Any 'Newx' call *must* be matched by Safefree.
Ah, okay. Finding a good docs on perl C API is just painful.
If libvirt allocated the typed params array, it is fine to call virTypedParamsFree which I did for the recent virDomainGetGuestVCPUs API, but for most existing APIs we can't do this.
Okay. Understood.
You'll need to create a local SafeTypedParamsFree() which iterates over each element free'ing strings, and then calling Safefree on the array itself.
Will do. Thanks for the review. Michal