Re: [libvirt] [PATCHv2 2/3] conf: tighten up XML integer parsing

https://bugzilla.redhat.com/show_bug.cgi?id=617711 reported that even with my recent patched to allow <memory unit='G'>1</memory>, people can still get away with trying <memory>1G</memory> and silently get <memory unit='KiB'>1</memory> instead. While virt-xml-validate catches the error, our C parser did not. Not to mention that it's always fun to fix bugs while reducing lines of code. :) * src/conf/domain_conf.c (virDomainParseMemory): Check for parse error. (virDomainDefParseXML): Avoid strtoll. * src/conf/storage_conf.c (virStorageDefParsePerms): Likewise. * src/util/xml.c (virXPathLongBase, virXPathULongBase) (virXPathULongLong, virXPathLongLong): Likewise. --- v2: fix virDomainParseMemory to detect parse errors on optional arguments, rather than silently ignoring those arguments src/conf/domain_conf.c | 24 ++++++++++++++---------- src/conf/storage_conf.c | 7 ++++--- src/util/xml.c | 36 ++++-------------------------------- 3 files changed, 22 insertions(+), 45 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5ab052a..8f352ea 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7609,10 +7609,16 @@ virDomainParseMemory(const char *xpath, xmlXPathContextPtr ctxt, virReportOOMError(); goto cleanup; } - if (virXPathULongLong(xpath_full, ctxt, &bytes) < 0) { - if (required) + ret = virXPathULongLong(xpath_full, ctxt, &bytes); + if (ret < 0) { + if (ret == -2) virDomainReportError(VIR_ERR_XML_ERROR, - "%s", _("missing memory element")); + _("could not parse memory element %s"), + xpath); + else if (required) + virDomainReportError(VIR_ERR_XML_ERROR, + _("missing memory element %s"), + xpath); else ret = 0; goto cleanup; @@ -8086,12 +8092,11 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (STREQ(tmp, "reset")) { def->clock.data.utc_reset = true; } else { - char *conv = NULL; - unsigned long long val; - val = strtoll(tmp, &conv, 10); - if (conv == tmp || *conv != '\0') { - virDomainReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown clock adjustment '%s'"), tmp); + if (virStrToLong_ll(tmp, NULL, 10, + &def->clock.data.variable.adjustment) < 0) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("unknown clock adjustment '%s'"), + tmp); goto error; } switch (def->clock.offset) { @@ -8103,7 +8108,6 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, break; } def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_VARIABLE; - def->clock.data.variable.adjustment = val; } VIR_FREE(tmp); } else { diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 2330fa1..7579327 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -570,14 +570,15 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, if (!mode) { perms->mode = defaultmode; } else { - char *end = NULL; - perms->mode = strtol(mode, &end, 8); - if (*end || (perms->mode & ~0777)) { + int tmp; + + if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) { VIR_FREE(mode); virStorageReportError(VIR_ERR_XML_ERROR, "%s", _("malformed octal mode")); goto error; } + perms->mode = tmp;