[libvirt] [PATCH 0/5] Various memleak fixes
*** BLURB HERE *** Michal Prívozník (5): domaincapstest: Don't leak cpu definitions testutilsxen: Avoid double free of driver caps virCapabilitiesHostNUMAUnref: Accept NULL qemu: Reoder cleanup in qemuStateCleanup() qemu: Don't leak hostcpu or hostnuma on driver cleanup src/conf/capabilities.c | 3 +++ src/qemu/qemu_driver.c | 45 +++++++++++++++++------------------------ tests/domaincapstest.c | 2 +- tests/testutilsxen.c | 1 - 4 files changed, 23 insertions(+), 28 deletions(-) -- 2.24.1
When generating domain capabilities, we need to fake host CPU to get reproducible result. We do this by copying a pre-existent CPU config and setting VIR_TEST_MOCK_FAKE_HOST_CPU env variable which is then consumed by qemucpumock. However, we forget to free the CPU copy afterwards. 2,196 (2,016 direct, 180 indirect) bytes in 18 blocks are definitely lost in loss record 291 of 297 at 0x4838B86: calloc (vg_replace_malloc.c:762) by 0x57CB6A0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.7) by 0x4A0F72D: virCPUDefNew (cpu_conf.c:87) by 0x4A0FAC7: virCPUDefCopyWithoutModel (cpu_conf.c:235) by 0x4A0FBBE: virCPUDefCopy (cpu_conf.c:273) by 0x10E3C0: testUtilsHostCpusGetDefForArch (testutilshostcpus.h:157) by 0x10E3C0: fakeHostCPU (domaincapstest.c:61) by 0x10E3C0: fillQemuCaps (domaincapstest.c:86) by 0x10E3C0: test_virDomainCapsFormat (domaincapstest.c:234) by 0x10F4BC: virTestRun (testutils.c:146) by 0x10DE93: doTestQemuInternal (domaincapstest.c:301) by 0x10E13D: doTestQemu (domaincapstest.c:332) by 0x1124CF: testQemuCapsIterate (testutilsqemu.c:635) by 0x10DCE3: mymain (domaincapstest.c:435) by 0x10FD8B: virTestMain (testutils.c:916) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/domaincapstest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index a4a443b1d6..9f5eab3230 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -56,7 +56,7 @@ fillStringValues(virDomainCapsStringValuesPtr values, ...) static int fakeHostCPU(virArch arch) { - virCPUDefPtr cpu; + g_autoptr(virCPUDef) cpu = NULL; if (!(cpu = testUtilsHostCpusGetDefForArch(arch))) { virReportError(VIR_ERR_INTERNAL_ERROR, -- 2.24.1
In testXLInitDriver() a dummy driver structure is filled and it is freed later in testXLFreeDriver(). However, it is sufficient to unref just driver->config because that results in libxlDriverConfigDispose() being called which unrefs driver->config->caps. There is no need to unref it again in testXLFreeDriver() - in fact it's undesired. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/testutilsxen.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/testutilsxen.c b/tests/testutilsxen.c index 75cd42ec43..b73c79581d 100644 --- a/tests/testutilsxen.c +++ b/tests/testutilsxen.c @@ -105,7 +105,6 @@ libxlDriverPrivatePtr testXLInitDriver(void) void testXLFreeDriver(libxlDriverPrivatePtr driver) { - virObjectUnref(driver->config->caps); virObjectUnref(driver->config); virObjectUnref(driver->xmlopt); virMutexDestroy(&driver->lock); -- 2.24.1
Fortunately, this is not causing any problems now because glib does this check for us when calling this function via attribute cleanup. But in future commit we will explicitly call this function over a struct member that might be NULL. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/capabilities.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 4fac59e6f7..a782d92956 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -184,6 +184,9 @@ virCapabilitiesFreeStoragePool(virCapsStoragePoolPtr pool) void virCapabilitiesHostNUMAUnref(virCapsHostNUMAPtr caps) { + if (!caps) + return; + if (g_atomic_int_dec_and_test(&caps->refs)) { g_ptr_array_unref(caps->cells); -- 2.24.1
On 12/18/19 5:37 AM, Michal Privoznik wrote:
Fortunately, this is not causing any problems now because glib does this check for us when calling this function via attribute cleanup. But in future commit we will explicitly call this
nit: "But in a future commit ..."
This function is supposed to clean up virQEMUDriver structure and free individual members. However, it's doing that in random order which makes it hard to track which members are being freed and which are not. Do the free in reverse order than the structure definition - assuming that the most important members (like mutex) are declared first and freed last. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 43 +++++++++++++++++------------------------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65c3be58d3..0b9e31b344 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1122,38 +1122,29 @@ qemuStateCleanup(void) if (!qemu_driver) return -1; - if (qemu_driver->lockFD != -1) - virPidFileRelease(qemu_driver->config->stateDir, "driver", qemu_driver->lockFD); - virThreadPoolFree(qemu_driver->workerPool); - virObjectUnref(qemu_driver->config); - virObjectUnref(qemu_driver->hostdevMgr); - virHashFree(qemu_driver->sharedDevices); - virObjectUnref(qemu_driver->caps); - virObjectUnref(qemu_driver->qemuCapsCache); - - virObjectUnref(qemu_driver->domains); - virPortAllocatorRangeFree(qemu_driver->remotePorts); - virPortAllocatorRangeFree(qemu_driver->webSocketPorts); - virPortAllocatorRangeFree(qemu_driver->migrationPorts); virObjectUnref(qemu_driver->migrationErrors); - - virObjectUnref(qemu_driver->xmlopt); - - virSysinfoDefFree(qemu_driver->hostsysinfo); - virObjectUnref(qemu_driver->closeCallbacks); - - VIR_FREE(qemu_driver->qemuImgBinary); - + virLockManagerPluginUnref(qemu_driver->lockManager); + virSysinfoDefFree(qemu_driver->hostsysinfo); + virPortAllocatorRangeFree(qemu_driver->migrationPorts); + virPortAllocatorRangeFree(qemu_driver->webSocketPorts); + virPortAllocatorRangeFree(qemu_driver->remotePorts); + virHashFree(qemu_driver->sharedDevices); + virObjectUnref(qemu_driver->hostdevMgr); virObjectUnref(qemu_driver->securityManager); - - ebtablesContextFree(qemu_driver->ebtables); - - /* Free domain callback list */ virObjectUnref(qemu_driver->domainEventState); + virObjectUnref(qemu_driver->qemuCapsCache); + virObjectUnref(qemu_driver->xmlopt); + virObjectUnref(qemu_driver->caps); + ebtablesContextFree(qemu_driver->ebtables); + VIR_FREE(qemu_driver->qemuImgBinary); + virObjectUnref(qemu_driver->domains); + virThreadPoolFree(qemu_driver->workerPool); - virLockManagerPluginUnref(qemu_driver->lockManager); + if (qemu_driver->lockFD != -1) + virPidFileRelease(qemu_driver->config->stateDir, "driver", qemu_driver->lockFD); + virObjectUnref(qemu_driver->config); virMutexDestroy(&qemu_driver->lock); VIR_FREE(qemu_driver); -- 2.24.1
Commit title has a typo: "Reoder" instead of "Reorder" On 12/18/19 5:37 AM, Michal Privoznik wrote:
This function is supposed to clean up virQEMUDriver structure and free individual members. However, it's doing that in random order which makes it hard to track which members are being freed and which are not. Do the free in reverse order than the structure definition - assuming that the most important members (like mutex) are declared first and freed last.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
When freeing qemu driver struct members, we forgot to free @hostcpu and @hostnuma members. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0b9e31b344..ec8faf384c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1135,6 +1135,8 @@ qemuStateCleanup(void) virObjectUnref(qemu_driver->domainEventState); virObjectUnref(qemu_driver->qemuCapsCache); virObjectUnref(qemu_driver->xmlopt); + virCPUDefFree(qemu_driver->hostcpu); + virCapabilitiesHostNUMAUnref(qemu_driver->hostnuma); virObjectUnref(qemu_driver->caps); ebtablesContextFree(qemu_driver->ebtables); VIR_FREE(qemu_driver->qemuImgBinary); -- 2.24.1
On 12/18/19 5:37 AM, Michal Privoznik wrote:
*** BLURB HERE ***
Michal Prívozník (5): domaincapstest: Don't leak cpu definitions testutilsxen: Avoid double free of driver caps virCapabilitiesHostNUMAUnref: Accept NULL qemu: Reoder cleanup in qemuStateCleanup() qemu: Don't leak hostcpu or hostnuma on driver cleanup
Just a couple of grammatical nits in 2 patches. For all patches: Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik