Libguestfs - Feb 2019 - [PATCH nbdkit v2] server: utils: Make nbdkit_parse_size to reject negative values

From: Nikolay Ivanets <stenavin@gmail.com>

nbdkit_parse_size() uses strtoumax() function to parse input strings
which states:

"if there was a leading minus sign, the negation of the result of the
conversion represented as an unsigned value, unless the original
(nonnegated) value would overflow."

Later validation doesn't catch the situation when parsed value appeared
within the valid range (due to negation) but original string passed to a
function literally represented negative number.

Thus nbdkit_parse_size() treats negative values in a range [-UINT64_MAX;
INT64_MIN - 1] as positive values in a range [1; INT64_MAX] due to
negation performed by strtoumax().

In this patch:

1. strtoimax() is used instead of strtoumax() to reflect the nature of
the 'size' which cannot be negative and is not expected to exceed
INT64_MAX.

2. Error reporting separates cases where the string is parsed to a
negative value or parsed value overflows (greater then INT64_MAX).

Tests:

1. Some more tests were added to cover described behaviour.

2. Input strings where grouped into a set which lead to
valid/invalid/negative/overflow result.

3. Some strings with a leading '+' sign were added.
---
 server/test-utils.c | 28 ++++++++++++++++++++++++----
 server/utils.c      | 19 ++++++++++++++-----
 2 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/server/test-utils.c b/server/test-utils.c
index f51f63a..3965fd3 100644
--- a/server/test-utils.c
+++ b/server/test-utils.c
@@ -62,14 +62,26 @@ test_nbdkit_parse_size (void)
     { "0x0", -1 },
     { "garbage", -1 },
     { "0garbage", -1 },
-    { "-1", -1 },
-    { "-2", -1 },
-    { "9223372036854775808", -1 },
-    { "-9223372036854775808", -1 },
     { "8E", -1 },
     { "8192P", -1 },
+
+    /* Strings leading to overflow */
+    { "9223372036854775808", -1 }, /* INT_MAX + 1 */
+    { "18446744073709551614", -1 }, /* UINT64_MAX - 1 */
+    { "18446744073709551615", -1 }, /* UINT64_MAX */
+    { "18446744073709551616", -1 }, /* UINT64_MAX + 1 */
     { "999999999999999999999999", -1 },
 
+    /* Strings representing negative values */
+    { "-1", -1 },
+    { "-2", -1 },
+    { "-9223372036854775809", -1 }, /* INT64_MIN - 1 */
+    { "-9223372036854775808", -1 }, /* INT64_MIN */
+    { "-9223372036854775807", -1 }, /* INT64_MIN + 1 */
+    { "-18446744073709551616", -1 }, /* -UINT64_MAX - 1 */
+    { "-18446744073709551615", -1 }, /* -UINT64_MAX */
+    { "-18446744073709551614", -1 }, /* -UINT64_MAX + 1 */
+
     /* Strings we may want to support in the future */
     { "M", -1 },
     { "1MB", -1 },
@@ -77,8 +89,14 @@ test_nbdkit_parse_size (void)
     { "1.5M", -1 },
 
     /* Valid strings */
+    { "-0", 0 },
     { "0", 0 },
+    { "+0", 0 },
     { " 08", 8 },
+    { "1", 1 },
+    { "+1", 1 },    
+    { "1234567890", 1234567890 },
+    { "+1234567890", 1234567890 },    
     { "9223372036854775807", INT64_MAX },
     { "1s", 512 },
     { "2S", 1024 },
@@ -88,12 +106,14 @@ test_nbdkit_parse_size (void)
     { "1K", 1024 },
     { "1m", 1024 * 1024 },
     { "1M", 1024 * 1024 },
+    { "+1M", 1024 * 1024 },    
     { "1g", 1024 * 1024 * 1024 },
     { "1G", 1024 * 1024 * 1024 },
     { "1t", 1024LL * 1024 * 1024 * 1024 },
     { "1T", 1024LL * 1024 * 1024 * 1024 },
     { "1p", 1024LL * 1024 * 1024 * 1024 * 1024 },
     { "1P", 1024LL * 1024 * 1024 * 1024 * 1024 },
+    { "8191p", 1024LL * 1024 * 1024 * 1024 * 1024 * 8191 },
     { "1e", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 },
     { "1E", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 },
   };
diff --git a/server/utils.c b/server/utils.c
index 18011fd..6feb564 100644
--- a/server/utils.c
+++ b/server/utils.c
@@ -88,21 +88,30 @@ nbdkit_absolute_path (const char *path)
 int64_t
 nbdkit_parse_size (const char *str)
 {
-  uint64_t size;
+  int64_t size;
   char *end;
   uint64_t scale = 1;
 
-  /* Disk sizes cannot usefully exceed off_t (which is signed), so
-   * scan unsigned, then range check later that result fits.  */
+  /* Disk sizes cannot usefully exceed off_t (which is signed) and
+   * cannot be negative.  */
   /* XXX Should we also parse things like '1.5M'? */
   /* XXX Should we allow hex? If so, hex cannot use scaling suffixes,
    * because some of them are valid hex digits */
   errno = 0;
-  size = strtoumax (str, &end, 10);
-  if (errno || str == end) {
+  size = strtoimax (str, &end, 10);
+  if (str == end) {
     nbdkit_error ("could not parse size string (%s)", str);
     return -1;
   }
+  if (size < 0) {
+    nbdkit_error ("size cannot be negative (%s)", str);
+    return -1;
+  }
+  if (errno) {
+    nbdkit_error ("size (%s) exceeds maximum value", str);
+    return -1;
+  }
+
   switch (*end) {
     /* No suffix */
   case '\0':
-- 
2.17.2

Looks OK to me.  I'll let Eric have the final say though.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On 2/8/19 12:10 AM, Mykola Ivanets wrote:
> From: Nikolay Ivanets <stenavin@gmail.com>
Grammar in the subject: s/ to//
> 
> nbdkit_parse_size() uses strtoumax() function to parse input strings
> which states:
> 
> 1. Some more tests were added to cover described behaviour.
> 
> 2. Input strings where grouped into a set which lead to
> valid/invalid/negative/overflow result.
> 
> 3. Some strings with a leading '+' sign were added.
> ---
>    errno = 0;
> -  size = strtoumax (str, &end, 10);
> -  if (errno || str == end) {
> +  size = strtoimax (str, &end, 10);
> +  if (str == end) {
>      nbdkit_error ("could not parse size string (%s)", str);
>      return -1;
>    }
> +  if (size < 0) {
> +    nbdkit_error ("size cannot be negative (%s)", str);
> +    return -1;
> +  }
> +  if (errno) {
> +    nbdkit_error ("size (%s) exceeds maximum value", str);
> +    return -1;
> +  }
On underflow, strtoimax returns INTMAX_MIN and sets ERANGE; which favors
a message about negative values over a message about overflow. Swapping
the errno message first would also work, but I'm fine with your approach.

Thanks; pushed!

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org