[libvirt] [PATCHv2 0/3] fix virstoragetest failure on 32-bit
v1 was here, but it got nacked for being ugly: https://www.redhat.com/archives/libvir-list/2014-April/msg01132.html so in this version, I improved virstring first, then used the new clean function. This is a build-breaker fix, so it deserves to be in 1.2.4; but it is large enough that it needs a review to go in. Eric Blake (3): util: fix uint parsing on 64-bit platforms util: new stricter unsigned int parsing storage: reject negative indices src/libvirt_private.syms | 3 + src/util/virstoragefile.c | 2 +- src/util/virstring.c | 98 +++++++++++++++++++++-- src/util/virstring.h | 14 +++- tests/virstringtest.c | 200 +++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 307 insertions(+), 10 deletions(-) -- 1.9.0
Commit f22b7899 called to light a long-standing latent bug: the behavior of virStrToLong_ui was different on 32-bit platforms than on 64-bit platforms. Curse you, C type promotion and narrowing rules, and strtoul specification. POSIX says that for a 32-bit long, strtol handles only 2^32 values [LONG_MIN to LONG_MAX] while strtoul handles 2^33 - 1 values [-ULONG_MAX to ULONG_MAX] with twos-complement wraparound for negatives. Thus, parsing -1 as unsigned long produces ULONG_MAX, rather than a range error. We WANT[1] this same shortcut for turning -1 into UINT_MAX when parsing to int; and get it for free with 32-bit long. But with 64-bit long, ULONG_MAX is outside the range of int and we were rejecting it as invalid; meanwhile, we were silently treating -18446744073709551615 as 1 even though it textually exceeds INT_MIN. Too bad there's not a strtoui() in libc that does guaranteed parsing to int, regardless of the size of long. The bug has been latent since 2007, introduced by Jim Meyering in commit 5d25419 in the attempt to eradicate unsafe use of strto[u]l when parsing ints and longs. How embarrassing that we are only discovering it now - so I'm adding a testsuite to ensure that it covers all the corner cases we care about. [1] Ideally, we really want the caller to be able to choose whether to allow negative numbers to wrap around to their 2s-complement counterpart, as in strtoul, or to force a stricter input range of [0 to UINT_MAX] by rejecting negative signs; this will be added in a later patch for all three int types. This patch is tested on both 32- and 64-bit; the enhanced virstringtest passes on both platforms, while virstoragetest now reliably fails on both platforms instead of just 32-bit platforms. That test will be fixed later. * src/util/virstring.c (virStrToLong_ui): Ensure same behavior regardless of platform long size. * tests/virstringtest.c (testStringToLong): New function. (mymain): Comprehensively test string to long parsing. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstring.c | 18 ++++- tests/virstringtest.c | 177 +++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 191 insertions(+), 4 deletions(-) diff --git a/src/util/virstring.c b/src/util/virstring.c index 64c7259..b5fc638 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2012-2013 Red Hat, Inc. + * Copyright (C) 2012-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -217,11 +217,23 @@ virStrToLong_ui(char const *s, char **end_ptr, int base, unsigned int *result) { unsigned long int val; char *p; - int err; + bool err = false; errno = 0; val = strtoul(s, &p, base); /* exempt from syntax-check */ - err = (errno || (!end_ptr && *p) || p == s || (unsigned int) val != val); + + /* This one's tricky. We _want_ to allow "-1" as shorthand for + * UINT_MAX regardless of whether long is 32-bit or 64-bit. But + * strtoul treats "-1" as ULONG_MAX, and going from ulong back + * to uint differs depending on the size of long. */ + if (sizeof(long) > sizeof(int) && memchr(s, '-', p - s)) { + if (-val > UINT_MAX) + err = true; + else + val &= 0xffffffff; + } + + err |= (errno || (!end_ptr && *p) || p == s || (unsigned int) val != val); if (end_ptr) *end_ptr = p; if (err) diff --git a/tests/virstringtest.c b/tests/virstringtest.c index a4ae966..0380df1 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -23,6 +23,8 @@ #include <stdlib.h> #include "testutils.h" +#include "intprops.h" +#include "verify.h" #include "virerror.h" #include "viralloc.h" #include "virfile.h" @@ -286,7 +288,7 @@ struct stringSearchData { }; static int -testStringSearch(const void *opaque ATTRIBUTE_UNUSED) +testStringSearch(const void *opaque) { const struct stringSearchData *data = opaque; char **matches = NULL; @@ -373,6 +375,93 @@ testStringReplace(const void *opaque ATTRIBUTE_UNUSED) } +struct stringToLongData { + const char *str; + const char *suffix; + int si; /* syntax-check doesn't like bare 'i' */ + int si_ret; + unsigned int ui; + int ui_ret; + /* No expected results for long: on 32-bit platforms, it is the + * same as int, on 64-bit platforms it is the same as long long */ + long long ll; + int ll_ret; + unsigned long long ull; + int ull_ret; +}; + +/* This test makes assumptions about our compilation platform that are + * not guaranteed by POSIX. Good luck to you if you are crazy enough + * to try and port libvirt to a platform with 16-bit int. */ +verify(sizeof(int) == 4); +verify(TYPE_TWOS_COMPLEMENT(int)); +verify(sizeof(long) == sizeof(int) || sizeof(long) == sizeof(long long)); +verify(TYPE_TWOS_COMPLEMENT(long)); +verify(sizeof(long long) == 8); +verify(TYPE_TWOS_COMPLEMENT(long long)); + +static int +testStringToLong(const void *opaque) +{ + const struct stringToLongData *data = opaque; + int ret = 0; + char *end; + long l; + unsigned long ul; + +#define TEST_ONE(Str, Suff, Type, Fn, Fmt, Exp, Exp_ret) \ + do { \ + Type value = 5; \ + int result; \ + end = (char *) "oops"; \ + result = virStrToLong_ ## Fn(Str, Suff ? &end : NULL, \ + 0, &value); \ + /* On failure, end is modified, value is unchanged */ \ + if (result != (Exp_ret)) { \ + fprintf(stderr, \ + "type " #Fn " returned %d expected %d\n", \ + result, Exp_ret); \ + ret = -1; \ + } \ + if (value != ((Exp_ret) ? 5 : Exp)) { \ + fprintf(stderr, \ + "type " #Fn " value " Fmt " expected " Fmt "\n", \ + value, ((Exp_ret) ? 5 : Exp)); \ + ret = -1; \ + } \ + if (Suff && STRNEQ_NULLABLE(Suff, end)) { \ + fprintf(stderr, \ + "type " #Fn " end '%s' expected '%s'\n", \ + NULLSTR(end), Suff); \ + ret = -1; \ + } \ + } while (0) + + TEST_ONE(data->str, data->suffix, int, i, "%d", + data->si, data->si_ret); + TEST_ONE(data->str, data->suffix, unsigned int, ui, "%u", + data->ui, data->ui_ret); + + /* We hate adding new API with 'long', and prefer 'int' or 'long + * long' instead, since platform-specific results are evil */ + l = (sizeof(int) == sizeof(long)) ? data->si : data->ll; + TEST_ONE(data->str, data->suffix, long, l, "%ld", + l, (sizeof(int) == sizeof(long)) ? data->si_ret : data->ll_ret); + ul = (sizeof(int) == sizeof(long)) ? data->ui : data->ull; + TEST_ONE(data->str, data->suffix, unsigned long, ul, "%lu", + ul, (sizeof(int) == sizeof(long)) ? data->ui_ret : data->ull_ret); + + TEST_ONE(data->str, data->suffix, long long, ll, "%lld", + data->ll, data->ll_ret); + TEST_ONE(data->str, data->suffix, unsigned long long, ull, "%llu", + data->ull, data->ull_ret); + +#undef TEST_ONE + + return ret; +} + + static int mymain(void) { @@ -493,6 +582,92 @@ mymain(void) TEST_REPLACE("fooooofoooo", "foo", "barwizzeek", "barwizzeekooobarwizzeekoo"); TEST_REPLACE("fooooofoooo", "foooo", "foo", "fooofoo"); +#define TEST_STRTOL(str, suff, i, i_ret, u, u_ret, \ + ll, ll_ret, ull, ull_ret) \ + do { \ + struct stringToLongData data = { \ + str, suff, i, i_ret, u, u_ret, ll, ll_ret, ull, ull_ret, \ + }; \ + if (virtTestRun("virStringToLong '" str "'", testStringToLong, \ + &data) < 0) \ + ret = -1; \ + } while (0) + + /* Start simple */ + TEST_STRTOL("0", NULL, 0, 0, 0U, 0, 0LL, 0, 0ULL, 0); + + /* All your base are belong to us */ + TEST_STRTOL("0x0", NULL, 0, 0, 0U, 0, 0LL, 0, 0ULL, 0); + TEST_STRTOL("0XaB", NULL, 171, 0, 171U, 0, 171LL, 0, 171ULL, 0); + TEST_STRTOL("010", NULL, 8, 0, 8U, 0, 8LL, 0, 8ULL, 0); + + /* Suffix handling */ + TEST_STRTOL("42", NULL, 42, 0, 42U, 0, 42LL, 0, 42ULL, 0); + TEST_STRTOL("42", "", 42, 0, 42U, 0, 42LL, 0, 42ULL, 0); + TEST_STRTOL("42.", NULL, 0, -1, 0U, -1, 0LL, -1, 0ULL, -1); + TEST_STRTOL("42.", ".", 42, 0, 42U, 0, 42LL, 0, 42ULL, 0); + + /* Blatant invalid input */ + TEST_STRTOL("", "", 0, -1, 0U, -1, 0LL, -1, 0ULL, -1); + TEST_STRTOL("", NULL, 0, -1, 0U, -1, 0LL, -1, 0ULL, -1); + TEST_STRTOL(" ", " ", 0, -1, 0U, -1, 0LL, -1, 0ULL, -1); + TEST_STRTOL(" ", NULL, 0, -1, 0U, -1, 0LL, -1, 0ULL, -1); + TEST_STRTOL(" -", " -", 0, -1, 0U, -1, 0LL, -1, 0ULL, -1); + TEST_STRTOL(" -", NULL, 0, -1, 0U, -1, 0LL, -1, 0ULL, -1); + TEST_STRTOL("a", "a", 0, -1, 0U, -1, 0LL, -1, 0ULL, -1); + TEST_STRTOL("a", NULL, 0, -1, 0U, -1, 0LL, -1, 0ULL, -1); + + /* Not a hex number, but valid when suffix expected */ + TEST_STRTOL(" 0x", NULL, 0, -1, 0U, -1, 0LL, -1, 0ULL, -1); + TEST_STRTOL(" 0x", "x", 0, 0, 0U, 0, 0LL, 0, 0ULL, 0); + + /* Upper bounds */ + TEST_STRTOL("2147483647", NULL, 2147483647, 0, 2147483647U, 0, + 2147483647LL, 0, 2147483647ULL, 0); + TEST_STRTOL("2147483648", NULL, 0, -1, 2147483648U, 0, + 2147483648LL, 0, 2147483648ULL, 0); + TEST_STRTOL("4294967295", NULL, 0, -1, 4294967295U, 0, + 4294967295LL, 0, 4294967295ULL, 0); + TEST_STRTOL("4294967296", NULL, 0, -1, 0U, -1, + 4294967296LL, 0, 4294967296ULL, 0); + TEST_STRTOL("9223372036854775807", NULL, 0, -1, 0U, -1, + 9223372036854775807LL, 0, 9223372036854775807ULL, 0); + TEST_STRTOL("9223372036854775808", NULL, 0, -1, 0U, -1, + 0LL, -1, 9223372036854775808ULL, 0); + TEST_STRTOL("18446744073709551615", NULL, 0, -1, 0U, -1, + 0LL, -1, 18446744073709551615ULL, 0); + TEST_STRTOL("18446744073709551616", NULL, 0, -1, 0U, -1, + 0LL, -1, 0ULL, -1); + TEST_STRTOL("18446744073709551616", "", 0, -1, 0U, -1, + 0LL, -1, 0ULL, -1); + + /* Negative bounds */ + TEST_STRTOL("-0", NULL, 0, 0, 0U, 0, 0LL, 0, 0ULL, 0); + TEST_STRTOL("-1", "", -1, 0, 4294967295U, 0, + -1LL, 0, 18446744073709551615ULL, 0); + TEST_STRTOL("-2147483647", NULL, -2147483647, 0, 2147483649U, 0, + -2147483647LL, 0, 18446744071562067969ULL, 0); + TEST_STRTOL("-2147483648", NULL, -2147483648, 0, 2147483648U, 0, + -2147483648LL, 0, 18446744071562067968ULL, 0); + TEST_STRTOL("-2147483649", NULL, 0, -1, 2147483647U, 0, + -2147483649LL, 0, 18446744071562067967ULL, 0); + TEST_STRTOL("-4294967295", NULL, 0, -1, 1U, 0, + -4294967295LL, 0, 18446744069414584321ULL, 0); + TEST_STRTOL("-4294967296", NULL, 0, -1, 0U, -1, + -4294967296LL, 0, 18446744069414584320ULL, 0); + TEST_STRTOL("-9223372036854775807", NULL, 0, -1, 0U, -1, + -9223372036854775807LL, 0, 9223372036854775809ULL, 0); + /* Bah, stupid gcc warning about -9223372036854775808LL being an + * unrepresentable integer constant */ + TEST_STRTOL("-9223372036854775808", NULL, 0, -1, 0U, -1, + 0x8000000000000000LL, 0, 9223372036854775808ULL, 0); + TEST_STRTOL("-9223372036854775809", NULL, 0, -1, 0U, -1, + 0LL, -1, 9223372036854775807ULL, 0); + TEST_STRTOL("-18446744073709551615", NULL, 0, -1, 0U, -1, + 0LL, -1, 1ULL, 0); + TEST_STRTOL("-18446744073709551616", NULL, 0, -1, 0U, -1, + 0LL, -1, 0ULL, -1); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.9.0
strtoul() is required to parse negative numbers as their twos-complement positive counterpart. But sometimes we want to reject negative numbers. Add new functions to do this. * src/util/virstring.h (virStrToLong_uip, virStrToLong_ulp) (virStrToLong_ullp): New prototypes. * src/util/virstring.c (virStrToLong_uip, virStrToLong_ulp) (virStrToLong_ullp): New functions. * src/libvirt_private.syms (virstring.h): Export them. * tests/virstringtest.c (testStringToLong): Test them. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt_private.syms | 3 ++ src/util/virstring.c | 80 +++++++++++++++++++++++++++++++++++++++++++++--- src/util/virstring.h | 14 ++++++++- tests/virstringtest.c | 23 ++++++++++++++ 4 files changed, 115 insertions(+), 5 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4cfaefc..cc3ae2c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1899,8 +1899,11 @@ virStrToLong_i; virStrToLong_l; virStrToLong_ll; virStrToLong_ui; +virStrToLong_uip; virStrToLong_ul; virStrToLong_ull; +virStrToLong_ullp; +virStrToLong_ulp; virTrimSpaces; virVasprintfInternal; diff --git a/src/util/virstring.c b/src/util/virstring.c index b5fc638..7a8430e 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -211,7 +211,9 @@ virStrToLong_i(char const *s, char **end_ptr, int base, int *result) return 0; } -/* Just like virStrToLong_i, above, but produce an "unsigned int" value. */ +/* Just like virStrToLong_i, above, but produce an "unsigned int" + * value. This version allows twos-complement wraparound of negative + * numbers. */ int virStrToLong_ui(char const *s, char **end_ptr, int base, unsigned int *result) { @@ -242,6 +244,27 @@ virStrToLong_ui(char const *s, char **end_ptr, int base, unsigned int *result) return 0; } +/* Just like virStrToLong_i, above, but produce an "unsigned int" + * value. This version rejects any negative signs. */ +int +virStrToLong_uip(char const *s, char **end_ptr, int base, unsigned int *result) +{ + unsigned long int val; + char *p; + bool err = false; + + errno = 0; + val = strtoul(s, &p, base); /* exempt from syntax-check */ + err = (memchr(s, '-', p - s) || + errno || (!end_ptr && *p) || p == s || (unsigned int) val != val); + if (end_ptr) + *end_ptr = p; + if (err) + return -1; + *result = val; + return 0; +} + /* Just like virStrToLong_i, above, but produce a "long" value. */ int virStrToLong_l(char const *s, char **end_ptr, int base, long *result) @@ -261,7 +284,9 @@ virStrToLong_l(char const *s, char **end_ptr, int base, long *result) return 0; } -/* Just like virStrToLong_i, above, but produce an "unsigned long" value. */ +/* Just like virStrToLong_i, above, but produce an "unsigned long" + * value. This version allows twos-complement wraparound of negative + * numbers. */ int virStrToLong_ul(char const *s, char **end_ptr, int base, unsigned long *result) { @@ -280,6 +305,28 @@ virStrToLong_ul(char const *s, char **end_ptr, int base, unsigned long *result) return 0; } +/* Just like virStrToLong_i, above, but produce an "unsigned long" + * value. This version rejects any negative signs. */ +int +virStrToLong_ulp(char const *s, char **end_ptr, int base, + unsigned long *result) +{ + unsigned long int val; + char *p; + int err; + + errno = 0; + val = strtoul(s, &p, base); /* exempt from syntax-check */ + err = (memchr(s, '-', p - s) || + errno || (!end_ptr && *p) || p == s); + if (end_ptr) + *end_ptr = p; + if (err) + return -1; + *result = val; + return 0; +} + /* Just like virStrToLong_i, above, but produce a "long long" value. */ int virStrToLong_ll(char const *s, char **end_ptr, int base, long long *result) @@ -299,9 +346,12 @@ virStrToLong_ll(char const *s, char **end_ptr, int base, long long *result) return 0; } -/* Just like virStrToLong_i, above, but produce an "unsigned long long" value. */ +/* Just like virStrToLong_i, above, but produce an "unsigned long + * long" value. This version allows twos-complement wraparound of + * negative numbers. */ int -virStrToLong_ull(char const *s, char **end_ptr, int base, unsigned long long *result) +virStrToLong_ull(char const *s, char **end_ptr, int base, + unsigned long long *result) { unsigned long long val; char *p; @@ -318,6 +368,28 @@ virStrToLong_ull(char const *s, char **end_ptr, int base, unsigned long long *re return 0; } +/* Just like virStrToLong_i, above, but produce an "unsigned long + * long" value. This version rejects any negative signs. */ +int +virStrToLong_ullp(char const *s, char **end_ptr, int base, + unsigned long long *result) +{ + unsigned long long val; + char *p; + int err; + + errno = 0; + val = strtoull(s, &p, base); /* exempt from syntax-check */ + err = (memchr(s, '-', p - s) || + errno || (!end_ptr && *p) || p == s); + if (end_ptr) + *end_ptr = p; + if (err) + return -1; + *result = val; + return 0; +} + int virStrToDouble(char const *s, char **end_ptr, diff --git a/src/util/virstring.h b/src/util/virstring.h index 5b77581..1ed1046 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2007-2012 Red Hat, Inc. + * Copyright (C) 2007-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -50,6 +50,10 @@ int virStrToLong_ui(char const *s, char **end_ptr, int base, unsigned int *result); +int virStrToLong_uip(char const *s, + char **end_ptr, + int base, + unsigned int *result); int virStrToLong_l(char const *s, char **end_ptr, int base, @@ -58,6 +62,10 @@ int virStrToLong_ul(char const *s, char **end_ptr, int base, unsigned long *result); +int virStrToLong_ulp(char const *s, + char **end_ptr, + int base, + unsigned long *result); int virStrToLong_ll(char const *s, char **end_ptr, int base, @@ -66,6 +74,10 @@ int virStrToLong_ull(char const *s, char **end_ptr, int base, unsigned long long *result); +int virStrToLong_ullp(char const *s, + char **end_ptr, + int base, + unsigned long long *result); int virStrToDouble(char const *s, char **end_ptr, double *result); diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 0380df1..1e330f9 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -408,6 +408,13 @@ testStringToLong(const void *opaque) char *end; long l; unsigned long ul; + bool negative; + + if (data->suffix) + negative = !!memchr(data->str, '-', + strlen(data->str) - strlen(data->suffix)); + else + negative = !!strchr(data->str, '-'); #define TEST_ONE(Str, Suff, Type, Fn, Fmt, Exp, Exp_ret) \ do { \ @@ -441,6 +448,11 @@ testStringToLong(const void *opaque) data->si, data->si_ret); TEST_ONE(data->str, data->suffix, unsigned int, ui, "%u", data->ui, data->ui_ret); + if (negative) + TEST_ONE(data->str, data->suffix, unsigned int, uip, "%u", 0U, -1); + else + TEST_ONE(data->str, data->suffix, unsigned int, uip, "%u", + data->ui, data->ui_ret); /* We hate adding new API with 'long', and prefer 'int' or 'long * long' instead, since platform-specific results are evil */ @@ -450,11 +462,22 @@ testStringToLong(const void *opaque) ul = (sizeof(int) == sizeof(long)) ? data->ui : data->ull; TEST_ONE(data->str, data->suffix, unsigned long, ul, "%lu", ul, (sizeof(int) == sizeof(long)) ? data->ui_ret : data->ull_ret); + if (negative) + TEST_ONE(data->str, data->suffix, unsigned long, ulp, "%lu", 0UL, -1); + else + TEST_ONE(data->str, data->suffix, unsigned long, ulp, "%lu", ul, + (sizeof(int) == sizeof(long)) ? data->ui_ret : data->ull_ret); TEST_ONE(data->str, data->suffix, long long, ll, "%lld", data->ll, data->ll_ret); TEST_ONE(data->str, data->suffix, unsigned long long, ull, "%llu", data->ull, data->ull_ret); + if (negative) + TEST_ONE(data->str, data->suffix, unsigned long long, ullp, "%llu", + 0ULL, -1); + else + TEST_ONE(data->str, data->suffix, unsigned long long, ullp, "%llu", + data->ull, data->ull_ret); #undef TEST_ONE -- 1.9.0
Commit f22b7899 stumbled across a difference between 32-bit and 64-bit platforms when parsing "-1" as an int. Now that we've fixed that difference, it's time to fix the testsuite. * src/util/virstoragefile.c (virStorageFileParseChainIndex): Require a positive index. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 5c43665..6870ae7 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1525,7 +1525,7 @@ virStorageFileParseChainIndex(const char *diskTarget, if (virStringListLength(strings) != 2) goto cleanup; - if (virStrToLong_ui(strings[1], &suffix, 10, &idx) < 0 || + if (virStrToLong_uip(strings[1], &suffix, 10, &idx) < 0 || STRNEQ(suffix, "]")) goto cleanup; -- 1.9.0
On 04/30/2014 10:27 PM, Eric Blake wrote:
v1 was here, but it got nacked for being ugly: https://www.redhat.com/archives/libvir-list/2014-April/msg01132.html
so in this version, I improved virstring first, then used the new clean function.
This is a build-breaker fix, so it deserves to be in 1.2.4; but it is large enough that it needs a review to go in.
Eric Blake (3): util: fix uint parsing on 64-bit platforms util: new stricter unsigned int parsing storage: reject negative indices
src/libvirt_private.syms | 3 + src/util/virstoragefile.c | 2 +- src/util/virstring.c | 98 +++++++++++++++++++++-- src/util/virstring.h | 14 +++- tests/virstringtest.c | 200 +++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 307 insertions(+), 10 deletions(-)
In short ACK... Since I believe this resolves the issue seen. Oh the games we have to play because 32 (and 16) bit computing still exists... I've looked at the changes... Ran them thru coverity... Things seem OK to me. Although we may have some work to do to check users of the existing API's... Having to "read minds" about what someone "really" meant when they used -1 in a CLI/API can be dangerous. Was using -1 meant to produce an error or was it meant to be a shortcut for a largest value or was it a result of using a 64 bit value in order to manipulate a 32 bit number? If you know the underlying code doesn't accept (or want) negative numbers, then -1 would be bad. My favorite is of course finding those places where someone uses ~0ULL (or some variant). Having to ensure/describe what each datum is for each API/CLI reference could be a time consuming excursion through a lot of code in order to determine and validate what "should" and/or "could" be passed. The other side of the coin is - what happens on display if say someone indicates -1 on input (for whatever reason) - does the display side then display the maximum whatever value or does it (or could/should it) display the -1? One example that comes to mind (because of a recent changes (c4206d7 and dff3ad0) that affected virt-test/tp-libvirt results) is from migrate-setspeed where the maximum value allowed is INT64_MAX (although it's not documented as that in the man page)... Although the recent changes don't allow passing -1 thru the virsh command - when they did - the output on a getspeed would be the 1844*1615 value (UINT64_MAX). The bz associated indicated use of -1 by the tester in order to force use of the maximum value, which is where it caught my attention as being somewhat related to this particular change... John
On 05/01/2014 02:41 PM, John Ferlan wrote:
On 04/30/2014 10:27 PM, Eric Blake wrote:
v1 was here, but it got nacked for being ugly: https://www.redhat.com/archives/libvir-list/2014-April/msg01132.html
so in this version, I improved virstring first, then used the new clean function.
This is a build-breaker fix, so it deserves to be in 1.2.4; but it is large enough that it needs a review to go in.
Eric Blake (3): util: fix uint parsing on 64-bit platforms util: new stricter unsigned int parsing storage: reject negative indices
src/libvirt_private.syms | 3 + src/util/virstoragefile.c | 2 +- src/util/virstring.c | 98 +++++++++++++++++++++-- src/util/virstring.h | 14 +++- tests/virstringtest.c | 200 +++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 307 insertions(+), 10 deletions(-)
In short ACK... Since I believe this resolves the issue seen.
Thanks; series pushed.
Oh the games we have to play because 32 (and 16) bit computing still exists... I've looked at the changes... Ran them thru coverity... Things seem OK to me. Although we may have some work to do to check users of the existing API's...
I'm glad it passed Coverity, with no warnings about dead code due to an impossible comparison (to be honest, I was a bit worried that it might have complained about 'long > UINT_MAX' always being false and thus dead code on 32-bit platforms; even though it can obviously be true on a 64-bit platform).
Having to "read minds" about what someone "really" meant when they used -1 in a CLI/API can be dangerous. Was using -1 meant to produce an error or was it meant to be a shortcut for a largest value or was it a result of using a 64 bit value in order to manipulate a 32 bit number? If you know the underlying code doesn't accept (or want) negative numbers, then -1 would be bad. My favorite is of course finding those places where someone uses ~0ULL (or some variant).
Having to ensure/describe what each datum is for each API/CLI reference could be a time consuming excursion through a lot of code in order to determine and validate what "should" and/or "could" be passed.
Yeah, there's probably other places in libvirt that _should_ use the new _uip variants to force a positive parse; vs. those that have already accepted a -1 parse and where back-compat says we can't go back. I only focused on getting the framework in (now the caller can choose which of the two semantics they want) and fixing the one place where the testsuite proved that we wanted the positive-only semantics.
The other side of the coin is - what happens on display if say someone indicates -1 on input (for whatever reason) - does the display side then display the maximum whatever value or does it (or could/should it) display the -1?
Yeah, we've fought that battle as well. For example, we WANT the XML to express '(uid_t)-1' as -1, and NOT as the maximum number; but have had mishaps in the past where we exposed the maximum number so now the code has hacks to support both spellings. I won't be surprised if the battle resurfaces in the future; but at least now we have a framework to make it easier.
One example that comes to mind (because of a recent changes (c4206d7 and dff3ad0) that affected virt-test/tp-libvirt results) is from migrate-setspeed where the maximum value allowed is INT64_MAX (although it's not documented as that in the man page)... Although the recent changes don't allow passing -1 thru the virsh command - when they did - the output on a getspeed would be the 1844*1615 value (UINT64_MAX). The bz associated indicated use of -1 by the tester in order to force use of the maximum value, which is where it caught my attention as being somewhat related to this particular change...
John
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Apr 30, 2014 at 08:27:59PM -0600, Eric Blake wrote:
v1 was here, but it got nacked for being ugly: https://www.redhat.com/archives/libvir-list/2014-April/msg01132.html
so in this version, I improved virstring first, then used the new clean function.
This is a build-breaker fix, so it deserves to be in 1.2.4; but it is large enough that it needs a review to go in.
Build is back to green, thanks! http://honk.sigxcpu.org:8001/job/libvirt-check/2249/consoleFull Cheers, -- Guido
participants (3)
-
Eric Blake -
Guido Günther -
John Ferlan