[libvirt] [PATCH] Resolve memory leak found by valgrind
Commit '6afdfc8e' adjusted the exit and error paths to go through the error and cleanup labels, but neglected to remove the return ret prior to cleanup. Also noted the 'type' xml string fetch was never checked for NULL which could lead to some interesting results. --- src/conf/storage_conf.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index f0ea41d..94eb69a 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -834,6 +834,12 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) } type = virXPathString("string(./@type)", ctxt); + if (type == NULL) { + virReportError(VIR_ERR_XML_ERROR, + _("storage pool missing type attribute")); + goto error; + } + if ((ret->type = virStoragePoolTypeFromString(type)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown storage pool type %s"), type); @@ -956,8 +962,6 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error; } - return ret; - cleanup: VIR_FREE(uuid); VIR_FREE(type); -- 1.8.1.4
On 31/05/13 21:54, John Ferlan wrote:
Commit '6afdfc8e' adjusted the exit and error paths to go through the error and cleanup labels, but neglected to remove the return ret prior to cleanup. Also noted the 'type' xml string fetch was never checked for NULL which could lead to some interesting results. --- src/conf/storage_conf.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index f0ea41d..94eb69a 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -834,6 +834,12 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) }
type = virXPathString("string(./@type)", ctxt); + if (type == NULL) { + virReportError(VIR_ERR_XML_ERROR, + _("storage pool missing type attribute")); + goto error; + } + if ((ret->type = virStoragePoolTypeFromString(type)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown storage pool type %s"), type); @@ -956,8 +962,6 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error; }
- return ret; -
ret needs to be set as "0" here. ret = 0;
cleanup: VIR_FREE(uuid); VIR_FREE(type); ACK with that.
On 31/05/13 21:59, Osier Yang wrote:
On 31/05/13 21:54, John Ferlan wrote:
Commit '6afdfc8e' adjusted the exit and error paths to go through the error and cleanup labels, but neglected to remove the return ret prior to cleanup. Also noted the 'type' xml string fetch was never checked for NULL which could lead to some interesting results. --- src/conf/storage_conf.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index f0ea41d..94eb69a 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -834,6 +834,12 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) } type = virXPathString("string(./@type)", ctxt); + if (type == NULL) { + virReportError(VIR_ERR_XML_ERROR, + _("storage pool missing type attribute")); + goto error; + } + if ((ret->type = virStoragePoolTypeFromString(type)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown storage pool type %s"), type); @@ -956,8 +962,6 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error; } - return ret; -
ret needs to be set as "0" here.
ret = 0;
Pointed out by Peter. I made a fast/stupid mistake, the function actually returns a pointer. So it's okay here. Worth to have in 1.0.6.
cleanup: VIR_FREE(uuid); VIR_FREE(type); ACK with that.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/31/2013 09:54 AM, John Ferlan wrote:
Commit '6afdfc8e' adjusted the exit and error paths to go through the error and cleanup labels, but neglected to remove the return ret prior to cleanup. Also noted the 'type' xml string fetch was never checked for NULL which could lead to some interesting results. --- src/conf/storage_conf.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index f0ea41d..94eb69a 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -834,6 +834,12 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) }
type = virXPathString("string(./@type)", ctxt); + if (type == NULL) { + virReportError(VIR_ERR_XML_ERROR, + _("storage pool missing type attribute")); + goto error; + } + if ((ret->type = virStoragePoolTypeFromString(type)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown storage pool type %s"), type); @@ -956,8 +962,6 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error; }
- return ret; - cleanup: VIR_FREE(uuid); VIR_FREE(type);
Peter also pointed out I was missing the "%s" in the new type check error path. Something syntax-check would have caught (of course I had run that on my other patch sent earlier, but forgot to on this one, sigh). In any case, with the "%s" added, syntax-check passing, and rerun of valgrind, I pushed. Thanks, John
participants (2)
-
John Ferlan -
Osier Yang