[libvirt] [PATCH 1/2] virBitmap: Place virBitmapIsAllClear check after virBitmapParse calls
This patch adds checks for empty bitmaps right after the calls of virBitmapParse. These only include spots where set API's are called and where domain's XML is parsed. https://bugzilla.redhat.com/show_bug.cgi?id=1210545 --- src/conf/domain_conf.c | 35 +++++++++++++++++++++++++++++++---- src/conf/numa_conf.c | 23 +++++++++++++++++++---- src/qemu/qemu_driver.c | 5 +++-- src/xenconfig/xen_sxpr.c | 7 +++++++ 4 files changed, 60 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1763305..c9488cf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11563,6 +11563,12 @@ virDomainMemorySourceDefParseXML(xmlNodePtr node, if (virBitmapParse(nodemask, 0, &def->sourceNodes, VIR_DOMAIN_CPUMASK_LEN) < 0) goto cleanup; + + if (virBitmapIsAllClear(def->sourceNodes)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid value of 'nodemask': %s"), nodemask); + goto cleanup; + } } ret = 0; @@ -13251,6 +13257,13 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node, if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) goto error; + if (virBitmapIsAllClear(def->cpumask)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid value of 'cpuset': %s"), + tmp); + goto error; + } + cleanup: VIR_FREE(tmp); ctxt->node = oldnode; @@ -13352,6 +13365,12 @@ virDomainHugepagesParseXML(xmlNodePtr node, if (virBitmapParse(nodeset, 0, &hugepage->nodemask, VIR_DOMAIN_CPUMASK_LEN) < 0) goto cleanup; + + if (virBitmapIsAllClear(hugepage->nodemask)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid value of 'nodeset': %s"), nodeset); + goto cleanup; + } } ret = 0; @@ -13473,13 +13492,14 @@ virDomainThreadSchedParse(xmlNodePtr node, goto error; } - if (!virBitmapParse(tmp, 0, &sp->ids, - VIR_DOMAIN_CPUMASK_LEN) || - virBitmapIsAllClear(sp->ids) || + if (virBitmapParse(tmp, 0, &sp->ids, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto error; + + if (virBitmapIsAllClear(sp->ids) || virBitmapNextSetBit(sp->ids, -1) < minid || virBitmapLastSetBit(sp->ids) > maxid) { - virReportError(VIR_ERR_XML_ERROR, + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid value of '%s': %s"), name, tmp); goto error; @@ -13847,6 +13867,13 @@ virDomainDefParseXML(xmlDocPtr xml, if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) goto error; + + if (virBitmapIsAllClear(def->cpumask)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid value of 'cpuset': %s"), tmp); + goto error; + } + VIR_FREE(tmp); } } diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 8a0f686..7ad3f66 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -178,6 +178,12 @@ virDomainNumatuneNodeParseXML(virDomainNumaPtr numa, if (virBitmapParse(tmp, 0, &mem_node->nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0) goto cleanup; + + if (virBitmapIsAllClear(mem_node->nodeset)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid value of 'nodeset': %s"), tmp); + goto cleanup; + } VIR_FREE(tmp); } @@ -233,10 +239,19 @@ virDomainNumatuneParseXML(virDomainNumaPtr numa, } VIR_FREE(tmp); - if ((tmp = virXMLPropString(node, "nodeset")) && - virBitmapParse(tmp, 0, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto cleanup; - VIR_FREE(tmp); + tmp = virXMLPropString(node, "nodeset"); + if (tmp) { + if (virBitmapParse(tmp, 0, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup; + + if (virBitmapIsAllClear(nodeset)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid value of 'nodeset': %s"), tmp); + goto cleanup; + } + + VIR_FREE(tmp); + } } if (virDomainNumatuneSet(numa, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7eb5a7d..eae2809 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10106,8 +10106,9 @@ qemuDomainSetNumaParameters(virDomainPtr dom, goto endjob; if (virBitmapIsAllClear(nodeset)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("Invalid nodeset for numatune")); + virReportError(VIR_ERR_OPERATION_INVALID, + _("Invalid nodeset of 'numatune': %s"), + param->value.s); goto endjob; } } diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c index 5a170d3..d77abf3 100644 --- a/src/xenconfig/xen_sxpr.c +++ b/src/xenconfig/xen_sxpr.c @@ -1165,6 +1165,13 @@ xenParseSxpr(const struct sexpr *root, if (virBitmapParse(cpus, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) goto error; + + if (virBitmapIsAllClear(def->cpumask)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid value of 'cpumask': %s"), + cpus); + goto error; + } } def->maxvcpus = sexpr_int(root, "domain/vcpus"); -- 1.9.3
This patch partially reverts commit 983f5a which added a check for invalid nodeset "0,^0" into virBitmapParse function. This change broke the logic, as an empty bitmap should not cause an error. This should rather be checked at individual spots. --- src/util/virbitmap.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 5322bce..bf905ab 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -416,9 +416,6 @@ virBitmapParse(const char *str, } } - if (virBitmapIsAllClear(*bitmap)) - goto error; - return virBitmapCountBits(*bitmap); error: -- 1.9.3
On Fri, Apr 10, 2015 at 12:41:13 +0200, Erik Skultety wrote:
This patch partially reverts commit 983f5a which added a check for invalid nodeset "0,^0" into virBitmapParse function. This change broke the logic, as an empty bitmap should not cause an error. This should rather be checked at individual spots.
This should be squashed in the previous patch. Additionally it would be great if you could also add tests to virbitmaptest to test this particular case so that somebody doesn't change it in the future. Peter
On Fri, Apr 10, 2015 at 12:41:12 +0200, Erik Skultety wrote:
This patch adds checks for empty bitmaps right after the calls of virBitmapParse. These only include spots where set API's are called and where domain's XML is parsed.
https://bugzilla.redhat.com/show_bug.cgi?id=1210545 --- src/conf/domain_conf.c | 35 +++++++++++++++++++++++++++++++---- src/conf/numa_conf.c | 23 +++++++++++++++++++---- src/qemu/qemu_driver.c | 5 +++-- src/xenconfig/xen_sxpr.c | 7 +++++++ 4 files changed, 60 insertions(+), 10 deletions(-)
I've "git grep"'d a few uses of this function and found a few places that would also need a chceck. Few examples are the XML parser for networks, the use in qemuProcessStart() to parse the automatic placemenet and so on .. The rest looks good though. Peter
On 04/13/2015 07:10 AM, Peter Krempa wrote:
On Fri, Apr 10, 2015 at 12:41:12 +0200, Erik Skultety wrote:
This patch adds checks for empty bitmaps right after the calls of virBitmapParse. These only include spots where set API's are called and where domain's XML is parsed.
https://bugzilla.redhat.com/show_bug.cgi?id=1210545 --- src/conf/domain_conf.c | 35 +++++++++++++++++++++++++++++++---- src/conf/numa_conf.c | 23 +++++++++++++++++++---- src/qemu/qemu_driver.c | 5 +++-- src/xenconfig/xen_sxpr.c | 7 +++++++ 4 files changed, 60 insertions(+), 10 deletions(-)
I've "git grep"'d a few uses of this function and found a few places that would also need a chceck. Few examples are the XML parser for networks, the use in qemuProcessStart() to parse the automatic placemenet and so on ..
The rest looks good though.
Peter
At the moment, there's really no need to check the bitmap 'class_id' in network.conf, because it's a status XML anyway, however even if user changed our status XML and an error occurred, it would be user's responsibility, the only exception to this would be a daemon crash which isn't this case. Moreover, if you parse the bitmap "0,^0" the empty bitmap wouldn't have almost any effect, because by default first 3 bits are always set during network object creation. To the qemuProcessStart, we call numad to get a suggestion for the nodeset and the only problem would be if numad returned empty string, however this would be handled by virBitmapParse itself successfully. The other occurrences are tests mostly, v1 included this check for XEN parser, I'm not sure about the 'Parallels' though. Anyway, I squashed the second patch into this one as you suggested and modified an existing test in virbitmaptest.c for v2. (I thought checking this specific case wasn't worth having a separate test, so I modified bitmap-overlap test, instead of just testing the bitmap parsing.) Erik
participants (2)
-
Erik Skultety -
Peter Krempa