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

[mdroth]

On Fri, May 10, 2013 at 08:08:05AM -0600, Eric Blake wrote:
> 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.

Yup, new error cases can be triggered, but in the case of
QmpInputVisitor this is handled appropriately (will add a test case to
confirm), and none of our other input visitors act on QObjects, and this
ambiguity isn't present for output visitors.

We also have monitor events that call qobject_from_json() to marshall
event payloads, but these are essentially open-coded QmpInputVisitors
where the JSON values come from native C types. The only case where I
can see this triggering the change is if they did something like:

  obj = qobject_from_jsonf("{'myInt': %f}", whole_valued_float);

which would be evil, and thankfully such cases don't appear to exist:

address@hidden:~/w/qemu.git$ ack-grep qobject_from_json | grep "%f"
tests/check-qjson.c:987:    obj = qobject_from_jsonf("%f", valuef);
address@hidden:~/w/qemu.git$

(the 'valuef' above is not whole-valued, and the output is expected to
be a QFloat)

I'm not aware of any other cases to consider, but I might've missed
something.

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

Command params will end up getting marshalled in QObject prior to being
passed into commands:

    mi = qmp_input_visitor_new_strict(QOBJECT(args));
    v = qmp_input_get_visitor(mi);
    visit_start_optional(v, &has_fdset_id, "fdset-id", errp);
    if (has_fdset_id) {
        visit_type_int(v, &fdset_id, "fdset-id", errp);
    }
    visit_end_optional(v, errp);
    visit_start_optional(v, &has_opaque, "opaque", errp);
    if (has_opaque) {
        visit_type_str(v, &opaque, "opaque", errp);
    }
    visit_end_optional(v, errp);
    qmp_input_visitor_cleanup(mi);

    if (error_is_set(errp)) {
        goto out;
    }
    retval = qmp_add_fd(has_fdset_id, fdset_id, has_opaque, opaque, errp);

so i think a check in tests-qmp-input-visitor that verifies that values that
exceed LLONG_MAX/LLONG_MIN will get added into the QObject as QFloats
and trigger a type error when being passed to visit_type_int() should
cover the cases in question.

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