[Libguestfs] [PATCH nbdkit] server: utils: Fix nbdkit_parse_size to correctly handle negative values

[Eric Blake]

On 2/7/19 7:21 AM, Mykola Ivanets wrote:
> From: Nikolay Ivanets <stenavin at 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 is
> within the valid range but original string passed to a function
> represented negative number.

In other words, you don't like -18446744073709551615 being parsed
successfully as '1', and instead want it to cause an error because of
the leading '-'.  Seems reasonable.

> 
> Thus nbdkit_parse_size() incorrectly parsed values in a range
> [-UINT64_MAX; INT64_MIN -1] translating them into an unsigned range
> of a valid values: [1; INT64_MAX].

I wouldn't call that incorrect, so much as being lenient in what we
accept, but I'm not opposed to taking the patch.

> 
> In this patch:
> 
> 1. strtoll() is used instead of strtoumax() to reflect the nature
> of the 'size' which cannot be negative and isn't expected to exceed
> INT64_MAX.

Rather, I'd go with strtoimax(), not strtoll().  It may be the same
function under the hood on current platforms, but future standards may
have types longer than 'long long' which we may still want to support.

> 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 found bug.
> 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(-)
> 

> +++ 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 = strtoll (str, &end, 10);

strtoimax() seems better here.

> +  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 exceeds maximum value (%s)", str);

This reads awkwardly - it looks like the value in () is the maximum;
better might be: "size %s exceeds maximum value".

> +    return -1;
> +  }
> +
>    switch (*end) {
>      /* No suffix */
>    case '\0':
> 

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190207/4304b8d2/attachment.sig>