[libvirt] [PATCH] conf: fix crash when parse a invalid vcpus in virDomainThreadSchedParse()

[Luyao Huang]

On 04/10/2015 06:43 PM, Erik Skultety wrote:
>
> On 04/09/2015 09:39 AM, Luyao Huang wrote:
>> When we set a invalid vcpus in /domain/cputune/vcpusched, like this:
>>
>> <vcpusched vcpus='0,^0' scheduler='fifo' priority='1'/>
>>
>> libvirtd will get segfault, because we will free bitmap (here is sp->ids) we
>> passed in virBitmapParse() then return -1, then call virBitmapIsAllClear() with
>> a null pointer bitmap will crash libvirtd.
>>
>> Split this check in two part to report parse error when cannot parse vcpus,
>> and report invalid argument error when the cpu number is not correct.
>>
>>   virBitmapIsAllClear (bitmap=0x0) at util/virbitmap.c:649
>>   649            for (i = 0; i < bitmap->map_len; i++)
>>   (gdb) bt
>>   #0  virBitmapIsAllClear (bitmap=0x0) at util/virbitmap.c:649
>>   #1  0x00007f28322f1245 in virDomainThreadSchedParse at conf/domain_conf.c:13464
>>   #2  0x00007f283238aec8 in virDomainDefParseXML  at conf/domain_conf.c:14059
>>   #3  0x00007f2832390ddb in virDomainDefParseNode at conf/domain_conf.c:15588
>>   #4  0x00007f2832390f02 in virDomainDefParse at conf/domain_conf.c:15530
>>   #5  0x00007f2832390f53 in virDomainDefParseString at conf/domain_conf.c:15546
>>   #6  0x00007f2819d8e8de in qemuDomainDefineXMLFlags  at qemu/qemu_driver.c:7385
>>
>> Signed-off-by: Luyao Huang <lhuang at redhat.com>
>> ---
>>   src/conf/domain_conf.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 1763305..46dfea1 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -13473,12 +13473,11 @@ virDomainThreadSchedParse(xmlNodePtr node,
>>           goto error;
>>       }
>>   
>> -    if (!virBitmapParse(tmp, 0, &sp->ids,
>> -                        VIR_DOMAIN_CPUMASK_LEN) ||
>> -        virBitmapIsAllClear(sp->ids) ||
> This line ^ should be left as is, see below.
>> -        virBitmapNextSetBit(sp->ids, -1) < minid ||
>> -        virBitmapLastSetBit(sp->ids) > maxid) {
>> +    if (virBitmapParse(tmp, 0, &sp->ids, VIR_DOMAIN_CPUMASK_LEN) < 0)
>> +        goto error;
> Although this line is correct, the problem resides in the virBitmapParse
> function itself. The virBitmapIsAllClear check should be removed from
> there and placed to every relevant place after the virBitmapParse call.

Oh, i see, reasonable to me, thanks for pointing out this :)

>
>> +    if (virBitmapNextSetBit(sp->ids, -1) < minid ||
>> +        virBitmapLastSetBit(sp->ids) > maxid) {
>>           virReportError(VIR_ERR_XML_ERROR,
>>                          _("Invalid value of '%s': %s"),
>>                          name, tmp);
>>
> I sent 2 patches to the list, first adding the checks to
> relevant spots and the second partially reverting commit 983f5a
> which broke the virBitmapParse logic.

Okay, so no need I write a new version :)

Thanks for your review

> Erik

Luyao