7:46 a.m.
Spotted by John through Coverity.
Andrea Bolognani (2):
qemu: Fix leak in qemuProcessInitCpuAffinity()
qemu: Drop cleanup label from qemuProcessInitCpuAffinity()
src/qemu/qemu_process.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
--
2.21.0
7:46 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Fix leak in qemuProcessInitCpuAffinity()
In two out of three scenarios we were cleaning up properly
after ourselves, but in the remaining one we were leaking
cpumapToSet.
Refactor the logic so that cpumapToSet is always a freshly
allocated bitmap that gets cleaned up automatically thanks
to VIR_AUTOPTR(); this also allows us to remove hostcpumap.
Reported-by: John Ferlan <jferlan(a)redhat.com>
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_process.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d44076288e..7d48c95973 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2464,8 +2464,7 @@ static int
qemuProcessInitCpuAffinity(virDomainObjPtr vm)
{
int ret = -1;
- virBitmapPtr cpumapToSet = NULL;
- VIR_AUTOPTR(virBitmap) hostcpumap = NULL;
+ VIR_AUTOPTR(virBitmap) cpumapToSet = NULL;
virDomainNumatuneMemMode mem_mode;
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -2500,11 +2499,11 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm)
if (virNumaNodesetToCPUset(nodeset, &cpumapToSet) < 0)
goto cleanup;
} else if (vm->def->cputune.emulatorpin) {
- cpumapToSet = vm->def->cputune.emulatorpin;
+ if (virBitmapCopy(cpumapToSet, vm->def->cputune.emulatorpin) < 0)
+ goto cleanup;
} else {
- if (qemuProcessGetAllCpuAffinity(&hostcpumap) < 0)
+ if (qemuProcessGetAllCpuAffinity(&cpumapToSet) < 0)
goto cleanup;
- cpumapToSet = hostcpumap;
}
if (cpumapToSet &&
--
2.21.0
8:37 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Fix leak in qemuProcessInitCpuAffinity()
On Tue, Jun 04, 2019 at 02:46:46PM +0200, Andrea Bolognani wrote:
In two out of three scenarios we were cleaning up properly
after ourselves, but in the remaining one we were leaking
cpumapToSet.
Given that the leak was introduced recently by commit 5f2212c, not mentioning
it here seems misleading.
Refactor the logic so that cpumapToSet is always a freshly
allocated bitmap that gets cleaned up automatically thanks
to VIR_AUTOPTR(); this also allows us to remove hostcpumap.
Reported-by: John Ferlan <jferlan(a)redhat.com>
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_process.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:50 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Fix leak in qemuProcessInitCpuAffinity()
On Tue, 2019-06-04 at 15:37 +0200, Ján Tomko wrote:
On Tue, Jun 04, 2019 at 02:46:46PM +0200, Andrea Bolognani wrote:
> In two out of three scenarios we were cleaning up properly
> after ourselves, but in the remaining one we were leaking
> cpumapToSet.
Given that the leak was introduced recently by commit 5f2212c, not mentioning
it here seems misleading.
Good point, I'll add such a mention before pushing.
Thanks for the review :)
--
Andrea Bolognani / Red Hat / Virtualization
5:44 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Fix leak in qemuProcessInitCpuAffinity()
On 6/4/19 8:46 AM, Andrea Bolognani wrote:
In two out of three scenarios we were cleaning up properly
after ourselves, but in the remaining one we were leaking
cpumapToSet.
Refactor the logic so that cpumapToSet is always a freshly
allocated bitmap that gets cleaned up automatically thanks
to VIR_AUTOPTR(); this also allows us to remove hostcpumap.
Reported-by: John Ferlan <jferlan(a)redhat.com>
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_process.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d44076288e..7d48c95973 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2464,8 +2464,7 @@ static int
qemuProcessInitCpuAffinity(virDomainObjPtr vm)
{
int ret = -1;
- virBitmapPtr cpumapToSet = NULL;
- VIR_AUTOPTR(virBitmap) hostcpumap = NULL;
+ VIR_AUTOPTR(virBitmap) cpumapToSet = NULL;
virDomainNumatuneMemMode mem_mode;
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -2500,11 +2499,11 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm)
if (virNumaNodesetToCPUset(nodeset, &cpumapToSet) < 0)
goto cleanup;
} else if (vm->def->cputune.emulatorpin) {
- cpumapToSet = vm->def->cputune.emulatorpin;
+ if (virBitmapCopy(cpumapToSet, vm->def->cputune.emulatorpin) < 0)
Now Coverity is unhappy for another reason. What happens to the NULL
cpumapToSet when calling virBitmapCopy?
Should have been virBitmapNewCopy
John
(sorry didn't get a chance to look at the patch when first posted)
+ goto cleanup;
} else {
- if (qemuProcessGetAllCpuAffinity(&hostcpumap) < 0)
+ if (qemuProcessGetAllCpuAffinity(&cpumapToSet) < 0)
goto cleanup;
- cpumapToSet = hostcpumap;
}
if (cpumapToSet &&
8:46 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Fix leak in qemuProcessInitCpuAffinity()
On Thu, 2019-06-06 at 06:44 -0400, John Ferlan wrote:
On 6/4/19 8:46 AM, Andrea Bolognani wrote:
> } else if (vm->def->cputune.emulatorpin) {
> - cpumapToSet = vm->def->cputune.emulatorpin;
> + if (virBitmapCopy(cpumapToSet, vm->def->cputune.emulatorpin) < 0)
Now Coverity is unhappy for another reason. What happens to the NULL
cpumapToSet when calling virBitmapCopy?
Should have been virBitmapNewCopy
Geez, so embarrassing :/
I just posted a fix:
https://www.redhat.com/archives/libvir-list/2019-June/msg00171.html
(sorry didn't get a chance to look at the patch when first
posted)
No need to apologize, but if you really feel like you need to make
up for it reviewing the above will do ;)
--
Andrea Bolognani / Red Hat / Virtualization
7:46 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Drop cleanup label from qemuProcessInitCpuAffinity()
We're using VIR_AUTOPTR() for everything now, plus the
cleanup section was not doing anything useful anyway.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_process.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7d48c95973..42a6271411 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2463,7 +2463,6 @@ qemuProcessGetAllCpuAffinity(virBitmapPtr *cpumapRet)
static int
qemuProcessInitCpuAffinity(virDomainObjPtr vm)
{
- int ret = -1;
VIR_AUTOPTR(virBitmap) cpumapToSet = NULL;
virDomainNumatuneMemMode mem_mode;
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -2494,25 +2493,24 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm)
priv->autoNodeset,
&nodeset,
-1) < 0)
- goto cleanup;
+ return -1;
if (virNumaNodesetToCPUset(nodeset, &cpumapToSet) < 0)
- goto cleanup;
+ return -1;
} else if (vm->def->cputune.emulatorpin) {
if (virBitmapCopy(cpumapToSet, vm->def->cputune.emulatorpin) < 0)
- goto cleanup;
+ return -1;
} else {
if (qemuProcessGetAllCpuAffinity(&cpumapToSet) < 0)
- goto cleanup;
+ return -1;
}
if (cpumapToSet &&
- virProcessSetAffinity(vm->pid, cpumapToSet) < 0)
- goto cleanup;
+ virProcessSetAffinity(vm->pid, cpumapToSet) < 0) {
+ return -1;
+ }
- ret = 0;
- cleanup:
- return ret;
+ return 0;
}
#else /* !defined(HAVE_SCHED_GETAFFINITY) && !defined(HAVE_BSD_CPU_AFFINITY) */
static int
--
2.21.0
8:38 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Drop cleanup label from qemuProcessInitCpuAffinity()
On Tue, Jun 04, 2019 at 02:46:47PM +0200, Andrea Bolognani wrote:
We're using VIR_AUTOPTR() for everything now, plus the
cleanup section was not doing anything useful anyway.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_process.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1996
days inactive
1998
days old
7 comments
3 participants
participants (3)
-
Andrea Bolognani -
John Ferlan -
Ján Tomko