[libvirt] [PATCH 0/2] qemu: Fix leak in qemuProcessInitCpuAffinity()
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
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@redhat.com> Signed-off-by: Andrea Bolognani <abologna@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
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@redhat.com> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_process.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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
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@redhat.com> Signed-off-by: Andrea Bolognani <abologna@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 &&
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
We're using VIR_AUTOPTR() for everything now, plus the cleanup section was not doing anything useful anyway. Signed-off-by: Andrea Bolognani <abologna@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
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@redhat.com> --- src/qemu/qemu_process.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Andrea Bolognani -
John Ferlan -
Ján Tomko