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

[mdroth <mdroth@linux.vnet.ibm.com>]

On Fri, May 10, 2013 at 02:47:18PM +0200, Laszlo Ersek wrote:
> 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 <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?
> 
> 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.
> 

This is correct, but it does make code acting on the resulting QObject
capable of checking for this scenario and acting accordingly in whatever
manner is appropriate. I think that's appropriate, since there's no error
as far as the JSON spec goes, but further up the stack we may have stricter
requirements on what the valid ranges are for various fields.

For instance, in the case of QmpInputVisitor, these cases will
trigger an error case that was previously being bypassed when
out-of-range values were provided in place of 'int' due to the parser
silently capping the max/min values:

in qmp_input_type_int():

    QObject *qobj = qmp_input_get_object(qiv, name);

    if (!qobj || qobject_type(qobj) != QTYPE_QINT) {
        error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                  "integer");
        return;
    }

We'd now trigger this error for such inputs (since we'd have QTYPE_QFLOAT),
which is stricter checking than before, but as intended since we map int to
int64_t and expect values in that range (though we could stand to be clearer
about this in the QAPI documentation)

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

That would trigger the error mentioned above, as opposed to silently changing
the value to LLONG_MAX. I'll see about covering this case somewhere in
tests/test-qmp-input-visitor.

> 
> Laszlo
>