[libvirt] [PATCH] storage: Fix the memory leak
The return value of virGetFCHostNameByWWN is a strdup'ed string.
Also add comments to declare that the caller should take care of
freeing it.
---
src/storage/storage_backend_scsi.c | 15 ++-
src/util/virutil.c | 5 -
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c
b/src/storage/storage_backend_scsi.c
index fce2bae..b38e530 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -668,6 +668,8 @@ static int
deleteVport(virStoragePoolSourceAdapter adapter)
{
unsigned int parent_host;
+char *name = NULL;
+int ret = -1;
if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
return 0;
@@ -680,18 +682,21 @@ deleteVport(virStoragePoolSourceAdapter adapter)
if (!adapter.data.fchost.parent)
return 0;
-if (!(virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
-adapter.data.fchost.wwpn)))
+if (!(name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
+ adapter.data.fchost.wwpn)))
return -1;
if (getHostNumber(adapter.data.fchost.parent, parent_host) 0)
-return -1;
+goto cleanup;
if (virManageVport(parent_host, adapter.data.fchost.wwpn,
adapter.data.fchost.wwnn, VPORT_DELETE) 0)
-return -1;
+goto cleanup;
-return 0;
+ret = 0;
+cleanup:
+VIR_FREE(name);
+return ret;
}
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 87cc2e7..7a2fbb0 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1820,7 +1820,10 @@ cleanup:
/* virGetHostNameByWWN:
*
* Iterate over the sysfs tree to get FC host name (e.g. host5)
- * by wwnn,wwpn pair.
+ * by the provided wwnn,wwpn pair.
+ *
+ * Returns the FC host name which must be freed by the caller,
+ * or NULL on failure.
*/
char *
virGetFCHostNameByWWN(const char *sysfs_prefix,
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: Fix the memory leak
On Thu, Jan 23, 2014 at 06:23:32PM +0800, Osier Yang wrote: The return value of virGetFCHostNameByWWN is a strdup'ed string. Also add comments to declare that the caller should take care of freeing it. --- src/storage/storage_backend_scsi.c | 15 ++- src/util/virutil.c | 5 - 2 files changed, 14 insertions(+), 6 deletions(-) ACK, Martin signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: Fix the memory leak
On 23/01/14 19:06, Martin Kletzander wrote: On Thu, Jan 23, 2014 at 06:23:32PM +0800, Osier Yang wrote: The return value of virGetFCHostNameByWWN is a strdup'ed string. Also add comments to declare that the caller should take care of freeing it. --- src/storage/storage_backend_scsi.c | 15 ++- src/util/virutil.c | 5 - 2 files changed, 14 insertions(+), 6 deletions(-) ACK, Thanks, pushed. Osier -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: fix unlikely memory leak in rbd backend
On 03/18/2013 06:28 PM, Eric Blake wrote: On 03/18/2013 02:07 PM, Laine Stump wrote: virStorageBackendRBDRefreshPool() first allocates an array big enough to hold 1024 names, then calls rbd_list(), which returns ERANGE if the array isn't big enough. When that happens, the VIR_ALLOC_N is called again with a larger size. Unfortunately, the original array isn't freed before allocating a new one. --- src/storage/storage_backend_rbd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 8a0e517..e815192 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -317,6 +317,7 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, VIR_WARN(%s, _(A problem occurred while listing RBD images)); goto cleanup; } +VIR_FREE(names); This works, but is possibly less efficient than using VIR_REALLOC_N instead of VIR_ALLOC_N in the first place. I had thought of that, but figured that internally it would likely be the same operation as a free + new malloc, but would also do a copy from the old region to new, which is pointless in this case, since the old memory hasn't been set to anything and will be immediately overwritten anyway. ACK, since it's not on the hot path. I'm pushing as is. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] storage: fix unlikely memory leak in rbd backend
virStorageBackendRBDRefreshPool() first allocates an array big enough
to hold 1024 names, then calls rbd_list(), which returns ERANGE if the
array isn't big enough. When that happens, the VIR_ALLOC_N is called
again with a larger size. Unfortunately, the original array isn't
freed before allocating a new one.
---
src/storage/storage_backend_rbd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/storage/storage_backend_rbd.c
b/src/storage/storage_backend_rbd.c
index 8a0e517..e815192 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -317,6 +317,7 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr
conn ATTRIBUTE_UNUSED,
VIR_WARN(%s, _(A problem occurred while listing RBD images));
goto cleanup;
}
+VIR_FREE(names);
}
for (i = 0, name = names; name names + max_size; i++) {
--
1.7.11.7
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: fix unlikely memory leak in rbd backend
On 03/18/2013 02:07 PM, Laine Stump wrote: virStorageBackendRBDRefreshPool() first allocates an array big enough to hold 1024 names, then calls rbd_list(), which returns ERANGE if the array isn't big enough. When that happens, the VIR_ALLOC_N is called again with a larger size. Unfortunately, the original array isn't freed before allocating a new one. --- src/storage/storage_backend_rbd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 8a0e517..e815192 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -317,6 +317,7 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, VIR_WARN(%s, _(A problem occurred while listing RBD images)); goto cleanup; } +VIR_FREE(names); This works, but is possibly less efficient than using VIR_REALLOC_N instead of VIR_ALLOC_N in the first place. ACK, since it's not on the hot path. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: fix unlikely memory leak in rbd backend
On 03/18/2013 04:07 PM, Laine Stump wrote: virStorageBackendRBDRefreshPool() first allocates an array big enough to hold 1024 names, then calls rbd_list(), which returns ERANGE if the array isn't big enough. When that happens, the VIR_ALLOC_N is called again with a larger size. Unfortunately, the original array isn't freed before allocating a new one. --- src/storage/storage_backend_rbd.c | 1 + 1 file changed, 1 insertion(+) ACK Interesting only by upping the Coverity analysis level would this show up in a coverity analysis (along with 400 more). John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
