src/storage/storage_util.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
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(-)
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);
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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); > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Apr 05, 2017 at 12:40:09 -0400, John Ferlan wrote: > > > 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. I vote for this. > > And as some would say the comment is "obvious". And for this. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2017-04-06 at 08:29 +0200, Peter Krempa wrote: > > If you went with this option, then VIR_STEAL_PTR(src->dir, volname); and > > VIR_STEAL_PTR(src->name, volname); instead of inlining. > > I vote for this. > > > And as some would say the comment is "obvious". > > And for this. Me too. I apparently forgot how to VIR_STEAL_PTR() :/ v2 coming right up. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.