[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values |
Date: |
Fri, 10 May 2013 14:47:18 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 |
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
- Re: [Qemu-devel] [PATCH 01/10] qapi: qapi-types.py, native list support, (continued)
- [Qemu-devel] [PATCH 02/10] qapi: qapi-visit.py, fix list handling for union types, Michael Roth, 2013/05/09
- [Qemu-devel] [PATCH 03/10] qapi: qapi-visit.py, native list support, Michael Roth, 2013/05/09
- [Qemu-devel] [PATCH 04/10] qapi: enable generation of native list code, Michael Roth, 2013/05/09
- [Qemu-devel] [PATCH 05/10] qapi: fix leak in unit tests, Michael Roth, 2013/05/09
- [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values, Michael Roth, 2013/05/09
- Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values, Luiz Capitulino, 2013/05/10
[Qemu-devel] [PATCH 07/10] qapi: fix visitor serialization tests for numbers/doubles, Michael Roth, 2013/05/09
[Qemu-devel] [PATCH 08/10] qapi: add native list coverage for visitor serialization tests, Michael Roth, 2013/05/09
[Qemu-devel] [PATCH 09/10] qapi: add native list coverage for QMP output visitor tests, Michael Roth, 2013/05/09
[Qemu-devel] [PATCH 10/10] qapi: add native list coverage for QMP input visitor tests, Michael Roth, 2013/05/09
Re: [Qemu-devel] [PATCH v2 00/10] qapi: add support for lists of native types, Luiz Capitulino, 2013/05/10