Re: [libvirt] [PATCH] qemu: fix a bug in numatune (was Re: [PATCH] qemu: Prevent crash of libvirtd when setting numa parameters)

On Wed, Jan 04, 2012 at 05:15:24PM +0800, Alex Jia wrote: > On 01/04/2012 05:04 PM, Hu Tao wrote: >> On Wed, Jan 04, 2012 at 03:53:19PM +0800, ajia(a)redhat.com wrote: >>> From: Alex Jia&lt;ajia(a)redhat.com&gt; >>> >>> It's a NULL pointer deref issue, which leads to libvirtd crash. This patch >>> directly use 'params[i].value.s' value instead of derefing a NULL pointer >>> on memcpy. >>> >>> * how to reproduce? >>> % virsh numatune<domain> --nodeset 0 >> The domain must have no nodeset set previously (to crash in this example). >> >>> % service libvirtd status >>> >>> * src/qemu/qemu_driver.c (qemuDomainSetNumaParameters): avoid a NULL pointer deref. >>> >>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=771562 >>> >>> Signed-off-by: Alex Jia&lt;ajia(a)redhat.com&gt; >>> --- >>> src/qemu/qemu_driver.c | 6 ++---- >>> 1 files changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >>> index 82bab67..1bd93f6 100644 >>> --- a/src/qemu/qemu_driver.c >>> +++ b/src/qemu/qemu_driver.c >>> @@ -6721,14 +6721,12 @@ qemuDomainSetNumaParameters(virDomainPtr dom, >>> } >>> >>> if (flags& VIR_DOMAIN_AFFECT_CONFIG) { >>> - memcpy(oldnodemask, persistentDef->numatune.memory.nodemask, >>> - VIR_DOMAIN_CPUMASK_LEN); >>> + memcpy(oldnodemask, params[i].value.s, VIR_DOMAIN_CPUMASK_LEN); >>> if (virDomainCpuSetParse(params[i].value.s, >>> 0, >>> persistentDef->numatune.memory.nodemask, >> Not correct. In this case persistentDef->numatune.memory.nodemask is >> null, and virDomainCpuSetParse will always fail, thus the nodeset will >> never be set. > In fact, I can successfully set nodeset value: > > # virsh numatune foo --nodeset 0-1 > > # virsh numatune foo > numa_mode : strict > numa_nodeset : 0-1 Weird. I've never succeeded with your patch. Can you double-check again?

>>> VIR_DOMAIN_CPUMASK_LEN)< 0) { >>> - memcpy(persistentDef->numatune.memory.nodemask, >>> - oldnodemask, VIR_DOMAIN_CPUMASK_LEN); >>> + memcpy(params[i].value.s, oldnodemask, VIR_DOMAIN_CPUMASK_LEN); >>> ret = -1; >>> continue; >>> } >> From f2fe7c7d0041779895330416dca6ac97fcbec88a Mon Sep 17 00:00:00 2001 >> From: Hu Tao&lt;hutao(a)cn.fujitsu.com&gt; >> Date: Wed, 4 Jan 2012 17:00:27 +0800 >> Subject: [PATCH] qemu: fix a bug in numatune >> >> When setting numa nodeset for a domain which has no nodeset set >> before, libvirtd crashes by dereferring the pointer to the old >> nodemask which is null in the case. >> --- >> src/qemu/qemu_driver.c | 22 ++++++++++++++++++---- >> 1 files changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index 82bab67..37330b4 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -6721,14 +6721,28 @@ qemuDomainSetNumaParameters(virDomainPtr dom, >> } >> >> if (flags& VIR_DOMAIN_AFFECT_CONFIG) { >> - memcpy(oldnodemask, persistentDef->numatune.memory.nodemask, >> - VIR_DOMAIN_CPUMASK_LEN); >> + bool savedmask = false; >> + if (!persistentDef->numatune.memory.nodemask) { >> + if (VIR_ALLOC_N(persistentDef->numatune.memory.nodemask, >> + VIR_DOMAIN_CPUMASK_LEN)< 0) { >> + virReportOOMError(); >> + ret = -1; >> + goto cleanup; >> + } >> + } else { >> + memcpy(oldnodemask, persistentDef->numatune.memory.nodemask, >> + VIR_DOMAIN_CPUMASK_LEN); >> + savedmask = true; >> + } >> if (virDomainCpuSetParse(params[i].value.s, >> 0, >> persistentDef->numatune.memory.nodemask, >> VIR_DOMAIN_CPUMASK_LEN)< 0) { >> - memcpy(persistentDef->numatune.memory.nodemask, >> - oldnodemask, VIR_DOMAIN_CPUMASK_LEN); >> + if (savedmask) >> + memcpy(persistentDef->numatune.memory.nodemask, >> + oldnodemask, VIR_DOMAIN_CPUMASK_LEN); >> + else >> + VIR_FREE(persistentDef->numatune.memory.nodemask); Oops. The same should be done when --live. >> ret = -1; >> continue; >> }