9:27 p.m.
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
9:28 p.m.
New subject: [libvirt] [PATCHv2 1/3] 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 <eblake(a)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
9:28 p.m.
New subject: [libvirt] [PATCHv2 2/3] util: new stricter unsigned int parsing
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(a)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
9:28 p.m.
New subject: [libvirt] [PATCHv2 3/3] storage: reject negative indices
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(a)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
3:41 p.m.
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
4:10 p.m.
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
4:03 a.m.
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
3887
days inactive
3888
days old
6 comments
3 participants
participants (3)
-
Eric Blake -
Guido Günther -
John Ferlan