[PATCH 0/7] A few Coverity related issues
Fix some of the issues that have built up. Also a docs link fix that I tripped across at some point in time. Again as a reminder, I no longer have push access ;-) John Ferlan (7): util: Fix memory leak in virNetDevOpenvswitchInterfaceGetMaster util: Resolve resource leak in virExec error path docs: Fix link for virConnectGetStoragePoolCapabilities qemu: Fix resource leak and coding error in qemuDomainGetIOThreadsMon logging: Resolve mem leak in virLogDaemonPreExecRestart locking: Resolve mem leak in virLockDaemonPreExecRestart qemu: Fix some issues in virQEMUDriverConfigLoadNVRAMEntry docs/formatstoragecaps.html.in | 2 +- src/locking/lock_daemon.c | 7 +++---- src/logging/log_daemon.c | 7 +++---- src/qemu/qemu_conf.c | 5 ++--- src/qemu/qemu_driver.c | 19 +++++++++---------- src/util/vircommand.c | 2 ++ src/util/virnetdevopenvswitch.c | 2 +- 7 files changed, 21 insertions(+), 23 deletions(-) -- 2.28.0
Since 032548c4 @cmd was never autofree'd. Perhaps as a result of VIR_AUTOPTR type changes occurring at roughly the same time so the copy pasta missed this. Found by Coverity. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdevopenvswitch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index 7452527f49..d380b0cf22 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -428,7 +428,7 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname, int virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master) { - virCommandPtr cmd = virNetDevOpenvswitchCreateCmd(); + g_autoptr(virCommand) cmd = virNetDevOpenvswitchCreateCmd(); int exitstatus; *master = NULL; -- 2.28.0
On a Wednesday in 2020, John Ferlan wrote:
Since 032548c4 @cmd was never autofree'd. Perhaps as a result of VIR_AUTOPTR type changes occurring at roughly the same time so the copy pasta missed this.
Found by Coverity.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdevopenvswitch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On error, the @pidfilefd was not released Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/vircommand.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index e47dd6b932..1716225aeb 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -792,12 +792,14 @@ virExec(virCommandPtr cmd) if (virSetInherit(pidfilefd, true) < 0) { virReportSystemError(errno, "%s", _("Cannot disable close-on-exec flag")); + virPidFileReleasePath(cmd->pidfile, pidfilefd); goto fork_error; } c = '1'; if (safewrite(pipesync[1], &c, sizeof(c)) != sizeof(c)) { virReportSystemError(errno, "%s", _("Unable to notify child process")); + virPidFileReleasePath(cmd->pidfile, pidfilefd); goto fork_error; } VIR_FORCE_CLOSE(pipesync[0]); -- 2.28.0
On a Wednesday in 2020, John Ferlan wrote:
On error, the @pidfilefd was not released
Found by Coverity
The pidfile code was introduced by: commit d146105f1e4a9e0ab179f0b78c070ea38b9d5334 Author: Michal Prívozník <mprivozn@redhat.com> CommitDate: 2020-03-24 15:44:23 +0100 virCommand: Actually acquire pidfile instead of just writing it further changed by: commit be00118d5d9a3afb41e0edcddec823dff63a7ae1 Author: Marc-André Lureau <marcandre.lureau@redhat.com> CommitDate: 2020-03-25 09:04:49 +0100 util: keep the pidfile locked then some code movement added more error paths after the pidfile code: commit 0bb796bda31103ebf54eefc11c471586c87b95fd Author: Daniel Henrique Barboza <danielhb413@gmail.com> CommitDate: 2020-10-02 14:32:57 -0300 vircommand.c: write child pidfile before process tuning in virExec() IIUC the point of leaking the pidfilefd is to make sure the child holds a lock on the pidfile - so strictly speaking we should close it in all code paths that don't end up in a successful execv. Practically, this already happens because the fork_error calls _exit. Jano
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/vircommand.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c index e47dd6b932..1716225aeb 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -792,12 +792,14 @@ virExec(virCommandPtr cmd) if (virSetInherit(pidfilefd, true) < 0) { virReportSystemError(errno, "%s", _("Cannot disable close-on-exec flag")); + virPidFileReleasePath(cmd->pidfile, pidfilefd); goto fork_error; }
c = '1'; if (safewrite(pipesync[1], &c, sizeof(c)) != sizeof(c)) { virReportSystemError(errno, "%s", _("Unable to notify child process")); + virPidFileReleasePath(cmd->pidfile, pidfilefd); goto fork_error; } VIR_FORCE_CLOSE(pipesync[0]); -- 2.28.0
The API is in the storage family not the domain family Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstoragecaps.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/formatstoragecaps.html.in b/docs/formatstoragecaps.html.in index 900303aef7..a9ecc371fa 100644 --- a/docs/formatstoragecaps.html.in +++ b/docs/formatstoragecaps.html.in @@ -20,7 +20,7 @@ (<span class="since">Since 5.2.0</span>):</p> <pre> -<a href="/html/libvirt-libvirt-domain.html#virConnectGetStoragePoolCapabilities">virConnectGetStoragePoolCapabilities</a> +<a href="/html/libvirt-libvirt-storage.html#virConnectGetStoragePoolCapabilities">virConnectGetStoragePoolCapabilities</a> </pre> <p>The root element that emulator capability XML document starts with is -- 2.28.0
On a Wednesday in 2020, John Ferlan wrote:
The API is in the storage family not the domain family
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstoragecaps.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
If qemuDomainGetIOThreadsMon fails because qemuDomainObjExitMonitor fails, then a -1 is returned which overwrites @niothreads causing a memory leak. Let's pass @niothreads instead. Found by Coverity. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8eaa3ce68f..870159de47 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4972,17 +4972,16 @@ qemuDomainGetMaxVcpus(virDomainPtr dom) static int qemuDomainGetIOThreadsMon(virQEMUDriverPtr driver, virDomainObjPtr vm, - qemuMonitorIOThreadInfoPtr **iothreads) + qemuMonitorIOThreadInfoPtr **iothreads, + int *niothreads) { qemuDomainObjPrivatePtr priv = vm->privateData; - int niothreads = 0; qemuDomainObjEnterMonitor(driver, vm); - niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads); - if (qemuDomainObjExitMonitor(driver, vm) < 0 || niothreads < 0) + *niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads); + if (qemuDomainObjExitMonitor(driver, vm) < 0) return -1; - - return niothreads; + return 0; } @@ -5014,7 +5013,7 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver, goto endjob; } - if ((niothreads = qemuDomainGetIOThreadsMon(driver, vm, &iothreads)) < 0) + if (qemuDomainGetIOThreadsMon(driver, vm, &iothreads, &niothreads) < 0) goto endjob; /* Nothing to do */ @@ -18507,7 +18506,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = dom->privateData; size_t i; qemuMonitorIOThreadInfoPtr *iothreads = NULL; - int niothreads; + int niothreads = 0; int ret = -1; if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) @@ -18516,8 +18515,8 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) return 0; - if ((niothreads = qemuDomainGetIOThreadsMon(driver, dom, &iothreads)) < 0) - return -1; + if (qemuDomainGetIOThreadsMon(driver, dom, &iothreads, &niothreads) < 0) + goto cleanup; /* qemuDomainGetIOThreadsMon returns a NULL-terminated list, so we must free * it even if it returns 0 */ -- 2.28.0
The commit summary uses 'and' which makes me think there are two issues, but the commit message and code only seem to fix one. On a Wednesday in 2020, John Ferlan wrote:
If qemuDomainGetIOThreadsMon fails because qemuDomainObjExitMonitor fails, then a -1 is returned which overwrites @niothreads causing a memory leak. Let's pass @niothreads instead.
Found by Coverity.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8eaa3ce68f..870159de47 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4972,17 +4972,16 @@ qemuDomainGetMaxVcpus(virDomainPtr dom) static int qemuDomainGetIOThreadsMon(virQEMUDriverPtr driver, virDomainObjPtr vm, - qemuMonitorIOThreadInfoPtr **iothreads) + qemuMonitorIOThreadInfoPtr **iothreads, + int *niothreads)
Returning niothreads separately from success/error might clear things up,
{ qemuDomainObjPrivatePtr priv = vm->privateData; - int niothreads = 0;
qemuDomainObjEnterMonitor(driver, vm); - niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads); - if (qemuDomainObjExitMonitor(driver, vm) < 0 || niothreads < 0) + *niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
but qemuMonitorGetIOThreads can also return -1. In that case we should not: a) return 0 in this function b) compare the return value against unsigned size_t in the cleanup section of qemuDomainGetStatsIOThread
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
I think that now that we take a job even for destroying the domain in processMonitorEOFEvent, this should not happen. But if it still can, it would be more consistent if qemuDomainGetIOThreadsMon cleaned up after itself if it returns -1, instead of leaving it up to the caller. (Other callers of qemuMonitorGetIOThreads and qemuDomainGetIOThreadsMon seem to handle it properly and check if (iothreads) in their cleanup sections, it's only qemuDomainGetStatsIOThread that relies on niothreads being right) Jano
return -1; - - return niothreads; + return 0; }
@@ -5014,7 +5013,7 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver, goto endjob; }
- if ((niothreads = qemuDomainGetIOThreadsMon(driver, vm, &iothreads)) < 0) + if (qemuDomainGetIOThreadsMon(driver, vm, &iothreads, &niothreads) < 0) goto endjob;
/* Nothing to do */ @@ -18507,7 +18506,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = dom->privateData; size_t i; qemuMonitorIOThreadInfoPtr *iothreads = NULL; - int niothreads; + int niothreads = 0; int ret = -1;
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) @@ -18516,8 +18515,8 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) return 0;
- if ((niothreads = qemuDomainGetIOThreadsMon(driver, dom, &iothreads)) < 0) - return -1; + if (qemuDomainGetIOThreadsMon(driver, dom, &iothreads, &niothreads) < 0) + goto cleanup;
/* qemuDomainGetIOThreadsMon returns a NULL-terminated list, so we must free * it even if it returns 0 */ -- 2.28.0
On 12/2/20 9:44 AM, Ján Tomko wrote:
The commit summary uses 'and' which makes me think there are two issues, but the commit message and code only seem to fix one.
True - I changed my mind multiple times on this one w/r/t how involved the fix should be. Originally I did have cleanup added, but then changed my mind to allow the caller to do it and pass &niothreads instead. I can rework this one - thanks for the reviews/pushes. I'll also drop the pidfilefd one and keep it in my false positive list. Tks - John
On a Wednesday in 2020, John Ferlan wrote:
If qemuDomainGetIOThreadsMon fails because qemuDomainObjExitMonitor fails, then a -1 is returned which overwrites @niothreads causing a memory leak. Let's pass @niothreads instead.
Found by Coverity.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8eaa3ce68f..870159de47 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4972,17 +4972,16 @@ qemuDomainGetMaxVcpus(virDomainPtr dom) static int qemuDomainGetIOThreadsMon(virQEMUDriverPtr driver, virDomainObjPtr vm, - qemuMonitorIOThreadInfoPtr **iothreads) + qemuMonitorIOThreadInfoPtr **iothreads, + int *niothreads)
Returning niothreads separately from success/error might clear things up,
{ qemuDomainObjPrivatePtr priv = vm->privateData; - int niothreads = 0;
qemuDomainObjEnterMonitor(driver, vm); - niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads); - if (qemuDomainObjExitMonitor(driver, vm) < 0 || niothreads < 0) + *niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
but qemuMonitorGetIOThreads can also return -1. In that case we should not: a) return 0 in this function b) compare the return value against unsigned size_t in the cleanup section of qemuDomainGetStatsIOThread
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
I think that now that we take a job even for destroying the domain in processMonitorEOFEvent, this should not happen.
But if it still can, it would be more consistent if qemuDomainGetIOThreadsMon cleaned up after itself if it returns -1, instead of leaving it up to the caller.
(Other callers of qemuMonitorGetIOThreads and qemuDomainGetIOThreadsMon seem to handle it properly and check if (iothreads) in their cleanup sections, it's only qemuDomainGetStatsIOThread that relies on niothreads being right)
Jano
return -1; - - return niothreads; + return 0; }
@@ -5014,7 +5013,7 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver, goto endjob; }
- if ((niothreads = qemuDomainGetIOThreadsMon(driver, vm, &iothreads)) < 0) + if (qemuDomainGetIOThreadsMon(driver, vm, &iothreads, &niothreads) < 0) goto endjob;
/* Nothing to do */ @@ -18507,7 +18506,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = dom->privateData; size_t i; qemuMonitorIOThreadInfoPtr *iothreads = NULL; - int niothreads; + int niothreads = 0; int ret = -1;
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) @@ -18516,8 +18515,8 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver, if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) return 0;
- if ((niothreads = qemuDomainGetIOThreadsMon(driver, dom, &iothreads)) < 0) - return -1; + if (qemuDomainGetIOThreadsMon(driver, dom, &iothreads, &niothreads) < 0) + goto cleanup;
/* qemuDomainGetIOThreadsMon returns a NULL-terminated list, so we must free * it even if it returns 0 */ -- 2.28.0
Initialize and free @magic since virJSONValueObjectAppendString does not free it for us eventually. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/logging/log_daemon.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index be93c63eb5..6b8f3b6fe5 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -508,7 +508,7 @@ virLogDaemonPreExecRestart(const char *state_file, virJSONValuePtr child; char *state = NULL; virJSONValuePtr object = virJSONValueNewObject(); - char *magic; + char *magic = NULL; VIR_DEBUG("Running pre-restart exec"); @@ -523,10 +523,8 @@ virLogDaemonPreExecRestart(const char *state_file, if (!(magic = virLogDaemonGetExecRestartMagic())) goto cleanup; - if (virJSONValueObjectAppendString(object, "magic", magic) < 0) { - VIR_FREE(magic); + if (virJSONValueObjectAppendString(object, "magic", magic) < 0) goto cleanup; - } if (!(child = virLogHandlerPreExecRestart(logDaemon->handler))) goto cleanup; @@ -559,6 +557,7 @@ virLogDaemonPreExecRestart(const char *state_file, abort(); /* This should be impossible to reach */ cleanup: + VIR_FREE(magic); VIR_FREE(state); virJSONValueFree(object); return -1; -- 2.28.0
On a Wednesday in 2020, John Ferlan wrote:
Initialize and free @magic since virJSONValueObjectAppendString does not free it for us eventually.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/logging/log_daemon.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Initialize and free @magic since virJSONValueObjectAppendString does not free it for us eventually. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/locking/lock_daemon.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 57c7fb088f..851e9fc6f0 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -700,7 +700,7 @@ virLockDaemonPreExecRestart(const char *state_file, virJSONValuePtr child; char *state = NULL; virJSONValuePtr object = virJSONValueNewObject(); - char *magic; + char *magic = NULL; virHashKeyValuePairPtr pairs = NULL, tmp; virJSONValuePtr lockspaces; @@ -748,10 +748,8 @@ virLockDaemonPreExecRestart(const char *state_file, if (!(magic = virLockDaemonGetExecRestartMagic())) goto cleanup; - if (virJSONValueObjectAppendString(object, "magic", magic) < 0) { - VIR_FREE(magic); + if (virJSONValueObjectAppendString(object, "magic", magic) < 0) goto cleanup; - } if (!(state = virJSONValueToString(object, true))) goto cleanup; @@ -775,6 +773,7 @@ virLockDaemonPreExecRestart(const char *state_file, abort(); /* This should be impossible to reach */ cleanup: + VIR_FREE(magic); VIR_FREE(pairs); VIR_FREE(state); virJSONValueFree(object); -- 2.28.0
On a Wednesday in 2020, John Ferlan wrote:
Initialize and free @magic since virJSONValueObjectAppendString does not free it for us eventually.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/locking/lock_daemon.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Commit c4f4e195 fixed a double free, but if the code returns before we realloc the list and virFirmwareFreeList was called with cfg->nfirmwares
0 (e.g. during virQEMUDriverConfigDispose), then it would be rather disasterous. So let's reinitialze that too to indicate the list is empty.
Coverity pointed out that using nvram[0] as a guard to reallocating the list could lead to a possible NULL deref. While nvram[0] may always be true in this case, if it wasn't then the subsequent for loop would fail. Just reallocate always regardless - even if nfirmwares == 0 as virFirmwareFreeList will free it for us anyway. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index cbdde0c0dc..690cfd39f9 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -835,6 +835,7 @@ virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr cfg, virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares); cfg->firmwares = NULL; + cfg->nfirmwares = 0; if (qemuFirmwareFetchConfigs(&fwList, privileged) < 0) return -1; @@ -843,13 +844,11 @@ virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr cfg, VIR_WARN("Obsolete nvram variable is set while firmware metadata " "files found. Note that the nvram config file variable is " "going to be ignored."); - cfg->nfirmwares = 0; return 0; } cfg->nfirmwares = virStringListLength((const char *const *)nvram); - if (nvram[0]) - cfg->firmwares = g_new0(virFirmwarePtr, cfg->nfirmwares); + cfg->firmwares = g_new0(virFirmwarePtr, cfg->nfirmwares); for (i = 0; nvram[i] != NULL; i++) { cfg->firmwares[i] = g_new0(virFirmware, 1); -- 2.28.0
On a Wednesday in 2020, John Ferlan wrote:
Commit c4f4e195 fixed a double free, but if the code returns before we realloc the list and virFirmwareFreeList was called with cfg->nfirmwares
0 (e.g. during virQEMUDriverConfigDispose), then it would be rather disasterous. So let's reinitialze that too to indicate the list is empty.
*disastrous *reinitialize
Coverity pointed out that using nvram[0] as a guard to reallocating the list could lead to a possible NULL deref. While nvram[0] may always be true in this case, if it wasn't then the subsequent for loop would fail. Just reallocate always regardless - even if nfirmwares == 0 as virFirmwareFreeList will free it for us anyway.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
John Ferlan -
Ján Tomko