[libvirt] [PATCH 0/2] conf: fix some memory leaks
Ján Tomko (2): conf: Fix cpumask leak in virDomainDefFree conf: Fix leaks in virNetworkObjUpdateParseFile src/conf/domain_conf.c | 2 +- src/conf/network_conf.c | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) -- 1.7.8.6
def->cpumask is a bitmap and needs to be freed by virBitmapFree. --- src/conf/domain_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7273790..c29d2ef 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1738,7 +1738,7 @@ void virDomainDefFree(virDomainDefPtr def) virDomainClockDefClear(&def->clock); VIR_FREE(def->name); - VIR_FREE(def->cpumask); + virBitmapFree(def->cpumask); VIR_FREE(def->emulator); VIR_FREE(def->description); VIR_FREE(def->title); -- 1.7.8.6
On 25.01.2013 16:36, Ján Tomko wrote:
def->cpumask is a bitmap and needs to be freed by virBitmapFree. --- src/conf/domain_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7273790..c29d2ef 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1738,7 +1738,7 @@ void virDomainDefFree(virDomainDefPtr def) virDomainClockDefClear(&def->clock);
VIR_FREE(def->name); - VIR_FREE(def->cpumask); + virBitmapFree(def->cpumask); VIR_FREE(def->emulator); VIR_FREE(def->description); VIR_FREE(def->title);
ACK Michal
On 01/25/13 17:40, Michal Privoznik wrote:
On 25.01.2013 16:36, Ján Tomko wrote:
def->cpumask is a bitmap and needs to be freed by virBitmapFree. --- src/conf/domain_conf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
ACK
Michal
Thanks, pushed now.
Free the bitmap before calling virBitmapParse, which will overwrite it. Also free xml. --- src/conf/network_conf.c | 19 +++++++++++-------- 1 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index c93916d..013333c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1855,14 +1855,16 @@ virNetworkObjUpdateParseFile(const char *filename, ctxt->node = node; class_id = virXPathString("string(./class_id[1]/@bitmap)", ctxt); - if (class_id && - virBitmapParse(class_id, 0, - &net->class_id, CLASS_ID_BITMAP_SIZE) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Malformed 'class_id' attribute: %s"), - class_id); - VIR_FREE(class_id); - goto cleanup; + if (class_id) { + virBitmapFree(net->class_id); + if (virBitmapParse(class_id, 0, + &net->class_id, CLASS_ID_BITMAP_SIZE) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed 'class_id' attribute: %s"), + class_id); + VIR_FREE(class_id); + goto cleanup; + } } VIR_FREE(class_id); @@ -1896,6 +1898,7 @@ virNetworkObjUpdateParseFile(const char *filename, cleanup: xmlXPathFreeContext(ctxt); + xmlFreeDoc(xml); return ret; } -- 1.7.8.6
On 25.01.2013 16:36, Ján Tomko wrote:
Free the bitmap before calling virBitmapParse, which will overwrite it.
Also free xml. --- src/conf/network_conf.c | 19 +++++++++++-------- 1 files changed, 11 insertions(+), 8 deletions(-)
ACK Michal
On 01/25/2013 10:36 AM, Ján Tomko wrote:
Free the bitmap before calling virBitmapParse, which will overwrite it.
Also free xml. --- src/conf/network_conf.c | 19 +++++++++++-------- 1 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index c93916d..013333c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1855,14 +1855,16 @@ virNetworkObjUpdateParseFile(const char *filename,
ctxt->node = node; class_id = virXPathString("string(./class_id[1]/@bitmap)", ctxt); - if (class_id && - virBitmapParse(class_id, 0, - &net->class_id, CLASS_ID_BITMAP_SIZE) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Malformed 'class_id' attribute: %s"), - class_id); - VIR_FREE(class_id); - goto cleanup; + if (class_id) { + virBitmapFree(net->class_id);
If there is a situation where this function can be called and net->class_id is already filled in, then doesn't that also mean that net->floor_sum could have already been set? If that's the case, then we need to also set net->floor_sum to 0, in case it was previously non-0 and the new status doesn't have anything set (implying 0). If not, then this virBitmapFree() is a NOP.
+ if (virBitmapParse(class_id, 0, + &net->class_id, CLASS_ID_BITMAP_SIZE) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed 'class_id' attribute: %s"), + class_id); + VIR_FREE(class_id); + goto cleanup; + } } VIR_FREE(class_id);
@@ -1896,6 +1898,7 @@ virNetworkObjUpdateParseFile(const char *filename,
cleanup: xmlXPathFreeContext(ctxt); + xmlFreeDoc(xml); return ret; }
On 01/25/13 20:15, Laine Stump wrote:
On 01/25/2013 10:36 AM, Ján Tomko wrote:
Free the bitmap before calling virBitmapParse, which will overwrite it.
Also free xml. --- src/conf/network_conf.c | 19 +++++++++++-------- 1 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index c93916d..013333c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1855,14 +1855,16 @@ virNetworkObjUpdateParseFile(const char *filename,
ctxt->node = node; class_id = virXPathString("string(./class_id[1]/@bitmap)", ctxt); - if (class_id && - virBitmapParse(class_id, 0, - &net->class_id, CLASS_ID_BITMAP_SIZE) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Malformed 'class_id' attribute: %s"), - class_id); - VIR_FREE(class_id); - goto cleanup; + if (class_id) { + virBitmapFree(net->class_id);
If there is a situation where this function can be called and net->class_id is already filled in, then doesn't that also mean that net->floor_sum could have already been set?
If that's the case, then we need to also set net->floor_sum to 0, in case it was previously non-0 and the new status doesn't have anything set (implying 0).
If not, then this virBitmapFree() is a NOP.
This function only gets called on the network driver initialization, shortly after net->class_id gets allocated and filled with 111 in virNetworkAssignDef (see valgrind log below). net->floor_sum is always zero at this point. It looks like networkFindActiveConfigs (which calls this function) is missing from networkReload. Also, net->class_id or net->floor_sum might get changed by this function, even if the rest of the status file could not be parsed. Jan ==22881== 8,216 (24 direct, 8,192 indirect) bytes in 1 blocks are definitely lost in loss record 852 of 860 ==22881== at 0x4C2A462: calloc (vg_replace_malloc.c:593) ==22881== by 0x528E0C1: virAlloc (viralloc.c:100) ==22881== by 0x528FB9E: virBitmapNew (virbitmap.c:74) ==22881== by 0x530F61F: virNetworkAssignDef (network_conf.c:347) ==22881== by 0x530F821: virNetworkLoadConfig (network_conf.c:2392) ==22881== by 0x530F9AA: virNetworkLoadAllConfigs (network_conf.c:2437) ==22881== by 0x5104D9: networkStartup (bridge_driver.c:419) ==22881== by 0x533EBF7: virStateInitialize (libvirt.c:822) ==22881== by 0x428E8C: daemonRunStateInit (libvirtd.c:877) ==22881== by 0x52CC815: virThreadHelper (virthreadpthread.c:161) ==22881== by 0x805DEC5: start_thread (pthread_create.c:305) ==22881== by 0x87636EC: clone (clone.S:115)
participants (3)
-
Ján Tomko -
Laine Stump -
Michal Privoznik