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

[Laszlo Ersek]

On 05/10/13 14:22, Eric Blake wrote:
> 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 <address@hidden>
>> ---
>>  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?

I tried to consider this while reviewing... Maybe I was wrong.

The pre-patch code for JSON_INTEGER:

obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 10)));

doesn't check for errors at all. (I assume that JSON_INTEGER is selected
by the parser, token_get_type(), based on syntax purely.)

I thought when the pre-patch version encounters an int-looking decimal
string that's actually too big in magnitude for an int, you'd simply end
up with LLONG_MIN or LLONG_MAX, but no error. strtoll() clamps the
value, errno is lost, and qint_from_int() sees nothing wrong.

With the patch, you end up with a float instead of an int-typed
LLONG_MIN/LLONG_MAX, and also no error.

> 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.

That would be likely interesting to test, yes.

Laszlo