On 04/05/2017 12:28 PM, Andrea Bolognani wrote:

The contents of volname would be leaked if the function were to be passed an invalid pooltype by the caller.

Make sure the memory is released instead. --- Initializing volname to NULL is strictly speaking not necessary, but I like it better that way :)

src/storage/storage_util.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)

This could be "as easy as" just adding a VIR_FREE(volname) in the "} else {" case prior to virReportError and goto cleanup; If you went with this option, then VIR_STEAL_PTR(src->dir, volname); and VIR_STEAL_PTR(src->name, volname); instead of inlining. And as some would say the comment is "obvious". IDC which way it happens... John

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 7cc125a..e0f948b 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -2846,7 +2846,7 @@ virStorageUtilGlusterExtractPoolSources(const char *host, xmlXPathContextPtr ctxt = NULL; xmlNodePtr *nodes = NULL; virStoragePoolSource *src = NULL; - char *volname; + char *volname = NULL; size_t i; int nnodes; int ret = -1; @@ -2871,12 +2871,17 @@ virStorageUtilGlusterExtractPoolSources(const char *host,

if (pooltype == VIR_STORAGE_POOL_NETFS) { src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS; + + /* Ownership of volname is passed to src */ src->dir = volname; + volname = NULL; } else if (pooltype == VIR_STORAGE_POOL_GLUSTER) { - src->name = volname; - if (VIR_STRDUP(src->dir, "/") < 0) goto cleanup; + + /* Ownership of volname is passed to src */ + src->name = volname; + volname = NULL; } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unsupported gluster lookup")); @@ -2894,6 +2899,7 @@ virStorageUtilGlusterExtractPoolSources(const char *host, ret = nnodes;

cleanup: + VIR_FREE(volname); VIR_FREE(nodes); xmlXPathFreeContext(ctxt); xmlFreeDoc(doc);