From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48164) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uamhj-0003FL-L8 for qemu-devel@nongnu.org; Fri, 10 May 2013 08:45:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uamhc-0001H6-Vg for qemu-devel@nongnu.org; Fri, 10 May 2013 08:45:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53538) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uamhc-0001Gg-Lc for qemu-devel@nongnu.org; Fri, 10 May 2013 08:45:36 -0400 Message-ID: <518CEC56.7070602@redhat.com> Date: Fri, 10 May 2013 14:47:18 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1368152462-13219-1-git-send-email-mdroth@linux.vnet.ibm.com> <1368152462-13219-7-git-send-email-mdroth@linux.vnet.ibm.com> <518CE683.4000600@redhat.com> In-Reply-To: <518CE683.4000600@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 06/10] json-parser: fix handling of large whole number values List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: lcapitulino@redhat.com, akong@redhat.com, Michael Roth , qemu-devel@nongnu.org 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 >> --- >> 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