From f18c02ec221a5b3ef91f7106d07fa3957a941c6e Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 30 Apr 2014 14:46:18 -0600 Subject: [PATCH] util: fix uint parsing on 64-bit platforms 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 --- 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 64c7259750..b5fc6380f6 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 a4ae966012..0380df136e 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -23,6 +23,8 @@ #include #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; }