[libvirt] [PATCH 0/2] Fix parsing of vCPU counts from domain XMLs.
These patches fix the parsing and the returned error if parsing of vCPUs failed. If a overflowing number was used for the count, the domain might disappear after the restart of the daemon. Peter Krempa (2): conf: Check if number of vCPUs fits in the storage variable conf: Improve error messages if parsing of vCPU count fails src/conf/domain_conf.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) -- 1.8.1.1
The count of vCPUs for a domain is extracted as a usingned long variable but is stored in a unsigned short. If the actual number was too large, a faulty number was stored. --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0b9ba13..3e95ec9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9085,7 +9085,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->maxvcpus = 1; } else { def->maxvcpus = count; - if (count == 0) { + if (count == 0 || (unsigned short) count != count) { virReportError(VIR_ERR_XML_ERROR, _("invalid maxvcpus %lu"), count); goto error; @@ -9101,7 +9101,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->vcpus = def->maxvcpus; } else { def->vcpus = count; - if (count == 0) { + if (count == 0 || (unsigned short) count != count) { virReportError(VIR_ERR_XML_ERROR, _("invalid current vcpus %lu"), count); goto error; -- 1.8.1.1
On 01/22/2013 09:31 AM, Peter Krempa wrote:
The count of vCPUs for a domain is extracted as a usingned long variable but is stored in a unsigned short. If the actual number was too large, a faulty number was stored. --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0b9ba13..3e95ec9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9085,7 +9085,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->maxvcpus = 1; } else { def->maxvcpus = count; - if (count == 0) { + if (count == 0 || (unsigned short) count != count) {
maxvcpus is a 'unsigned short' and count is an 'unsigned long', thus if def->maxvcpus != count after this point, then we have the overflow, right? Or would the compiler "adjust" that comparison behind our back on an if check?
virReportError(VIR_ERR_XML_ERROR, _("invalid maxvcpus %lu"), count); goto error; @@ -9101,7 +9101,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->vcpus = def->maxvcpus; } else { def->vcpus = count; - if (count == 0) { + if (count == 0 || (unsigned short) count != count) {
Same comment as 'maxvcpus'
virReportError(VIR_ERR_XML_ERROR, _("invalid current vcpus %lu"), count); goto error;
ACK - I think what you've done is right, although perhaps someone with a bit more knowledge of what the compiler does could pipe in (I'm curious too). John
On 01/22/13 16:07, John Ferlan wrote:
On 01/22/2013 09:31 AM, Peter Krempa wrote:
The count of vCPUs for a domain is extracted as a usingned long variable but is stored in a unsigned short. If the actual number was too large, a faulty number was stored. --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0b9ba13..3e95ec9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9085,7 +9085,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->maxvcpus = 1; } else { def->maxvcpus = count; - if (count == 0) { + if (count == 0 || (unsigned short) count != count) {
maxvcpus is a 'unsigned short' and count is an 'unsigned long', thus if def->maxvcpus != count after this point, then we have the overflow, right? Or would the compiler "adjust" that comparison behind our back on an if check?
You mean changing the explicit typecast with checking of def->maxvcpus? This works in gcc too, but I'm afraid I have -O0 as a default for libvirt. I'm not sure if a compiler is allowed to optimize such a comparison, but the explicit typecast is probably safer and it is more noticeable to a possible future reader of that piece of code.
virReportError(VIR_ERR_XML_ERROR, _("invalid maxvcpus %lu"), count); goto error; @@ -9101,7 +9101,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->vcpus = def->maxvcpus; } else { def->vcpus = count; - if (count == 0) { + if (count == 0 || (unsigned short) count != count) {
Same comment as 'maxvcpus'
virReportError(VIR_ERR_XML_ERROR, _("invalid current vcpus %lu"), count); goto error;
ACK - I think what you've done is right, although perhaps someone with a bit more knowledge of what the compiler does could pipe in (I'm curious too).
For now, I will probably push this as is. We still can fix this in the future if there will be a cleaner solution. Thanks for the review. Peter
John
At Tue, 22 Jan 2013 17:47:36 +0100, Peter Krempa wrote:
On 01/22/13 16:07, John Ferlan wrote:
On 01/22/2013 09:31 AM, Peter Krempa wrote:
The count of vCPUs for a domain is extracted as a usingned long variable
May be too late, but there's a typo s/usingned/unsigned Claudio -- AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany Phone: +49 341 265 310 19 Web:<http://www.av-test.org> Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076) Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
On 01/22/2013 08:07 AM, John Ferlan wrote:
On 01/22/2013 09:31 AM, Peter Krempa wrote:
The count of vCPUs for a domain is extracted as a usingned long variable but is stored in a unsigned short. If the actual number was too large, a faulty number was stored.
+ if (count == 0 || (unsigned short) count != count) {
maxvcpus is a 'unsigned short' and count is an 'unsigned long', thus if def->maxvcpus != count after this point, then we have the overflow, right? Or would the compiler "adjust" that comparison behind our back on an if check?
For unsigned types, the C standard guarantees that overflow wraps around, and that casting a larger type down to a smaller type in order to compare the same number is required to tell you if overflow happened, at all optimization levels. This code is valid. For signed types, the C standard says overflow leads to unspecified behavior, so all bets are off. Thankfully, this isn't dealing with signed types.
ACK - I think what you've done is right, although perhaps someone with a bit more knowledge of what the compiler does could pipe in (I'm curious too).
I think the ACK stands. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- src/conf/domain_conf.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e95ec9..75450ad 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9087,7 +9087,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->maxvcpus = count; if (count == 0 || (unsigned short) count != count) { virReportError(VIR_ERR_XML_ERROR, - _("invalid maxvcpus %lu"), count); + _("invalid maximum number of vCPUs '%lu'"), count); goto error; } } @@ -9103,13 +9103,14 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->vcpus = count; if (count == 0 || (unsigned short) count != count) { virReportError(VIR_ERR_XML_ERROR, - _("invalid current vcpus %lu"), count); + _("invalid current number of vCPUs ;%lu'"), count); goto error; } if (def->maxvcpus < count) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("maxvcpus must not be less than current vcpus (%d < %lu)"), + _("maxvcpus must not be less than current vcpus " + "(%d < %lu)"), def->maxvcpus, count); goto error; } -- 1.8.1.1
On 01/22/2013 09:31 AM, Peter Krempa wrote:
--- src/conf/domain_conf.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e95ec9..75450ad 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9087,7 +9087,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->maxvcpus = count; if (count == 0 || (unsigned short) count != count) { virReportError(VIR_ERR_XML_ERROR, - _("invalid maxvcpus %lu"), count); + _("invalid maximum number of vCPUs '%lu'"), count); goto error; } } @@ -9103,13 +9103,14 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->vcpus = count; if (count == 0 || (unsigned short) count != count) { virReportError(VIR_ERR_XML_ERROR, - _("invalid current vcpus %lu"), count); + _("invalid current number of vCPUs ;%lu'"), count);
s/;%lu/'%lu
goto error; }
if (def->maxvcpus < count) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("maxvcpus must not be less than current vcpus (%d < %lu)"), + _("maxvcpus must not be less than current vcpus " + "(%d < %lu)"), def->maxvcpus, count); goto error; }
ACK with the change
On 01/22/13 16:07, John Ferlan wrote:
On 01/22/2013 09:31 AM, Peter Krempa wrote:
--- src/conf/domain_conf.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e95ec9..75450ad 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9087,7 +9087,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->maxvcpus = count; if (count == 0 || (unsigned short) count != count) { virReportError(VIR_ERR_XML_ERROR, - _("invalid maxvcpus %lu"), count); + _("invalid maximum number of vCPUs '%lu'"), count); goto error; } } @@ -9103,13 +9103,14 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, def->vcpus = count; if (count == 0 || (unsigned short) count != count) { virReportError(VIR_ERR_XML_ERROR, - _("invalid current vcpus %lu"), count); + _("invalid current number of vCPUs ;%lu'"), count);
s/;%lu/'%lu
goto error; }
if (def->maxvcpus < count) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("maxvcpus must not be less than current vcpus (%d < %lu)"), + _("maxvcpus must not be less than current vcpus " + "(%d < %lu)"), def->maxvcpus, count); goto error; }
ACK with the change
Bah, right. Thanks for the review. Peter
participants (4)
-
Claudio Bley -
Eric Blake -
John Ferlan -
Peter Krempa