[libvirt] [PATCH 0/3] tighten up integer parsing
I thought my recent improvements to <memory> parsing were complete, until I stumbled upon https://bugzilla.redhat.com/show_bug.cgi?id=617711, which shows a file that fails 'virt-xml-validate' but was silently accepted, and worse, misintepreted the user's intentions. I didn't have time to scrub the entire code base, but I can at least prevent the clean files from getting worse. 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 | 12 +++----- src/conf/storage_conf.c | 6 ++-- src/util/util.c | 14 +++++----- src/util/virmacaddr.c | 2 +- src/util/xml.c | 36 +++---------------------- tools/virsh.c | 66 +++++++++++++--------------------------------- 7 files changed, 53 insertions(+), 97 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. --- tools/virsh.c | 66 ++++++++++++++++---------------------------------------- 1 files changed, 19 insertions(+), 47 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 95ed7bc..3ec853b 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5758,14 +5758,11 @@ static const vshCmdOptDef opts_send_key[] = { 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; + int ret; - return val; + if (virStrToLong_i(key_name, NULL, 0, &ret) < 0 || ret > 0xffff) + return -1; + return ret; } static bool @@ -17973,8 +17970,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 +17980,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 +18000,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 +18010,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 +18030,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 +18040,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 +18094,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 +18104,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 +18124,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 +18134,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/18/2012 08:14 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. --- tools/virsh.c | 66 ++++++++++++++++---------------------------------------- 1 files changed, 19 insertions(+), 47 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 95ed7bc..3ec853b 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5758,14 +5758,11 @@ static const vshCmdOptDef opts_send_key[] = {
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; + int ret;
- return val; + if (virStrToLong_i(key_name, NULL, 0,&ret)< 0 || ret> 0xffff) + return -1; + return ret; }
static bool
Looks like a replacement pattern repeated a couple of times. ACK.
On 04/18/2012 06:59 PM, Stefan Berger wrote:
On 04/18/2012 08:14 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. --- tools/virsh.c | 66 ++++++++++++++++---------------------------------------- 1 files changed, 19 insertions(+), 47 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 95ed7bc..3ec853b 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5758,14 +5758,11 @@ static const vshCmdOptDef opts_send_key[] = {
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)
The old code rejected 0,
- return -1; + int ret;
- return val; + if (virStrToLong_i(key_name, NULL, 0,&ret)< 0 || ret> 0xffff)
but the new code allows it. v2 coming up. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 was not. I always love it when I can reduce lines of code while fixing bugs. * src/conf/domain_conf.c (virDomainDefParseXML): Avoid strtoll. * src/conf/storage_conf.c (virStorageDefParsePerms): Likewise. * src/util/xml.c (virXPathLongBase, virXPathULongBase) (virXPathULongLong, virXPathLongLong): Likewise. --- src/conf/domain_conf.c | 12 +++++------- src/conf/storage_conf.c | 6 +++--- src/util/xml.c | 36 ++++-------------------------------- 3 files changed, 12 insertions(+), 42 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5ab052a..9d61448 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8086,12 +8086,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 +8102,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..64cc0cd 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -570,14 +570,14 @@ 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/18/2012 08:14 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 was not.
I always love it when I can reduce lines of code while fixing bugs.
* src/conf/domain_conf.c (virDomainDefParseXML): Avoid strtoll. * src/conf/storage_conf.c (virStorageDefParsePerms): Likewise. * src/util/xml.c (virXPathLongBase, virXPathULongBase) (virXPathULongLong, virXPathLongLong): Likewise. --- src/conf/domain_conf.c | 12 +++++------- src/conf/storage_conf.c | 6 +++--- src/util/xml.c | 36 ++++-------------------------------- 3 files changed, 12 insertions(+), 42 deletions(-)
index 2330fa1..64cc0cd 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -570,14 +570,14 @@ 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;
Nit: empty line after var decl?
+ if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) {
[...] @@ -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;
Again there was pattern to it :-) ACK
On 04/18/2012 07:03 PM, Stefan Berger wrote:
On 04/18/2012 08:14 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 was not.
I always love it when I can reduce lines of code while fixing bugs.
* src/conf/domain_conf.c (virDomainDefParseXML): Avoid strtoll. * src/conf/storage_conf.c (virStorageDefParsePerms): Likewise. * src/util/xml.c (virXPathLongBase, virXPathULongBase) (virXPathULongLong, virXPathLongLong): Likewise. --- src/conf/domain_conf.c | 12 +++++------- src/conf/storage_conf.c | 6 +++--- src/util/xml.c | 36 ++++-------------------------------- 3 files changed, 12 insertions(+), 42 deletions(-)
Phooey. I fixed <memory>780M<memory>, but not <currentMemory>780M</currentMemory>. It's not enough to change virXPathULongLong to return -2 instead of 0 on parse errors, but the caller has to actually check for that return value. v2 coming up.
- if (*end || (perms->mode& ~0777)) { + int tmp;
Nit: empty line after var decl?
+ if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) {
Sure, I'll fix that as well in v2. -- 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-sckip) 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. --- 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/18/2012 08:14 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-sckip) before we are bulletproof, but that also entails scrubbing I'm not ready to do at the moment.
[...]
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) ||
ACK
participants (2)
-
Eric Blake -
Stefan Berger