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

[Erik Skultety]

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.

> +    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.

Erik