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: mdroth@loki:~/w/qemu.git$ ack-grep qobject_from_json | grep "%f" tests/check-qjson.c:987: obj = qobject_from_jsonf("%f", valuef); mdroth@loki:~/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 >