[libvirt] [PATCH 0/3] Various Valgrind fixes
Valgrind reported a couple of memory leaks and jumps conditional on uninitialized values. Happy New Year! Michael Chapman (3): qemu: do not copy out non-existent block job info qemu: do not leak NBD disk data in migration cookie storage: do not leak storage pool XML filename src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_migration.c | 5 +++-- src/storage/storage_driver.c | 10 +++++----- 3 files changed, 10 insertions(+), 9 deletions(-) -- 2.4.3
Valgrind complained: ==23975== Conditional jump or move depends on uninitialised value(s) ==23975== at 0x22255FA6: qemuDomainGetBlockJobInfo (qemu_driver.c:16538) ==23975== by 0x538E97C: virDomainGetBlockJobInfo (libvirt-domain.c:9685) ==23975== by 0x12F740: remoteDispatchDomainGetBlockJobInfoHelper (remote.c:2834) ==23975== by 0x53FF287: virNetServerProgramDispatch (virnetserverprogram.c:437) ==23975== by 0x540523D: virNetServerProcessMsg (virnetserver.c:135) ==23975== by 0x54052C7: virNetServerHandleJob (virnetserver.c:156) ==23975== by 0x52F515B: virThreadPoolWorker (virthreadpool.c:145) ==23975== by 0x52F4668: virThreadHelper (virthread.c:206) ==23975== by 0x6E08A50: start_thread (in /lib64/libpthread-2.12.so) ==23975== by 0x82BE93C: clone (in /lib64/libc-2.12.so) ==23975== ==23975== Conditional jump or move depends on uninitialised value(s) ==23975== at 0x22255FB4: qemuDomainGetBlockJobInfo (qemu_driver.c:16542) ==23975== by 0x538E97C: virDomainGetBlockJobInfo (libvirt-domain.c:9685) ==23975== by 0x12F740: remoteDispatchDomainGetBlockJobInfoHelper (remote.c:2834) ==23975== by 0x53FF287: virNetServerProgramDispatch (virnetserverprogram.c:437) ==23975== by 0x540523D: virNetServerProcessMsg (virnetserver.c:135) ==23975== by 0x54052C7: virNetServerHandleJob (virnetserver.c:156) ==23975== by 0x52F515B: virThreadPoolWorker (virthreadpool.c:145) ==23975== by 0x52F4668: virThreadHelper (virthread.c:206) ==23975== by 0x6E08A50: start_thread (in /lib64/libpthread-2.12.so) ==23975== by 0x82BE93C: clone (in /lib64/libc-2.12.so) If no matching block job is found, qemuMonitorGetBlockJobInfo returns 0 and we should not write anything to the caller-supplied virDomainBlockJobInfo pointer. Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/qemu/qemu_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e8ba3a6..304165c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16527,7 +16527,7 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, disk->info.alias, &rawInfo); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; - if (ret < 0) + if (ret <= 0) goto endjob; info->cur = rawInfo.cur; @@ -16554,7 +16554,7 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, * we can ignore failure because it is only an optimization. We * hold the vm lock, so modifying the in-memory representation is * safe, even if we are a query rather than a modify job. */ - if (ret == 1 && disk->mirror && + if (disk->mirror && rawInfo.ready != 0 && info->cur == info->end && !disk->mirrorState) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); -- 2.4.3
Valgrind complained: ==18990== 20 (16 direct, 4 indirect) bytes in 1 blocks are definitely lost in loss record 188 of 996 ==18990== at 0x4A057BB: calloc (vg_replace_malloc.c:593) ==18990== by 0x5292E9B: virAllocN (viralloc.c:191) ==18990== by 0x2221E731: qemuMigrationCookieXMLParseStr (qemu_migration.c:1012) ==18990== by 0x2221F390: qemuMigrationEatCookie (qemu_migration.c:1413) ==18990== by 0x222228CE: qemuMigrationPrepareAny (qemu_migration.c:3463) ==18990== by 0x22224121: qemuMigrationPrepareDirect (qemu_migration.c:3865) ==18990== by 0x22251C25: qemuDomainMigratePrepare3Params (qemu_driver.c:12414) ==18990== by 0x5389EE0: virDomainMigratePrepare3Params (libvirt-domain.c:5107) ==18990== by 0x1278DB: remoteDispatchDomainMigratePrepare3ParamsHelper (remote.c:5425) ==18990== by 0x53FF287: virNetServerProgramDispatch (virnetserverprogram.c:437) ==18990== by 0x540523D: virNetServerProcessMsg (virnetserver.c:135) ==18990== by 0x54052C7: virNetServerHandleJob (virnetserver.c:156) ==18990== ==18990== 20 (16 direct, 4 indirect) bytes in 1 blocks are definitely lost in loss record 189 of 996 ==18990== at 0x4A057BB: calloc (vg_replace_malloc.c:593) ==18990== by 0x5292E9B: virAllocN (viralloc.c:191) ==18990== by 0x2221E731: qemuMigrationCookieXMLParseStr (qemu_migration.c:1012) ==18990== by 0x2221F390: qemuMigrationEatCookie (qemu_migration.c:1413) ==18990== by 0x222249D2: qemuMigrationRun (qemu_migration.c:4395) ==18990== by 0x22226365: doNativeMigrate (qemu_migration.c:4693) ==18990== by 0x22228E45: qemuMigrationPerform (qemu_migration.c:5553) ==18990== by 0x2225144B: qemuDomainMigratePerform3Params (qemu_driver.c:12621) ==18990== by 0x539F5D8: virDomainMigratePerform3Params (libvirt-domain.c:5206) ==18990== by 0x127305: remoteDispatchDomainMigratePerform3ParamsHelper (remote.c:5557) ==18990== by 0x53FF287: virNetServerProgramDispatch (virnetserverprogram.c:437) ==18990== by 0x540523D: virNetServerProcessMsg (virnetserver.c:135) If we're replacing the NBD data, it's simplest to free the old object (including the disk list) and allocate a new one. Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/qemu/qemu_migration.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4519aef..bb708a3 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -571,8 +571,9 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, int ret = -1, rc; /* It is not a bug if there already is a NBD data */ - if (!mig->nbd && - VIR_ALLOC(mig->nbd) < 0) + qemuMigrationCookieNBDFree(mig->nbd); + + if (VIR_ALLOC(mig->nbd) < 0) return -1; if (vm->def->ndisks && -- 2.4.3
Valgrind complained: ==28277== 38 bytes in 1 blocks are definitely lost in loss record 298 of 957 ==28277== at 0x4A06A2E: malloc (vg_replace_malloc.c:270) ==28277== by 0x82D7F57: __vasprintf_chk (in /lib64/libc-2.12.so) ==28277== by 0x52EF16A: virVasprintfInternal (stdio2.h:199) ==28277== by 0x52EF25C: virAsprintfInternal (virstring.c:514) ==28277== by 0x52B1FA9: virFileBuildPath (virfile.c:2831) ==28277== by 0x19B1947C: storageDriverAutostart (storage_driver.c:191) ==28277== by 0x19B196A7: storageStateAutoStart (storage_driver.c:307) ==28277== by 0x538527E: virStateInitialize (libvirt.c:793) ==28277== by 0x11D7CF: daemonRunStateInit (libvirtd.c:947) ==28277== by 0x52F4694: virThreadHelper (virthread.c:206) ==28277== by 0x6E08A50: start_thread (in /lib64/libpthread-2.12.so) ==28277== by 0x82BE93C: clone (in /lib64/libc-2.12.so) Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/storage/storage_driver.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 0bb577f..c8f259e 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -151,7 +151,6 @@ static void storageDriverAutostart(void) { size_t i; - char *stateFile = NULL; virConnectPtr conn = NULL; /* XXX Remove hardcoding of QEMU URI */ @@ -187,6 +186,8 @@ storageDriverAutostart(void) } if (started) { + char *stateFile; + virStoragePoolObjClearVols(pool); stateFile = virFileBuildPath(driver->stateDir, pool->def->name, ".xml"); @@ -201,11 +202,10 @@ storageDriverAutostart(void) VIR_ERROR(_("Failed to autostart storage pool '%s': %s"), pool->def->name, err ? err->message : _("no error message found")); - VIR_FREE(stateFile); - virStoragePoolObjUnlock(pool); - continue; + } else { + pool->active = true; } - pool->active = true; + VIR_FREE(stateFile); } virStoragePoolObjUnlock(pool); } -- 2.4.3
On 31.12.2015 07:04, Michael Chapman wrote:
Valgrind reported a couple of memory leaks and jumps conditional on uninitialized values.
Happy New Year!
To you too!
Michael Chapman (3): qemu: do not copy out non-existent block job info qemu: do not leak NBD disk data in migration cookie storage: do not leak storage pool XML filename
src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_migration.c | 5 +++-- src/storage/storage_driver.c | 10 +++++----- 3 files changed, 10 insertions(+), 9 deletions(-)
ACKed and pushed. Michal
On Mon, 2016-01-04 at 15:03 +0100, Michal Privoznik wrote:
Michael Chapman (3): qemu: do not copy out non-existent block job info qemu: do not leak NBD disk data in migration cookie storage: do not leak storage pool XML filename
src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_migration.c | 5 +++-- src/storage/storage_driver.c | 10 +++++----- 3 files changed, 10 insertions(+), 9 deletions(-)
ACKed and pushed.
I just sent out a follow-up to patch 1/3: https://www.redhat.com/archives/libvir-list/2016-January/msg00026.html Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
participants (3)
-
Andrea Bolognani -
Michael Chapman -
Michal Privoznik