[libvirt] [PATCHv2 0/3] tighten up integer parsing
v1: https://www.redhat.com/archives/libvir-list/2012-April/msg00954.html changes in v2: fix regression in patch 1, incomplete change in patch 2 Eric Blake (3): virsh: avoid strtol conf: tighten up XML integer parsing build: avoid strtol and strtod cfg.mk | 14 ++++++++++ src/conf/domain_conf.c | 24 ++++++++++------- src/conf/storage_conf.c | 7 +++-- src/util/util.c | 14 +++++----- src/util/virmacaddr.c | 2 +- src/util/xml.c | 36 +++---------------------- tools/virsh.c | 67 ++++++++++++++--------------------------------- 7 files changed, 64 insertions(+), 100 deletions(-) -- 1.7.7.6
We were forgetting to check errno for overflow. * tools/virsh.c (get_integer_keycode, vshCommandOptInt) (vshCommandOptUInt, vshCommandOptUL, vshCommandOptLongLong) (vshCommandOptULongLong): Rewrite to be safer. --- v2: get_integer_keycode still must reject 0 tools/virsh.c | 67 +++++++++++++++++---------------------------------------- 1 files changed, 20 insertions(+), 47 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 95ed7bc..0bfb529 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5756,15 +5756,13 @@ static const vshCmdOptDef opts_send_key[] = { {NULL, 0, 0, NULL} }; -static int get_integer_keycode(const char *key_name) +static int +get_integer_keycode(const char *key_name) { - long val; - char *endptr; - - val = strtol(key_name, &endptr, 0); - if (*endptr != '\0' || val > 0xffff || val <= 0) - return -1; + unsigned int val; + if (virStrToLong_ui(key_name, NULL, 0, &val) < 0 || val > 0xffff || !val) + return -1; return val; } @@ -17973,8 +17971,6 @@ vshCommandOptInt(const vshCmd *cmd, const char *name, int *value) { vshCmdOpt *arg; int ret; - int num; - char *end_p = NULL; ret = vshCommandOpt(cmd, name, &arg); if (ret <= 0) @@ -17985,12 +17981,9 @@ vshCommandOptInt(const vshCmd *cmd, const char *name, int *value) return -2; } - num = strtol(arg->data, &end_p, 10); - if (arg->data != end_p && *end_p == 0) { - *value = num; - return 1; - } - return -1; + if (virStrToLong_i(arg->data, NULL, 10, value) < 0) + return -1; + return 1; } @@ -18008,8 +18001,6 @@ vshCommandOptUInt(const vshCmd *cmd, const char *name, unsigned int *value) { vshCmdOpt *arg; int ret; - unsigned int num; - char *end_p = NULL; ret = vshCommandOpt(cmd, name, &arg); if (ret <= 0) @@ -18020,12 +18011,9 @@ vshCommandOptUInt(const vshCmd *cmd, const char *name, unsigned int *value) return -2; } - num = strtoul(arg->data, &end_p, 10); - if (arg->data != end_p && *end_p == 0) { - *value = num; - return 1; - } - return -1; + if (virStrToLong_ui(arg->data, NULL, 10, value) < 0) + return -1; + return 1; } @@ -18043,8 +18031,6 @@ vshCommandOptUL(const vshCmd *cmd, const char *name, unsigned long *value) { vshCmdOpt *arg; int ret; - unsigned long num; - char *end_p = NULL; ret = vshCommandOpt(cmd, name, &arg); if (ret <= 0) @@ -18055,12 +18041,9 @@ vshCommandOptUL(const vshCmd *cmd, const char *name, unsigned long *value) return -2; } - num = strtoul(arg->data, &end_p, 10); - if (arg->data != end_p && *end_p == 0) { - *value = num; - return 1; - } - return -1; + if (virStrToLong_ul(arg->data, NULL, 10, value) < 0) + return -1; + return 1; } /** @@ -18112,8 +18095,6 @@ vshCommandOptLongLong(const vshCmd *cmd, const char *name, { vshCmdOpt *arg; int ret; - long long num; - char *end_p = NULL; ret = vshCommandOpt(cmd, name, &arg); if (ret <= 0) @@ -18124,12 +18105,9 @@ vshCommandOptLongLong(const vshCmd *cmd, const char *name, return -2; } - num = strtoll(arg->data, &end_p, 10); - if (arg->data != end_p && *end_p == 0) { - *value = num; - return 1; - } - return -1; + if (virStrToLong_ll(arg->data, NULL, 10, value) < 0) + return -1; + return 1; } /** @@ -18147,8 +18125,6 @@ vshCommandOptULongLong(const vshCmd *cmd, const char *name, { vshCmdOpt *arg; int ret; - unsigned long long num; - char *end_p = NULL; ret = vshCommandOpt(cmd, name, &arg); if (ret <= 0) @@ -18159,12 +18135,9 @@ vshCommandOptULongLong(const vshCmd *cmd, const char *name, return -2; } - num = strtoull(arg->data, &end_p, 10); - if (arg->data != end_p && *end_p == 0) { - *value = num; - return 1; - } - return -1; + if (virStrToLong_ull(arg->data, NULL, 10, value) < 0) + return -1; + return 1; } -- 1.7.7.6
On 04/19/2012 02:24 PM, Eric Blake wrote:
We were forgetting to check errno for overflow.
* tools/virsh.c (get_integer_keycode, vshCommandOptInt) (vshCommandOptUInt, vshCommandOptUL, vshCommandOptLongLong) (vshCommandOptULongLong): Rewrite to be safer. ---
v2: get_integer_keycode still must reject 0
tools/virsh.c | 67 +++++++++++++++++---------------------------------------- 1 files changed, 20 insertions(+), 47 deletions(-)
ACK.
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; VIR_FREE(mode); } diff --git a/src/util/xml.c b/src/util/xml.c index 79a9d27..7411968 100644 --- a/src/util/xml.c +++ b/src/util/xml.c @@ -174,15 +174,8 @@ virXPathLongBase(const char *xpath, ctxt->node = relnode; if ((obj != NULL) && (obj->type == XPATH_STRING) && (obj->stringval != NULL) && (obj->stringval[0] != 0)) { - char *conv = NULL; - long val; - - val = strtol((const char *) obj->stringval, &conv, base); - if (conv == (const char *) obj->stringval) { + if (virStrToLong_l((char *) obj->stringval, NULL, base, value) < 0) ret = -2; - } else { - *value = val; - } } else if ((obj != NULL) && (obj->type == XPATH_NUMBER) && (!(isnan(obj->floatval)))) { *value = (long) obj->floatval; @@ -287,15 +280,8 @@ virXPathULongBase(const char *xpath, ctxt->node = relnode; if ((obj != NULL) && (obj->type == XPATH_STRING) && (obj->stringval != NULL) && (obj->stringval[0] != 0)) { - char *conv = NULL; - long val; - - val = strtoul((const char *) obj->stringval, &conv, base); - if (conv == (const char *) obj->stringval) { + if (virStrToLong_ul((char *) obj->stringval, NULL, base, value) < 0) ret = -2; - } else { - *value = val; - } } else if ((obj != NULL) && (obj->type == XPATH_NUMBER) && (!(isnan(obj->floatval)))) { *value = (unsigned long) obj->floatval; @@ -411,15 +397,8 @@ virXPathULongLong(const char *xpath, ctxt->node = relnode; if ((obj != NULL) && (obj->type == XPATH_STRING) && (obj->stringval != NULL) && (obj->stringval[0] != 0)) { - char *conv = NULL; - unsigned long long val; - - val = strtoull((const char *) obj->stringval, &conv, 10); - if (conv == (const char *) obj->stringval) { + if (virStrToLong_ull((char *) obj->stringval, NULL, 10, value) < 0) ret = -2; - } else { - *value = val; - } } else if ((obj != NULL) && (obj->type == XPATH_NUMBER) && (!(isnan(obj->floatval)))) { *value = (unsigned long long) obj->floatval; @@ -465,15 +444,8 @@ virXPathLongLong(const char *xpath, ctxt->node = relnode; if ((obj != NULL) && (obj->type == XPATH_STRING) && (obj->stringval != NULL) && (obj->stringval[0] != 0)) { - char *conv = NULL; - unsigned long long val; - - val = strtoll((const char *) obj->stringval, &conv, 10); - if (conv == (const char *) obj->stringval) { + if (virStrToLong_ll((char *) obj->stringval, NULL, 10, value) < 0) ret = -2; - } else { - *value = val; - } } else if ((obj != NULL) && (obj->type == XPATH_NUMBER) && (!(isnan(obj->floatval)))) { *value = (long long) obj->floatval; -- 1.7.7.6
On 04/19/2012 02:24 PM, Eric Blake wrote:
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;
I'm curious why in the case of clock.data.variable.adjustment, you switched it to do the conversion directly into the object attribute, while in this case you switched it in the opposite direction. ACK, in any case.
On 04/19/2012 06:33 PM, Laine Stump wrote:
On 04/19/2012 02:24 PM, Eric Blake wrote:
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. :)
- 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;
I'm curious why in the case of clock.data.variable.adjustment, you switched it to do the conversion directly into the object attribute, while in this case you switched it in the opposite direction.
virStrToLong_i() requires an int, but we are not guaranteed that mode_t is an int, so I had to go through a temporary here.
ACK, in any case.
Thanks for the reviews. I've pushed the series. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Ensure we don't introduce any more lousy integer parsing in new code, while avoiding a scrub-down of existing legacy code. Note that we also need to enable sc_prohibit_atoi_atof (see cfg.mk local-checks-to-skip) before we are bulletproof, but that also entails scrubbing I'm not ready to do at the moment. * src/util/util.c (virStrToLong_i, virStrToLong_ui) (virStrToLong_l, virStrToLong_ul, virStrToLong_ll) (virStrToLong_ull, virStrToDouble): Mark exemptions. * src/util/virmacaddr.c (virMacAddrParse): Likewise. * cfg.mk (sc_prohibit_strtol): New syntax check. (exclude_file_name_regexp--sc_prohibit_strtol): Ignore files that I'm not willing to fix yet. (local-checks-to-skip): Re-enable sc_prohibit_atoi_atof. --- v2: no change cfg.mk | 14 ++++++++++++++ src/util/util.c | 14 +++++++------- src/util/virmacaddr.c | 2 +- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/cfg.mk b/cfg.mk index 71e9a1d..fb4df2f 100644 --- a/cfg.mk +++ b/cfg.mk @@ -353,6 +353,17 @@ sc_prohibit_strncmp: halt='$(ME): use STREQLEN or STRPREFIX instead of str''ncmp' \ $(_sc_search_regexp) +# strtol and friends are too easy to misuse +sc_prohibit_strtol: + @prohibit='\bstrto(u?ll?|[ui]max) *\(' \ + exclude='exempt from syntax-check' \ + halt='$(ME): use virStrToLong_*, not strtol variants' \ + $(_sc_search_regexp) + @prohibit='\bstrto[df] *\(' \ + exclude='exempt from syntax-check' \ + halt='$(ME): use virStrToDouble, not strtod variants' \ + $(_sc_search_regexp) + # Use virAsprintf rather than as'printf since *strp is undefined on error. sc_prohibit_asprintf: @prohibit='\<v?a[s]printf\>' \ @@ -799,6 +810,9 @@ exclude_file_name_regexp--sc_prohibit_sprintf = \ exclude_file_name_regexp--sc_prohibit_strncpy = \ ^(src/util/util|tools/virsh)\.c$$ +exclude_file_name_regexp--sc_prohibit_strtol = \ + ^src/(util/sexpr|(vbox|xen|xenxs)/.*)\.c$$ + exclude_file_name_regexp--sc_prohibit_xmlGetProp = ^src/util/xml\.c$$ exclude_file_name_regexp--sc_prohibit_xmlURI = ^src/util/viruri\.c$$ diff --git a/src/util/util.c b/src/util/util.c index 1b39227..48358b2 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1497,7 +1497,7 @@ virStrToLong_i(char const *s, char **end_ptr, int base, int *result) int err; errno = 0; - val = strtol(s, &p, base); + val = strtol(s, &p, base); /* exempt from syntax-check */ err = (errno || (!end_ptr && *p) || p == s || (int) val != val); if (end_ptr) *end_ptr = p; @@ -1516,7 +1516,7 @@ virStrToLong_ui(char const *s, char **end_ptr, int base, unsigned int *result) int err; errno = 0; - val = strtoul(s, &p, base); + val = strtoul(s, &p, base); /* exempt from syntax-check */ err = (errno || (!end_ptr && *p) || p == s || (unsigned int) val != val); if (end_ptr) *end_ptr = p; @@ -1535,7 +1535,7 @@ virStrToLong_l(char const *s, char **end_ptr, int base, long *result) int err; errno = 0; - val = strtol(s, &p, base); + val = strtol(s, &p, base); /* exempt from syntax-check */ err = (errno || (!end_ptr && *p) || p == s); if (end_ptr) *end_ptr = p; @@ -1554,7 +1554,7 @@ virStrToLong_ul(char const *s, char **end_ptr, int base, unsigned long *result) int err; errno = 0; - val = strtoul(s, &p, base); + val = strtoul(s, &p, base); /* exempt from syntax-check */ err = (errno || (!end_ptr && *p) || p == s); if (end_ptr) *end_ptr = p; @@ -1573,7 +1573,7 @@ virStrToLong_ll(char const *s, char **end_ptr, int base, long long *result) int err; errno = 0; - val = strtoll(s, &p, base); + val = strtoll(s, &p, base); /* exempt from syntax-check */ err = (errno || (!end_ptr && *p) || p == s); if (end_ptr) *end_ptr = p; @@ -1592,7 +1592,7 @@ virStrToLong_ull(char const *s, char **end_ptr, int base, unsigned long long *re int err; errno = 0; - val = strtoull(s, &p, base); + val = strtoull(s, &p, base); /* exempt from syntax-check */ err = (errno || (!end_ptr && *p) || p == s); if (end_ptr) *end_ptr = p; @@ -1612,7 +1612,7 @@ virStrToDouble(char const *s, int err; errno = 0; - val = strtod(s, &p); + val = strtod(s, &p); /* exempt from syntax-check */ err = (errno || (!end_ptr && *p) || p == s); if (end_ptr) *end_ptr = p; diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c index beb6274..6c0fb24 100644 --- a/src/util/virmacaddr.c +++ b/src/util/virmacaddr.c @@ -86,7 +86,7 @@ virMacAddrParse(const char* str, unsigned char *addr) if (!c_isxdigit(*str)) break; - result = strtoul(str, &end_ptr, 16); + result = strtoul(str, &end_ptr, 16); /* exempt from syntax-check */ if ((end_ptr - str) < 1 || 2 < (end_ptr - str) || (errno != 0) || -- 1.7.7.6
On 04/19/2012 02:24 PM, Eric Blake wrote:
Ensure we don't introduce any more lousy integer parsing in new code, while avoiding a scrub-down of existing legacy code.
Note that we also need to enable sc_prohibit_atoi_atof (see cfg.mk local-checks-to-skip) before we are bulletproof, but that also entails scrubbing I'm not ready to do at the moment.
* src/util/util.c (virStrToLong_i, virStrToLong_ui) (virStrToLong_l, virStrToLong_ul, virStrToLong_ll) (virStrToLong_ull, virStrToDouble): Mark exemptions. * src/util/virmacaddr.c (virMacAddrParse): Likewise. * cfg.mk (sc_prohibit_strtol): New syntax check. (exclude_file_name_regexp--sc_prohibit_strtol): Ignore files that I'm not willing to fix yet. (local-checks-to-skip): Re-enable sc_prohibit_atoi_atof. ---
v2: no change
Then Stefan's ACK from V1 holds :-)
participants (2)
-
Eric Blake -
Laine Stump