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(a)redhat.com&gt; > --- > 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