I was surprised to see that virStoragePoolSourceFree(S) does not free S.
The other three vir*Free functions in storage_conf *do* free S.
There are at least three places where this causes leaks.
Here's one:
virStoragePoolSourcePtr
virStoragePoolDefParseSourceString(virConnectPtr conn,
const char *srcSpec,
int pool_type)
{
xmlDocPtr doc = NULL;
xmlNodePtr node = NULL;
xmlXPathContextPtr xpath_ctxt = NULL;
virStoragePoolSourcePtr def = NULL, ret = NULL;
doc = xmlReadDoc((const xmlChar *)srcSpec, "srcSpec.xml", NULL,
XML_PARSE_NOENT | XML_PARSE_NONET |
XML_PARSE_NOERROR | XML_PARSE_NOWARNING);
if (doc == NULL) {
virStorageReportError(conn, VIR_ERR_XML_ERROR,
"%s", _("bad <source> spec"));
goto cleanup;
}
xpath_ctxt = xmlXPathNewContext(doc);
if (xpath_ctxt == NULL) {
virReportOOMError(conn);
goto cleanup;
}
if (VIR_ALLOC(def) < 0) { <<<====== def is alloc'd
virReportOOMError(conn);
goto cleanup;
}
node = virXPathNode(conn, "/source", xpath_ctxt);
if (!node) {
virStorageReportError(conn, VIR_ERR_XML_ERROR,
"%s", _("root element was not source"));
goto cleanup;
}
if (virStoragePoolDefParseSource(conn, xpath_ctxt, def, pool_type,
node) < 0)
goto cleanup;
ret = def;
def = NULL;
cleanup:
if (def)
virStoragePoolSourceFree(def); <<<<======== yet not freed
xmlFreeDoc(doc);
xmlXPathFreeContext(xpath_ctxt);
return ret;
}
One fix might be to call VIR_FREE(def) in the "if (def)..."
block, but that seems short-sighted, since the name
virStoragePoolSourceFree makes me think *it* should be
doing the whole job.
However, if I make the logical change to virStoragePoolSourceFree,
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 62b8394..ffe38cc 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -291,6 +291,7 @@ virStoragePoolSourceFree(virStoragePoolSourcePtr source) {
VIR_FREE(source->auth.chap.login);
VIR_FREE(source->auth.chap.passwd);
}
+ VIR_FREE(source);
}
that causes "make check" to fail miserably due to heap corruption,
as reported by valgrind:
==30311== Invalid free() / delete / delete[]
==30311== at 0x4A04D72: free (vg_replace_malloc.c:325)
==30311== by 0x42950C: virFree (memory.c:177)
==30311== by 0x4A2687: virStoragePoolSourceFree (storage_conf.c:294)
==30311== by 0x4A26BD: virStoragePoolDefFree (storage_conf.c:304)
==30311== by 0x4A2723: virStoragePoolObjFree (storage_conf.c:319)
==30311== by 0x4A27A2: virStoragePoolObjListFree (storage_conf.c:334)
==30311== by 0x4757AD: storageDriverShutdown (storage_driver.c:246)
==30311== by 0x4C1E3F: virStateCleanup (libvirt.c:909)
==30311== by 0x4103C5: qemudCleanup (libvirtd.c:2402)
==30311== by 0x41273B: main (libvirtd.c:3196)
==30311== Address 0x5c48c38 is 56 bytes inside a block of size 184 alloc'd
==30311== at 0x4A04481: calloc (vg_replace_malloc.c:418)
==30311== by 0x4293F5: virAlloc (memory.c:100)
==30311== by 0x4A330E: virStoragePoolDefParseXML (storage_conf.c:615)
==30311== by 0x4A3A21: virStoragePoolDefParseNode (storage_conf.c:762)
==30311== by 0x4A3BE6: virStoragePoolDefParse (storage_conf.c:810)
==30311== by 0x4A3C7F: virStoragePoolDefParseFile (storage_conf.c:834)
==30311== by 0x4A5816: virStoragePoolObjLoad (storage_conf.c:1495)
==30311== by 0x4A5BDB: virStoragePoolLoadAllConfigs (storage_conf.c:1575)
==30311== by 0x475598: storageDriverStartup (storage_driver.c:164)
==30311== by 0x4C1D9D: virStateInitialize (libvirt.c:888)
==30311== by 0x4125E9: main (libvirtd.c:3153)
I tracked the problem down to this definition in src/conf/storage_conf.h:
typedef struct _virStoragePoolDef virStoragePoolDef;
typedef virStoragePoolDef *virStoragePoolDefPtr;
struct _virStoragePoolDef {
/* General metadata */
char *name;
unsigned char uuid[VIR_UUID_BUFLEN];
int type; /* virStoragePoolType */
unsigned long long allocation;
unsigned long long capacity;
unsigned long long available;
virStoragePoolSource source; <== this is a *STRUCT*, not a ptr
virStoragePoolTarget target; <== Likewise
};
Hence, when calling virStoragePoolDefFree (defined like this)
void
virStoragePoolDefFree(virStoragePoolDefPtr def) {
if (!def)
return;
VIR_FREE(def->name);
virStoragePoolSourceFree(&def->source);
That function must *not* call VIR_FREE on &def->source,
because as valgrind tells us, above, that is in the middle
of a virStoragePoolDef buffer.
Thus, the naive patch seems to be the way to go,
unless you want to change the struct members to be pointers
(and adjust all uses -- much more invasive).
There are other leaks associated with similar uses of
virStoragePoolSourceFree just like this one.
Here's a patch that fixes them all at once.
If you agree in principle, I'll write a longer commit log
than the one below:
[note that there's no need to guard uses of
virStoragePoolSourceFree against NULL args, so I removed those. ]
>From a53efcee7ec74948a2cf4c1c02902419856b0154 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@xxxxxxxxxx>
Date: Fri, 5 Feb 2010 17:09:43 +0100
Subject: [PATCH] plug four virStoragePoolSourceFree-related leaks
* src/conf/storage_conf.c (virStoragePoolDefParseSourceString):
* src/storage/storage_backend_fs.c:
(virStorageBackendFileSystemNetFindPoolSourcesFunc):
(virStorageBackendFileSystemNetFindPoolSources):
* src/test/test_driver.c (testStorageFindPoolSources):
---
src/conf/storage_conf.c | 4 ++--
src/storage/storage_backend_fs.c | 8 ++++----
src/test/test_driver.c | 3 ++-
3 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 62b8394..b98b8e9 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -524,8 +524,8 @@ virStoragePoolDefParseSourceString(virConnectPtr conn,
ret = def;
def = NULL;
cleanup:
- if (def)
- virStoragePoolSourceFree(def);
+ virStoragePoolSourceFree(def);
+ VIR_FREE(def);
xmlFreeDoc(doc);
xmlXPathFreeContext(xpath_ctxt);
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 0d1c7a7..70e91a0 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -174,8 +174,8 @@ virStorageBackendFileSystemNetFindPoolSourcesFunc(virConnectPtr conn,
src = NULL;
ret = 0;
cleanup:
- if (src)
- virStoragePoolSourceFree(src);
+ virStoragePoolSourceFree(src);
+ VIR_FREE(src);
return ret;
}
@@ -236,8 +236,8 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn,
for (i = 0; i < state.list.nsources; i++)
virStoragePoolSourceFree(&state.list.sources[i]);
- if (source)
- virStoragePoolSourceFree(source);
+ virStoragePoolSourceFree(source);
+ VIR_FREE(source);
VIR_FREE(state.list.sources);
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 2122a1b..8e5ecf2 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1,7 +1,7 @@
/*
* test.c: A "mock" hypervisor for use by application unit tests
*
- * Copyright (C) 2006-2009 Red Hat, Inc.
+ * Copyright (C) 2006-2010 Red Hat, Inc.
* Copyright (C) 2006 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -3763,6 +3763,7 @@ testStorageFindPoolSources(virConnectPtr conn,
cleanup:
virStoragePoolSourceFree(source);
+ VIR_FREE(source);
return ret;
}
--
1.7.0.rc1.204.gb96e
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list