Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2183 bytes --]

On 05/09/2013 08:20 PM, Michael Roth wrote:
> Currently our JSON parser assumes that numbers lacking a mantissa are
> integers and attempts to store them as QInt/int64 values. This breaks in
> the case where the number overflows/underflows int64 values (which is
> still valid JSON)
> 
> Fix this by detecting such cases and using a QFloat to store the value
> instead.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  qobject/json-parser.c |   26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)

This changes the error message handed back to QMP clients, and possibly
exposes problems in other qemu code that receives the result of json
parses.  Previously, for an 'int' argument, if you passed in a too-large
number, you got an error that the argument was too large for int.  Now,
the number is accepted as a double; are we guaranteed that in a context
that expects a qint, when that code is now handed a qfloat (a case which
was previously impossible because qint_from_int protected it), that the
code will still behave correctly?

At any rate, libvirt already checks that all numbers that fall outside
the range of int64_t are never passed over qmp when passing an int
argument (and yes, this is annoying, in that large 64-bit unsigned
numbers have to be passed as negative numbers, rather than exceeding
INT64_MAX), so libvirt should not be triggering this newly exposed code
path.  But even if libvirt doesn't plan on triggering it, I'd still feel
better if your commit message documented evidence of testing what
happens in this case.  For example, compare what
{"execute":"add-fd","arguments":{"fdset-id":"99999999999999999999"}}
does before and after this patch.

> +        /* fall through to JSON_FLOAT */
> +    }
>      case JSON_FLOAT:
>          /* FIXME dependent on locale */

You know, strtoll() also is dependent on locale (that is, non-C locale
can accept strings that the C locale rejects), if you want to repeat
this comment earlier for case JSON_INTEGER.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]