[libvirt] [PATCH] conf: Fix memory leaks in virStoragePoolDefParseSource
Detected by valgrind. Leaks are introduced in commit 122fa379. src/conf/storage_conf.c: fix memory leaks. How to reproduce? $ make && make -C tests check TESTS=storagepoolxml2xmltest $ cd tests && valgrind -v --leak-check=full ./storagepoolxml2xmltest actual result: ==28571== LEAK SUMMARY: ==28571== definitely lost: 40 bytes in 5 blocks ==28571== indirectly lost: 0 bytes in 0 blocks ==28571== possibly lost: 0 bytes in 0 blocks ==28571== still reachable: 1,054 bytes in 21 blocks ==28571== suppressed: 0 bytes in 0 blocks Signed-off-by: Alex Jia <ajia@redhat.com> --- src/conf/storage_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 0b34f28..668e679 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -462,6 +462,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; } } + VIR_FREE(nodeset); } } -- 1.7.1
On 2012年05月09日 14:15, Alex Jia wrote:
Detected by valgrind. Leaks are introduced in commit 122fa379.
src/conf/storage_conf.c: fix memory leaks.
How to reproduce? $ make&& make -C tests check TESTS=storagepoolxml2xmltest $ cd tests&& valgrind -v --leak-check=full ./storagepoolxml2xmltest
actual result: ==28571== LEAK SUMMARY: ==28571== definitely lost: 40 bytes in 5 blocks ==28571== indirectly lost: 0 bytes in 0 blocks ==28571== possibly lost: 0 bytes in 0 blocks ==28571== still reachable: 1,054 bytes in 21 blocks ==28571== suppressed: 0 bytes in 0 blocks
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/conf/storage_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 0b34f28..668e679 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -462,6 +462,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; } } + VIR_FREE(nodeset); } }
NACK, not the right fix, "nodeset" will still be used by the 'for' loop. Osier
On 05/09/2012 04:01 PM, Osier Yang wrote:
On 2012年05月09日 14:15, Alex Jia wrote:
Detected by valgrind. Leaks are introduced in commit 122fa379.
src/conf/storage_conf.c: fix memory leaks.
How to reproduce? $ make&& make -C tests check TESTS=storagepoolxml2xmltest $ cd tests&& valgrind -v --leak-check=full ./storagepoolxml2xmltest
actual result: ==28571== LEAK SUMMARY: ==28571== definitely lost: 40 bytes in 5 blocks ==28571== indirectly lost: 0 bytes in 0 blocks ==28571== possibly lost: 0 bytes in 0 blocks ==28571== still reachable: 1,054 bytes in 21 blocks ==28571== suppressed: 0 bytes in 0 blocks
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/conf/storage_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 0b34f28..668e679 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -462,6 +462,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; } } + VIR_FREE(nodeset); } }
NACK, not the right fix, "nodeset" will still be used by the 'for' loop. Yeah, however, I originally tried to free 'nodeset' in a loop in cleanup label, it doesn't work and I still can see memory leaks, we use 'VIR_ALLOC_N()' allocate memory to 'nodeset' in virXPathNodeSet(), but I only free it by VIR_FREE().
Osier
On 05/09/2012 08:15 AM, Alex Jia wrote:
Detected by valgrind. Leaks are introduced in commit 122fa379.
src/conf/storage_conf.c: fix memory leaks.
How to reproduce? $ make&& make -C tests check TESTS=storagepoolxml2xmltest $ cd tests&& valgrind -v --leak-check=full ./storagepoolxml2xmltest
actual result: ==28571== LEAK SUMMARY: ==28571== definitely lost: 40 bytes in 5 blocks ==28571== indirectly lost: 0 bytes in 0 blocks ==28571== possibly lost: 0 bytes in 0 blocks ==28571== still reachable: 1,054 bytes in 21 blocks ==28571== suppressed: 0 bytes in 0 blocks
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/conf/storage_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 0b34f28..668e679 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -462,6 +462,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; } } + VIR_FREE(nodeset); } }
The problem with the added line is not visible with this context, so I'm adding some more: source->nhost = virXPathNodeSet("./host", ctxt, &nodeset); if (source->nhost) { if (VIR_ALLOC_N(source->hosts, source->nhost) < 0) { virReportOOMError(); goto cleanup; } for (i = 0 ; i < source->nhost ; i++) { name = virXMLPropString(nodeset[i], "name"); if (name == NULL) { virStorageReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool host name")); goto cleanup; } source->hosts[i].name = name; port = virXMLPropString(nodeset[i], "port"); if (port) { if (virStrToLong_i(port, NULL, 10, &source->hosts[i].port) < 0) { virStorageReportError(VIR_ERR_XML_ERROR, _("Invalid port number: %s"), port); goto cleanup; } } + VIR_FREE(nodeset); } } You added the VIR_FREE inside the for loop, so it gets freed before the next iteration and might cause a segfault in the second iteration. NACK Peter
On 05/09/2012 04:06 PM, Peter Krempa wrote:
On 05/09/2012 08:15 AM, Alex Jia wrote:
Detected by valgrind. Leaks are introduced in commit 122fa379.
src/conf/storage_conf.c: fix memory leaks.
How to reproduce? $ make&& make -C tests check TESTS=storagepoolxml2xmltest $ cd tests&& valgrind -v --leak-check=full ./storagepoolxml2xmltest
actual result: ==28571== LEAK SUMMARY: ==28571== definitely lost: 40 bytes in 5 blocks ==28571== indirectly lost: 0 bytes in 0 blocks ==28571== possibly lost: 0 bytes in 0 blocks ==28571== still reachable: 1,054 bytes in 21 blocks ==28571== suppressed: 0 bytes in 0 blocks
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/conf/storage_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 0b34f28..668e679 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -462,6 +462,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; } } + VIR_FREE(nodeset); } }
The problem with the added line is not visible with this context, so I'm adding some more:
source->nhost = virXPathNodeSet("./host", ctxt,&nodeset);
if (source->nhost) { if (VIR_ALLOC_N(source->hosts, source->nhost)< 0) { virReportOOMError(); goto cleanup; }
for (i = 0 ; i< source->nhost ; i++) { name = virXMLPropString(nodeset[i], "name"); if (name == NULL) { virStorageReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool host name")); goto cleanup; } source->hosts[i].name = name;
port = virXMLPropString(nodeset[i], "port"); if (port) { if (virStrToLong_i(port, NULL, 10,&source->hosts[i].port)< 0) { virStorageReportError(VIR_ERR_XML_ERROR, _("Invalid port number: %s"), port); goto cleanup; } } + VIR_FREE(nodeset); } }
You added the VIR_FREE inside the for loop, so it gets freed before the next iteration and might cause a segfault in the second iteration. Although I haven't meet a segfault error, I think you're right, thanks. NACK
Peter
On 05/09/2012 10:45 AM, Alex Jia wrote:
On 05/09/2012 04:06 PM, Peter Krempa wrote:
The problem with the added line is not visible with this context, so I'm adding some more:
source->nhost = virXPathNodeSet("./host", ctxt,&nodeset);
if (source->nhost) { if (VIR_ALLOC_N(source->hosts, source->nhost)< 0) { virReportOOMError(); goto cleanup; }
for (i = 0 ; i< source->nhost ; i++) { name = virXMLPropString(nodeset[i], "name"); if (name == NULL) { virStorageReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool host name")); goto cleanup; } source->hosts[i].name = name;
port = virXMLPropString(nodeset[i], "port"); if (port) { if (virStrToLong_i(port, NULL, 10,&source->hosts[i].port)< 0) { virStorageReportError(VIR_ERR_XML_ERROR, _("Invalid port number: %s"), port); goto cleanup; } } + VIR_FREE(nodeset); } }
You added the VIR_FREE inside the for loop, so it gets freed before the next iteration and might cause a segfault in the second iteration. Although I haven't meet a segfault error, I think you're right, thanks.
You were lucky and the loop iterated just once. If the xml contained more "host" elements you'd hit the SIGSEGV. The correct point to free the nodeset is _after_ the for loop. Peter
On 05/09/2012 05:50 PM, Peter Krempa wrote:
On 05/09/2012 10:45 AM, Alex Jia wrote:
On 05/09/2012 04:06 PM, Peter Krempa wrote:
The problem with the added line is not visible with this context, so I'm adding some more:
source->nhost = virXPathNodeSet("./host", ctxt,&nodeset);
if (source->nhost) { if (VIR_ALLOC_N(source->hosts, source->nhost)< 0) { virReportOOMError(); goto cleanup; }
for (i = 0 ; i< source->nhost ; i++) { name = virXMLPropString(nodeset[i], "name"); if (name == NULL) { virStorageReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool host name")); goto cleanup; } source->hosts[i].name = name;
port = virXMLPropString(nodeset[i], "port"); if (port) { if (virStrToLong_i(port, NULL, 10,&source->hosts[i].port)< 0) { virStorageReportError(VIR_ERR_XML_ERROR, _("Invalid port number: %s"), port); goto cleanup; } } + VIR_FREE(nodeset); } }
You added the VIR_FREE inside the for loop, so it gets freed before the next iteration and might cause a segfault in the second iteration. Although I haven't meet a segfault error, I think you're right, thanks.
You were lucky and the loop iterated just once. If the xml contained more "host" elements you'd hit the SIGSEGV. The correct point to free the nodeset is _after_ the for loop. Indeed, thanks.
Peter
participants (3)
-
Alex Jia -
Osier Yang -
Peter Krempa