[libvirt] [PATCH] Fix memory leak in virDomainSnapshotDiskDefClear()
While running domainsnapshotxml2xmltest, it was found that valgrind pointed out the following memory leaks: ==32176== 42 (32 direct, 10 indirect) bytes in 1 blocks are definitely lost in loss record 42 of 66 ==32176== at 0x4A069EE: malloc (vg_replace_malloc.c:270) ==32176== by 0x4A06B62: realloc (vg_replace_malloc.c:662) ==32176== by 0x4C65A07: virReallocN (viralloc.c:243) ==32176== by 0x4C65B2E: virExpandN (viralloc.c:292) ==32176== by 0x4C65E30: virInsertElementsN (viralloc.c:434) ==32176== by 0x4CD71F3: virDomainDiskSourceDefParse (domain_conf.c:5078) ==32176== by 0x4CF6EF4: virDomainSnapshotDefParseNode (snapshot_conf.c:151) ==32176== by 0x4CF7314: virDomainSnapshotDefParseString (snapshot_conf.c:410) ==32176== by 0x41FB8D: testCompareXMLToXMLHelper (domainsnapshotxml2xmltest.c:100) ==32176== by 0x420FD1: virtTestRun (testutils.c:199) ==32176== by 0x41F859: mymain (domainsnapshotxml2xmltest.c:222) ==32176== by 0x42174D: virtTestMain (testutils.c:782) ==32176== ==32176== 128 (96 direct, 32 indirect) bytes in 1 blocks are definitely lost in loss record 51 of 66 ==32176== at 0x4A06BE0: realloc (vg_replace_malloc.c:662) ==32176== by 0x4C65A07: virReallocN (viralloc.c:243) ==32176== by 0x4C65B2E: virExpandN (viralloc.c:292) ==32176== by 0x4C65E30: virInsertElementsN (viralloc.c:434) ==32176== by 0x4CD71F3: virDomainDiskSourceDefParse (domain_conf.c:5078) ==32176== by 0x4CF6EF4: virDomainSnapshotDefParseNode (snapshot_conf.c:151) ==32176== by 0x4CF7314: virDomainSnapshotDefParseString (snapshot_conf.c:410) ==32176== by 0x41FB8D: testCompareXMLToXMLHelper (domainsnapshotxml2xmltest.c:100) ==32176== by 0x420FD1: virtTestRun (testutils.c:199) ==32176== by 0x41F859: mymain (domainsnapshotxml2xmltest.c:222) ==32176== by 0x42174D: virtTestMain (testutils.c:782) ==32176== by 0x3E6CE1ED1C: (below main) (libc-start.c:226) ==32176== --- src/conf/snapshot_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 12b0930..475525f 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -82,6 +82,7 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk) { VIR_FREE(disk->name); VIR_FREE(disk->file); + virDomainDiskHostDefFree(disk->nhosts, disk->hosts); } void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) -- 1.7.1
On 02/22/2014 08:37 PM, Nehal J Wani wrote:
While running domainsnapshotxml2xmltest, it was found that valgrind pointed out the following memory leaks:
==32176== 42 (32 direct, 10 indirect) bytes in 1 blocks are definitely lost in loss record 42 of 66 ==32176== at 0x4A069EE: malloc (vg_replace_malloc.c:270) ==32176== by 0x4A06B62: realloc (vg_replace_malloc.c:662) ==32176== by 0x4C65A07: virReallocN (viralloc.c:243) ==32176== by 0x4C65B2E: virExpandN (viralloc.c:292) ==32176== by 0x4C65E30: virInsertElementsN (viralloc.c:434) ==32176== by 0x4CD71F3: virDomainDiskSourceDefParse (domain_conf.c:5078) ==32176== by 0x4CF6EF4: virDomainSnapshotDefParseNode (snapshot_conf.c:151) ==32176== by 0x4CF7314: virDomainSnapshotDefParseString (snapshot_conf.c:410) ==32176== by 0x41FB8D: testCompareXMLToXMLHelper (domainsnapshotxml2xmltest.c:100) ==32176== by 0x420FD1: virtTestRun (testutils.c:199) ==32176== by 0x41F859: mymain (domainsnapshotxml2xmltest.c:222) ==32176== by 0x42174D: virtTestMain (testutils.c:782) ==32176== ==32176== 128 (96 direct, 32 indirect) bytes in 1 blocks are definitely lost in loss record 51 of 66 ==32176== at 0x4A06BE0: realloc (vg_replace_malloc.c:662) ==32176== by 0x4C65A07: virReallocN (viralloc.c:243) ==32176== by 0x4C65B2E: virExpandN (viralloc.c:292) ==32176== by 0x4C65E30: virInsertElementsN (viralloc.c:434) ==32176== by 0x4CD71F3: virDomainDiskSourceDefParse (domain_conf.c:5078) ==32176== by 0x4CF6EF4: virDomainSnapshotDefParseNode (snapshot_conf.c:151) ==32176== by 0x4CF7314: virDomainSnapshotDefParseString (snapshot_conf.c:410) ==32176== by 0x41FB8D: testCompareXMLToXMLHelper (domainsnapshotxml2xmltest.c:100) ==32176== by 0x420FD1: virtTestRun (testutils.c:199) ==32176== by 0x41F859: mymain (domainsnapshotxml2xmltest.c:222) ==32176== by 0x42174D: virtTestMain (testutils.c:782) ==32176== by 0x3E6CE1ED1C: (below main) (libc-start.c:226) ==32176==
--- src/conf/snapshot_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 12b0930..475525f 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -82,6 +82,7 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk) { VIR_FREE(disk->name); VIR_FREE(disk->file); + virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
This leaves nhosts and hosts at their original values, which seems to be OK everywhere this function is called, but it might be a problem if someone tries to reuse the disk definition instead of freeing it. I'd rather write this as: while (disk->nhosts) virDomainDiskHostDefClear(&disk->hosts[--def->nhosts]) VIR_FREE(disk->hosts) Jan
--- src/conf/snapshot_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 12b0930..475525f 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -82,6 +82,7 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk) { VIR_FREE(disk->name); VIR_FREE(disk->file); + virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
This leaves nhosts and hosts at their original values, which seems to be OK everywhere this function is called, but it might be a problem if someone tries to reuse the disk definition instead of freeing it.
I'd rather write this as:
while (disk->nhosts) virDomainDiskHostDefClear(&disk->hosts[--def->nhosts]) VIR_FREE(disk->hosts)
Jan
1. Since virDomainDiskHostDefFree() already calls VIR_FREE(hosts), shouldn't the modified patch just be: diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 12b0930..a233e8e 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -82,6 +82,8 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk) { VIR_FREE(disk->name); VIR_FREE(disk->file); + virDomainDiskHostDefFree(disk->nhosts, disk->hosts); + disk->nhosts = 0; } void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) 2. I have a question. Why don't we have disk->nhosts = 0; each time someone calls virDomainDiskHostDefFree(disk->nhosts, disk->hosts) in ./src/conf/domain_conf.c, ./src/storage/storage_driver.c and ./src/qemu/qemu_conf.c? Or we can change the definition of virDomainDiskHostDefFree to virDomainDiskHostDefFree(size_t *nhosts, virDomainDiskHostDefPtr hosts) and make *nhosts = 0 inside the function itself and make the necessary changes wherever this function is called? -- Nehal J Wani
[cc-ing Peter as he invented the function] On 03/03/2014 09:04 PM, Nehal J Wani wrote:
--- src/conf/snapshot_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 12b0930..475525f 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -82,6 +82,7 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk) { VIR_FREE(disk->name); VIR_FREE(disk->file); + virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
This leaves nhosts and hosts at their original values, which seems to be OK everywhere this function is called, but it might be a problem if someone tries to reuse the disk definition instead of freeing it.
I'd rather write this as:
while (disk->nhosts) virDomainDiskHostDefClear(&disk->hosts[--def->nhosts]) VIR_FREE(disk->hosts)
Jan
1. Since virDomainDiskHostDefFree() already calls VIR_FREE(hosts), shouldn't the modified patch just be: diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 12b0930..a233e8e 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -82,6 +82,8 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk) { VIR_FREE(disk->name); VIR_FREE(disk->file); + virDomainDiskHostDefFree(disk->nhosts, disk->hosts); + disk->nhosts = 0; }
This leaves the pointer to the freed memory in disk->hosts.
void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
2. I have a question. Why don't we have disk->nhosts = 0; each time someone calls virDomainDiskHostDefFree(disk->nhosts, disk->hosts) in ./src/conf/domain_conf.c, ./src/storage/storage_driver.c and ./src/qemu/qemu_conf.c?
In most cases either the whole def gets freed, or hosts and nhosts are overwritten. I'm not sure if we overwrite it in all cases in qemuTranslateDiskSourcePool or it needs to be added there too.
Or we can change the definition of virDomainDiskHostDefFree to virDomainDiskHostDefFree(size_t *nhosts, virDomainDiskHostDefPtr hosts) and make *nhosts = 0 inside the function itself and make the necessary changes wherever this function is called?
That looks more awkward than the original function to me. I think it should be called virDomainDiskHostDefsFree instead, since it's different from all the other DefFree functions, which only take one DefPtr argument. The callers should make sure that the leftover values in nhosts/hosts aren't reused. Jan
On 03/04/14 10:44, Ján Tomko wrote:
[cc-ing Peter as he invented the function]
On 03/03/2014 09:04 PM, Nehal J Wani wrote:
--- src/conf/snapshot_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 12b0930..475525f 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -82,6 +82,7 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk) { VIR_FREE(disk->name); VIR_FREE(disk->file); + virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
This leaves nhosts and hosts at their original values, which seems to be OK everywhere this function is called, but it might be a problem if someone tries to reuse the disk definition instead of freeing it.
Maybe we should just note that this is happening in the function and let our future selves worry about that in case somebody will need to use the clear function in a different context.
I'd rather write this as:
while (disk->nhosts) virDomainDiskHostDefClear(&disk->hosts[--def->nhosts]) VIR_FREE(disk->hosts)
Jan
1. Since virDomainDiskHostDefFree() already calls VIR_FREE(hosts), shouldn't the modified patch just be: diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 12b0930..a233e8e 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -82,6 +82,8 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk) { VIR_FREE(disk->name); VIR_FREE(disk->file); + virDomainDiskHostDefFree(disk->nhosts, disk->hosts); + disk->nhosts = 0; }
This leaves the pointer to the freed memory in disk->hosts.
void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
2. I have a question. Why don't we have disk->nhosts = 0; each time someone calls virDomainDiskHostDefFree(disk->nhosts, disk->hosts) in ./src/conf/domain_conf.c, ./src/storage/storage_driver.c and ./src/qemu/qemu_conf.c?
In most cases either the whole def gets freed, or hosts and nhosts are overwritten. I'm not sure if we overwrite it in all cases in qemuTranslateDiskSourcePool or it needs to be added there too.
Or we can change the definition of virDomainDiskHostDefFree to virDomainDiskHostDefFree(size_t *nhosts, virDomainDiskHostDefPtr hosts) and make *nhosts = 0 inside the function itself and make the necessary changes wherever this function is called?
That looks more awkward than the original function to me.
Seconded. Additionally it would require a macro as calling the function with reference operators would be even more ugly.
I think it should be called virDomainDiskHostDefsFree instead, since it's different from all the other DefFree functions, which only take one DefPtr argument. The callers should make sure that the leftover values in nhosts/hosts aren't reused.
Hmmm I must agree that my choice of name for this function wasn't ideal. It does indeed free more entries, so that the plural form would be a better choice.
Jan
Peter
participants (3)
-
Ján Tomko -
Nehal J Wani -
Peter Krempa