[libvirt] [PATCH 0/4] fix some resource leaks
Pavel Hrdina (4): conf/domain_capabilities: fix resource leak src: fix multiple resource leaks in loops rpc: fix resource leak tests: fix some resource leaks src/conf/domain_capabilities.c | 1 + src/conf/node_device_conf.c | 10 +++++----- src/conf/storage_conf.c | 1 + src/rpc/virnetdaemon.c | 1 + src/util/virsysinfo.c | 2 ++ tests/commandtest.c | 3 +++ tests/domaincapstest.c | 2 ++ tests/fchosttest.c | 2 ++ tests/vircgroupmock.c | 2 ++ 9 files changed, 19 insertions(+), 5 deletions(-) -- 2.12.2
Commit 14319c81a0 introduced CPU host model in domain capabilities and the *hostmodel* variable is always filled by virCPUDefCopy() and needs to be freed. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_capabilities.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 7a3d2e6fb1..7a81c10fd1 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -82,6 +82,7 @@ virDomainCapsDispose(void *obj) VIR_FREE(caps->path); VIR_FREE(caps->machine); virObjectUnref(caps->cpu.custom); + virCPUDefFree(caps->cpu.hostModel); virDomainCapsStringValuesFree(&caps->os.loader.values); } -- 2.12.2
All of the variables are filled inside a loop and therefore needs to be also freed in every cycle. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/node_device_conf.c | 10 +++++----- src/conf/storage_conf.c | 1 + src/util/virsysinfo.c | 2 ++ 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 7d0baa9d1a..e21a98d51f 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1582,7 +1582,6 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, for (i = 0, m = 0; i < n; i++) { xmlNodePtr node = nodes[i]; char *tmp = virXMLPropString(node, "type"); - virNodeDevDevnodeType type; int val; if (!tmp) { @@ -1591,15 +1590,16 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, goto error; } - if ((val = virNodeDevDevnodeTypeFromString(tmp)) < 0) { + val = virNodeDevDevnodeTypeFromString(tmp); + VIR_FREE(tmp); + + if (val < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown devnode type '%s'"), tmp); - VIR_FREE(tmp); goto error; } - type = val; - switch (type) { + switch ((virNodeDevDevnodeType)val) { case VIR_NODE_DEV_DEVNODE_DEV: def->devnode = (char*)xmlNodeGetContent(node); break; diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index fe0f0bcdc7..b66e67254a 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -471,6 +471,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, port); goto cleanup; } + VIR_FREE(port); } } } diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index 8d3377c04e..650216d006 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -521,6 +521,8 @@ virSysinfoParseS390Processor(const char *base, virSysinfoDefPtr ret) &processor->processor_family, '=', '\n')) goto cleanup; + + VIR_FREE(procline); } result = 0; -- 2.12.2
On Mon, Apr 10, 2017 at 09:53:58AM +0200, Pavel Hrdina wrote:
All of the variables are filled inside a loop and therefore needs to be also freed in every cycle.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/node_device_conf.c | 10 +++++----- src/conf/storage_conf.c | 1 + src/util/virsysinfo.c | 2 ++ 3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 7d0baa9d1a..e21a98d51f 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1582,7 +1582,6 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, for (i = 0, m = 0; i < n; i++) { xmlNodePtr node = nodes[i]; char *tmp = virXMLPropString(node, "type"); - virNodeDevDevnodeType type; int val;
if (!tmp) { @@ -1591,15 +1590,16 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, goto error; }
- if ((val = virNodeDevDevnodeTypeFromString(tmp)) < 0) { + val = virNodeDevDevnodeTypeFromString(tmp); + VIR_FREE(tmp); + + if (val < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown devnode type '%s'"), tmp);
This accesses the 'tmp' variable after it has been freed. I suggest preserving the error message, so there will be two VIR_FREE's needed - one for the success path and one for the error path. Jan
- VIR_FREE(tmp); goto error; }
Commit 252610f7dd1 switched to use hash to store servers. Function virHashGetItems returns allocated array which needs to be freed also for successful path, not only if there is an error. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/rpc/virnetdaemon.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index dcc89fa097..fabacf2039 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -401,6 +401,7 @@ virNetDaemonPreExecRestart(virNetDaemonPtr dmn) } } + VIR_FREE(srvArray); virObjectUnlock(dmn); return object; -- 2.12.2
Found by running valgrind for these tests. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/commandtest.c | 3 +++ tests/domaincapstest.c | 2 ++ tests/fchosttest.c | 2 ++ tests/vircgroupmock.c | 2 ++ 4 files changed, 9 insertions(+) diff --git a/tests/commandtest.c b/tests/commandtest.c index 80f10f88fc..1f6f16bcde 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1093,6 +1093,9 @@ static int test25(const void *unused ATTRIBUTE_UNUSED) virCommandPtr cmd = virCommandNew("some/nonexistent/binary"); rv = virCommandExec(cmd); + + virCommandFree(cmd); + if (safewrite(pipeFD[1], &rv, sizeof(rv)) < 0) fprintf(stderr, "Unable to write to pipe\n"); _exit(EXIT_FAILURE); diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index f871197c25..b76332ce94 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -563,6 +563,8 @@ mymain(void) DO_TEST_BHYVE("fbuf", "/usr/sbin/bhyve", &bhyve_caps, VIR_DOMAIN_VIRT_BHYVE); #endif /* WITH_BHYVE */ + virObjectUnref(cfg); + return ret; } diff --git a/tests/fchosttest.c b/tests/fchosttest.c index ac4e92910d..3ee1912926 100644 --- a/tests/fchosttest.c +++ b/tests/fchosttest.c @@ -315,6 +315,7 @@ manageVHBAByStoragePool(const void *data) ignore_value(virStoragePoolDestroy(pool)); goto cleanup; } + virNodeDeviceFree(dev); if (virStoragePoolDestroy(pool) < 0) goto cleanup; @@ -322,6 +323,7 @@ manageVHBAByStoragePool(const void *data) if ((dev = virNodeDeviceLookupByName(conn, expect_hostname))) { VIR_DEBUG("Found expected_hostname '%s' after destroy", expect_hostname); + virNodeDeviceFree(dev); goto cleanup; } diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c index ce6fd46266..9eb989ad7e 100644 --- a/tests/vircgroupmock.c +++ b/tests/vircgroupmock.c @@ -429,8 +429,10 @@ static void init_sysfs(void) abort(); \ if (make_controller(path, 0755) < 0) { \ fprintf(stderr, "Cannot initialize %s\n", path); \ + free(path); \ abort(); \ } \ + free(path); \ } while (0) MAKE_CONTROLLER("cpu"); -- 2.12.2
On Mon, Apr 10, 2017 at 09:54:00AM +0200, Pavel Hrdina wrote:
Found by running valgrind for these tests.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/commandtest.c | 3 +++ tests/domaincapstest.c | 2 ++ tests/fchosttest.c | 2 ++ tests/vircgroupmock.c | 2 ++ 4 files changed, 9 insertions(+)
diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index f871197c25..b76332ce94 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -563,6 +563,8 @@ mymain(void) DO_TEST_BHYVE("fbuf", "/usr/sbin/bhyve", &bhyve_caps, VIR_DOMAIN_VIRT_BHYVE); #endif /* WITH_BHYVE */
+ virObjectUnref(cfg); + return ret; }
The cfg variable is defined inside an #if WITH_QEMU block, so the unref should be inside one too. Jan
On Tue, Apr 11, 2017 at 12:51:58PM +0200, Ján Tomko wrote:
On Mon, Apr 10, 2017 at 09:53:56AM +0200, Pavel Hrdina wrote:
Pavel Hrdina (4): conf/domain_capabilities: fix resource leak src: fix multiple resource leaks in loops rpc: fix resource leak tests: fix some resource leaks
ACK series, terms and conditions apply.
Thanks, terms and conditions accepted and pushed. Pavel
participants (2)
-
Ján Tomko -
Pavel Hrdina