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

[Alex Jia]

On 01/04/2012 05:28 PM, Hu Tao wrote:
> 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?
Hu Tao, Indeed, it's weird. the patch always works well for me:

# for i in $(seq 10); do virsh numatune foo --nodeset 0-$i; virsh 
numatune foo; done

numa_mode      : strict
numa_nodeset   : 0-1


numa_mode      : strict
numa_nodeset   : 0-2


numa_mode      : strict
numa_nodeset   : 0-3


numa_mode      : strict
numa_nodeset   : 0-4


numa_mode      : strict
numa_nodeset   : 0-5


numa_mode      : strict
numa_nodeset   : 0-6


numa_mode      : strict
numa_nodeset   : 0-7


numa_mode      : strict
numa_nodeset   : 0-8


numa_mode      : strict
numa_nodeset   : 0-9


numa_mode      : strict
numa_nodeset   : 0-10

>>>>                                            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;
>>>                   }