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

[Hu Tao]

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 at redhat.com wrote:
> >>From: Alex Jia<ajia at redhat.com>
> >>
> >>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<ajia at redhat.com>
> >>---
> >>  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<hutao at cn.fujitsu.com>
> >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;
> >                  }

-- 
Thanks,
Hu Tao