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: 2659 bytes --]

On 05/10/2013 06:47 AM, Laszlo Ersek wrote:

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

Oh, right.  _That's_ why libvirt had to add checks that it wasn't
passing 0x8000000000000000ULL as a positive number - because the qemu
parser was silently clamping it to 0x7fffffffffffffffLL, which is not
what libvirt wanted.  So the code was NOT erroring out with an overflow
message, but was acting on the wrong integer.

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

Ah, but here we have a difference - beforehand, the code was passing a
valid (albeit wrong value) qint, so the rest of the qemu code was
oblivious to the fact that the QMP message contained an overflow.  But
now the code is passing a qdouble, and the rest of the qemu code may be
unprepared to handle it when expecting a qint.

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

add-fd may not be the best candidate (it expects an fd to be passed at
the same time, and does its own checking that it does not get a negative
number); but I'm sure there's plenty of other candidates (add-cpu is
another possibility that comes quickly to mind) - basically, pick a
command that takes an explicit 'int' argument, and overflow that
argument to see what happens when the command now has to deal with a
qdouble.

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