[libvirt] [libvirt-perl][PATCH 0/3] Fix a few memleaks
I've noticed these while trying to write support for the new libvirt APIs (but Dan was quicker :-)). Michal Privoznik (3): Prefer virTypedParameterPtr over virTypedParameter * get/set_memory_parameters: Favour virTypedParameter over virMemoryParameter Substitute Safefree with virTypedParamsFree Virt.xs | 146 ++++++++++++++++++++++++++++++++-------------------------------- 1 file changed, 74 insertions(+), 72 deletions(-) -- 2.8.4
There's no functional change, this is just a cosmetic fix. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Virt.xs | 50 +++++++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/Virt.xs b/Virt.xs index aed7f6f..99ab150 100644 --- a/Virt.xs +++ b/Virt.xs @@ -159,7 +159,7 @@ _populate_constant_ull(HV *stash, const char *name, unsigned long long val) #define REGISTER_CONSTANT_ULL(name, key) _populate_constant_ull(stash, #key, name) static HV * -vir_typed_param_to_hv(virTypedParameter *params, int nparams) +vir_typed_param_to_hv(virTypedParameterPtr params, int nparams) { HV *ret = (HV *)sv_2mortal((SV*)newHV()); unsigned int i; @@ -210,7 +210,7 @@ vir_typed_param_to_hv(virTypedParameter *params, int nparams) static int -vir_typed_param_from_hv(HV *newparams, virTypedParameter *params, int nparams) +vir_typed_param_from_hv(HV *newparams, virTypedParameterPtr params, int nparams) { unsigned int i; char * ptr; @@ -274,7 +274,7 @@ vir_typed_param_from_hv(HV *newparams, virTypedParameter *params, int nparams) static void vir_typed_param_add_string_list_from_hv(HV *newparams, - virTypedParameter **params, + virTypedParameterPtr *params, int *nparams, const char *key) { @@ -282,7 +282,7 @@ vir_typed_param_add_string_list_from_hv(HV *newparams, return; } SSize_t nstr, i; - virTypedParameter *localparams = *params; + virTypedParameterPtr localparams = *params; SV **val = hv_fetch(newparams, key, strlen(key), 0); AV *av = (AV*)(SvRV(*val)); @@ -2236,7 +2236,7 @@ get_node_memory_parameters(conn, flags=0) virConnectPtr conn; unsigned int flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; CODE: nparams = 0; @@ -2262,7 +2262,7 @@ set_node_memory_parameters(conn, newparams, flags=0) HV *newparams; unsigned int flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; PPCODE: nparams = 0; @@ -3918,7 +3918,7 @@ get_job_stats(dom, flags=0) unsigned int flags; PREINIT: int type; - virTypedParameter *params; + virTypedParameterPtr params; int nparams; HV *paramsHv; SV *typeSv; @@ -4017,7 +4017,7 @@ block_copy(dom, path, destxml, newparams, flags=0) HV *newparams; unsigned long flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; PPCODE: nparams = 3; @@ -4092,7 +4092,7 @@ get_scheduler_parameters(dom, flags=0) virDomainPtr dom; unsigned int flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; char *type; CODE: @@ -4124,7 +4124,7 @@ set_scheduler_parameters(dom, newparams, flags=0) HV *newparams; unsigned int flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; char *type; PPCODE: @@ -4186,7 +4186,7 @@ set_memory_parameters(dom, newparams, flags=0) HV *newparams; unsigned int flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; PPCODE: nparams = 0; @@ -4212,7 +4212,7 @@ get_numa_parameters(dom, flags=0) virDomainPtr dom; unsigned int flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; CODE: nparams = 0; @@ -4238,7 +4238,7 @@ set_numa_parameters(dom, newparams, flags=0) HV *newparams; unsigned int flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; PPCODE: nparams = 0; @@ -4264,7 +4264,7 @@ get_blkio_parameters(dom, flags=0) virDomainPtr dom; unsigned int flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; CODE: nparams = 0; @@ -4290,7 +4290,7 @@ set_blkio_parameters(dom, newparams, flags=0) HV *newparams; unsigned int flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; PPCODE: nparams = 0; @@ -4317,7 +4317,7 @@ get_perf_events(dom, flags=0) virDomainPtr dom; unsigned int flags; PREINIT: - virTypedParameter *params = NULL; + virTypedParameterPtr params = NULL; int nparams = 0; CODE: if (virDomainGetPerfEvents(dom, ¶ms, &nparams, flags) < 0) { @@ -4337,7 +4337,7 @@ set_perf_events(dom, newparams, flags=0) HV *newparams; unsigned int flags; PREINIT: - virTypedParameter *params = NULL; + virTypedParameterPtr params = NULL; int nparams = 0; PPCODE: if (virDomainGetPerfEvents(dom, ¶ms, &nparams, flags) < 0) { @@ -4448,7 +4448,7 @@ get_guest_vcpus(dom, flags=0) virDomainPtr dom; unsigned int flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; unsigned int nparams; CODE: if (virDomainGetGuestVcpus(dom, ¶ms, &nparams, flags) < 0) { @@ -4643,7 +4643,7 @@ _migrate(dom, destcon, newparams, flags=0) HV *newparams; unsigned long flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; CODE: nparams = 15; @@ -4734,7 +4734,7 @@ _migrate_to_uri(dom, desturi, newparams, flags=0) HV *newparams; unsigned long flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; PPCODE: nparams = 15; @@ -4937,7 +4937,7 @@ get_block_iotune(dom, disk, flags=0) const char *disk; unsigned int flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; CODE: nparams = 0; @@ -4964,7 +4964,7 @@ set_block_iotune(dom, disk, newparams, flags=0) HV *newparams; unsigned int flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; PPCODE: nparams = 0; @@ -4988,7 +4988,7 @@ get_interface_parameters(dom, intf, flags=0) const char *intf; unsigned int flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; CODE: nparams = 0; @@ -5015,7 +5015,7 @@ set_interface_parameters(dom, intf, newparams, flags=0) HV *newparams; unsigned int flags; PREINIT: - virTypedParameter *params; + virTypedParameterPtr params; int nparams; PPCODE: nparams = 0; @@ -5040,7 +5040,7 @@ block_stats(dom, path, flags=0) unsigned int flags; PREINIT: virDomainBlockStatsStruct stats; - virTypedParameter *params; + virTypedParameterPtr params; int nparams; unsigned int i; const char *field; -- 2.8.4
On Tue, Jun 28, 2016 at 01:43:54PM +0200, Michal Privoznik wrote:
There's no functional change, this is just a cosmetic fix.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Virt.xs | 50 +++++++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 25 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The libvirt-domain.h header file suggests using virTypedParameter as virMemoryParameter is just ancient alias for it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Virt.xs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Virt.xs b/Virt.xs index 99ab150..cf4e5bc 100644 --- a/Virt.xs +++ b/Virt.xs @@ -4160,14 +4160,14 @@ get_memory_parameters(dom, flags=0) virDomainPtr dom; unsigned int flags; PREINIT: - virMemoryParameter *params; + virTypedParameterPtr params; int nparams; CODE: nparams = 0; if (virDomainGetMemoryParameters(dom, NULL, &nparams, flags) < 0) _croak_error(); - Newx(params, nparams, virMemoryParameter); + Newx(params, nparams, virTypedParameter); if (virDomainGetMemoryParameters(dom, params, &nparams, flags) < 0) { Safefree(params); @@ -4193,7 +4193,7 @@ set_memory_parameters(dom, newparams, flags=0) if (virDomainGetMemoryParameters(dom, NULL, &nparams, flags) < 0) _croak_error(); - Newx(params, nparams, virMemoryParameter); + Newx(params, nparams, virTypedParameter); if (virDomainGetMemoryParameters(dom, params, &nparams, flags) < 0) { Safefree(params); -- 2.8.4
On Tue, Jun 28, 2016 at 01:43:55PM +0200, Michal Privoznik wrote:
The libvirt-domain.h header file suggests using virTypedParameter as virMemoryParameter is just ancient alias for it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Virt.xs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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(-) 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); _croak_error(); } RETVAL = vir_typed_param_to_hv(params, nparams); - Safefree(params); + virTypedParamsFree(params, nparams); OUTPUT: RETVAL @@ -2272,7 +2272,7 @@ set_node_memory_parameters(conn, newparams, flags=0) Newx(params, nparams, virTypedParameter); if (virNodeGetMemoryParameters(conn, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } @@ -2280,7 +2280,7 @@ set_node_memory_parameters(conn, newparams, flags=0) if (virNodeSetMemoryParameters(conn, params, nparams, flags) < 0) _croak_error(); - Safefree(params); + virTypedParamsFree(params, nparams); @@ -3924,13 +3924,13 @@ get_job_stats(dom, flags=0) SV *typeSv; PPCODE: if (virDomainGetJobStats(dom, &type, ¶ms, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } typeSv = newSViv(type); paramsHv = vir_typed_param_to_hv(params, nparams); - Safefree(params); + virTypedParamsFree(params, nparams); EXTEND(SP, 2); PUSHs(newRV_noinc((SV*)typeSv)); @@ -4038,11 +4038,11 @@ block_copy(dom, path, destxml, newparams, flags=0) nparams = vir_typed_param_from_hv(newparams, params, nparams); if (virDomainBlockCopy(dom, path, destxml, params, nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } - Safefree(params); + virTypedParamsFree(params, nparams); void @@ -4103,17 +4103,17 @@ get_scheduler_parameters(dom, flags=0) Newx(params, nparams, virTypedParameter); if (flags) { if (virDomainGetSchedulerParametersFlags(dom, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } } else { if (virDomainGetSchedulerParameters(dom, params, &nparams) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } } RETVAL = vir_typed_param_to_hv(params, nparams); - Safefree(params); + virTypedParamsFree(params, nparams); OUTPUT: RETVAL @@ -4135,12 +4135,12 @@ set_scheduler_parameters(dom, newparams, flags=0) Newx(params, nparams, virTypedParameter); if (flags) { if (virDomainGetSchedulerParametersFlags(dom, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } } else { if (virDomainGetSchedulerParameters(dom, params, &nparams) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } } @@ -4152,7 +4152,7 @@ set_scheduler_parameters(dom, newparams, flags=0) if (virDomainSetSchedulerParameters(dom, params, nparams) < 0) _croak_error(); } - Safefree(params); + virTypedParamsFree(params, nparams); HV * @@ -4170,12 +4170,12 @@ get_memory_parameters(dom, flags=0) Newx(params, nparams, virTypedParameter); if (virDomainGetMemoryParameters(dom, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } RETVAL = vir_typed_param_to_hv(params, nparams); - Safefree(params); + virTypedParamsFree(params, nparams); OUTPUT: RETVAL @@ -4196,7 +4196,7 @@ set_memory_parameters(dom, newparams, flags=0) Newx(params, nparams, virTypedParameter); if (virDomainGetMemoryParameters(dom, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } @@ -4204,7 +4204,7 @@ set_memory_parameters(dom, newparams, flags=0) if (virDomainSetMemoryParameters(dom, params, nparams, flags) < 0) _croak_error(); - Safefree(params); + virTypedParamsFree(params, nparams); HV * @@ -4222,12 +4222,12 @@ get_numa_parameters(dom, flags=0) Newx(params, nparams, virTypedParameter); if (virDomainGetNumaParameters(dom, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } RETVAL = vir_typed_param_to_hv(params, nparams); - Safefree(params); + virTypedParamsFree(params, nparams); OUTPUT: RETVAL @@ -4248,7 +4248,7 @@ set_numa_parameters(dom, newparams, flags=0) Newx(params, nparams, virTypedParameter); if (virDomainGetNumaParameters(dom, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } @@ -4256,7 +4256,7 @@ set_numa_parameters(dom, newparams, flags=0) if (virDomainSetNumaParameters(dom, params, nparams, flags) < 0) _croak_error(); - Safefree(params); + virTypedParamsFree(params, nparams); HV * @@ -4274,12 +4274,12 @@ get_blkio_parameters(dom, flags=0) Newx(params, nparams, virBlkioParameter); if (virDomainGetBlkioParameters(dom, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } RETVAL = vir_typed_param_to_hv(params, nparams); - Safefree(params); + virTypedParamsFree(params, nparams); OUTPUT: RETVAL @@ -4300,7 +4300,7 @@ set_blkio_parameters(dom, newparams, flags=0) Newx(params, nparams, virBlkioParameter); if (virDomainGetBlkioParameters(dom, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } @@ -4309,7 +4309,7 @@ set_blkio_parameters(dom, newparams, flags=0) if (virDomainSetBlkioParameters(dom, params, nparams, flags) < 0) _croak_error(); - Safefree(params); + virTypedParamsFree(params, nparams); HV * @@ -4321,12 +4321,12 @@ get_perf_events(dom, flags=0) int nparams = 0; CODE: if (virDomainGetPerfEvents(dom, ¶ms, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } RETVAL = vir_typed_param_to_hv(params, nparams); - Safefree(params); + virTypedParamsFree(params, nparams); OUTPUT: RETVAL @@ -4341,7 +4341,7 @@ set_perf_events(dom, newparams, flags=0) int nparams = 0; PPCODE: if (virDomainGetPerfEvents(dom, ¶ms, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } @@ -4349,7 +4349,7 @@ set_perf_events(dom, newparams, flags=0) if (virDomainSetPerfEvents(dom, params, nparams, flags) < 0) _croak_error(); - Safefree(params); + virTypedParamsFree(params, nparams); unsigned long @@ -4719,10 +4719,10 @@ _migrate(dom, destcon, newparams, flags=0) * if it is possible todo so */ if ((RETVAL = virDomainMigrate3(dom, destcon, params, nparams, flags)) == NULL) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } - Safefree(params); + virTypedParamsFree(params, nparams); OUTPUT: RETVAL @@ -4810,10 +4810,10 @@ _migrate_to_uri(dom, desturi, newparams, flags=0) * if it is possible todo so */ if (virDomainMigrateToURI3(dom, desturi, params, nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } - Safefree(params); + virTypedParamsFree(params, nparams); void @@ -4947,12 +4947,12 @@ get_block_iotune(dom, disk, flags=0) Newx(params, nparams, virTypedParameter); if (virDomainGetBlockIoTune(dom, disk, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } RETVAL = vir_typed_param_to_hv(params, nparams); - Safefree(params); + virTypedParamsFree(params, nparams); OUTPUT: RETVAL @@ -4973,13 +4973,14 @@ set_block_iotune(dom, disk, newparams, flags=0) Newx(params, nparams, virTypedParameter); if (virDomainGetBlockIoTune(dom, disk, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } nparams = vir_typed_param_from_hv(newparams, params, nparams); if (virDomainSetBlockIoTune(dom, disk, params, nparams, flags) < 0) _croak_error(); + virTypedParamsFree(params, nparams); HV * @@ -4998,12 +4999,12 @@ get_interface_parameters(dom, intf, flags=0) Newx(params, nparams, virTypedParameter); if (virDomainGetInterfaceParameters(dom, intf, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } RETVAL = vir_typed_param_to_hv(params, nparams); - Safefree(params); + virTypedParamsFree(params, nparams); OUTPUT: RETVAL @@ -5024,13 +5025,14 @@ set_interface_parameters(dom, intf, newparams, flags=0) Newx(params, nparams, virTypedParameter); if (virDomainGetInterfaceParameters(dom, intf, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } nparams = vir_typed_param_from_hv(newparams, params, nparams); if (virDomainSetInterfaceParameters(dom, intf, params, nparams, flags) < 0) _croak_error(); + virTypedParamsFree(params, nparams); HV * @@ -5066,7 +5068,7 @@ block_stats(dom, path, flags=0) Newx(params, nparams, virTypedParameter); if (virDomainBlockStatsFlags(dom, path, params, &nparams, flags) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } @@ -5086,7 +5088,7 @@ block_stats(dom, path, flags=0) (void)hv_store(RETVAL, field, strlen(field), val, 0); } } - Safefree(params); + virTypedParamsFree(params, nparams); } OUTPUT: RETVAL @@ -5312,7 +5314,7 @@ get_cpu_stats(dom, start_cpu, ncpus, flags=0) Newx(params, ncpus * nparams, virTypedParameter); if ((ret = virDomainGetCPUStats(dom, params, nparams, start_cpu, ncpus, flags)) < 0) { - Safefree(params); + virTypedParamsFree(params, nparams); _croak_error(); } @@ -5322,7 +5324,7 @@ get_cpu_stats(dom, start_cpu, ncpus, flags=0) PUSHs(newRV_noinc((SV *)rec)); } - Safefree(params); + virTypedParamsFree(params, nparams); void -- 2.8.4
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. 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. You'll need to create a local SafeTypedParamsFree() which iterates over each element free'ing strings, and then calling Safefree on the array itself. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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
On Tue, Jun 28, 2016 at 03:03:15PM +0200, Michal Privoznik wrote:
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.
FWIW, there are man pages which tell you most of the info, albeit in a fairly terse manner. 'perlapi' is the API reference in particular and says [quote] Newx The XSUB-writer's interface to the C "malloc" function. Memory obtained by this should ONLY be freed with "Safefree". [/quote] The 'perlguts' man page is more of a tutorial like thing describing Perl internals concepts. The 'perlxs' man page talks about the specific horrors of writing XS modules, aka perl extensions in C. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange -
Michal Privoznik