1:43 p.m.
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
1:43 p.m.
New subject: [PATCH 1/7] util: Fix memory leak in virNetDevOpenvswitchInterfaceGetMaster
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(a)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
2:51 p.m.
New subject: [PATCH 1/7] util: Fix memory leak in virNetDevOpenvswitchInterfaceGetMaster
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(a)redhat.com>
---
src/util/virnetdevopenvswitch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1:43 p.m.
New subject: [PATCH 2/7] util: Resolve resource leak in virExec error path
On error, the @pidfilefd was not released
Found by Coverity
Signed-off-by: John Ferlan <jferlan(a)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
3:06 p.m.
New subject: [PATCH 2/7] util: Resolve resource leak in virExec error path
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(a)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(a)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(a)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(a)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
1:43 p.m.
New subject: [PATCH 3/7] docs: Fix link for virConnectGetStoragePoolCapabilities
The API is in the storage family not the domain family
Signed-off-by: John Ferlan <jferlan(a)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
3:07 p.m.
New subject: [PATCH 3/7] docs: Fix link for virConnectGetStoragePoolCapabilities
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(a)redhat.com>
---
docs/formatstoragecaps.html.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1:43 p.m.
New subject: [PATCH 4/7] qemu: Fix resource leak and coding error in qemuDomainGetIOThreadsMon
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(a)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
3:44 p.m.
New subject: [PATCH 4/7] qemu: Fix resource leak and coding error in qemuDomainGetIOThreadsMon
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(a)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
5:20 p.m.
New subject: [PATCH 4/7] qemu: Fix resource leak and coding error in qemuDomainGetIOThreadsMon
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(a)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
>
1:43 p.m.
New subject: [PATCH 5/7] logging: Resolve mem leak in virLogDaemonPreExecRestart
Initialize and free @magic since virJSONValueObjectAppendString
does not free it for us eventually.
Signed-off-by: John Ferlan <jferlan(a)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
3:50 p.m.
New subject: [PATCH 5/7] logging: Resolve mem leak in virLogDaemonPreExecRestart
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(a)redhat.com>
---
src/logging/log_daemon.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1:43 p.m.
New subject: [PATCH 6/7] locking: Resolve mem leak in virLockDaemonPreExecRestart
Initialize and free @magic since virJSONValueObjectAppendString
does not free it for us eventually.
Signed-off-by: John Ferlan <jferlan(a)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
3:50 p.m.
New subject: [PATCH 6/7] locking: Resolve mem leak in virLockDaemonPreExecRestart
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(a)redhat.com>
---
src/locking/lock_daemon.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1:43 p.m.
New subject: [PATCH 7/7] qemu: Fix some issues in virQEMUDriverConfigLoadNVRAMEntry
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(a)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
4:08 p.m.
New subject: [PATCH 7/7] qemu: Fix some issues in virQEMUDriverConfigLoadNVRAMEntry
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(a)redhat.com>
---
src/qemu/qemu_conf.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1384
days inactive
1384
days old
15 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko