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

[mdroth]

On Fri, May 10, 2013 at 11:17:17AM -0400, Luiz Capitulino wrote:
> On Thu,  9 May 2013 21:20:58 -0500
> Michael Roth <address@hidden> 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)
> 
> Anthony wanted to fix this by moving to another wire format :)
> 
> But, how this patch related to this series?

In v1 Laszlo pointed out that the QFloat tests I added in
test-visitor-serialization were actually non-functional, and those tests were
based on pre-existing code. So I added a patch to fix the pre-existing
code as a pre-cursor to the new unit tests based on it. But fixing that code
exposed the json-parser bug, so I added that fix as a precursor to the
precursor :)

Same with mem leak fixes, etc, to ensure the tests were functional and
leak-free within this series.

> 
> > 
> > Fix this by detecting such cases and using a QFloat to store the value
> > instead.
> > 
> > Signed-off-by: Michael Roth <address@hidden>
> > ---
> >  qobject/json-parser.c |   26 +++++++++++++++++++++++---
> >  1 file changed, 23 insertions(+), 3 deletions(-)
> > 
> > diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> > index 05279c1..4d14e71 100644
> > --- a/qobject/json-parser.c
> > +++ b/qobject/json-parser.c
> > @@ -640,9 +640,29 @@ static QObject *parse_literal(JSONParserContext *ctxt)
> >      case JSON_STRING:
> >          obj = QOBJECT(qstring_from_escaped_str(ctxt, token));
> >          break;
> > -    case JSON_INTEGER:
> > -        obj = QOBJECT(qint_from_int(strtoll(token_get_value(token), NULL, 
> > 10)));
> > -        break;
> > +    case JSON_INTEGER: {
> > +        /* A possibility exists that this is a whole-valued float where the
> > +         * mantissa was left out due to being 0 (.0). It's not a big deal 
> > to
> > +         * treat these as ints in the parser, so long as users of the
> > +         * resulting QObject know to expect a QInt in place of a QFloat in
> > +         * cases like these.
> > +         *
> > +         * However, in some cases these values will overflow/underflow a
> > +         * QInt/int64 container, thus we should assume these are to be 
> > handled
> > +         * as QFloats/doubles rather than silently changing their values.
> > +         *
> > +         * strtoll() indicates these instances by setting errno to ERANGE
> > +         */
> > +        int64_t value;
> > +
> > +        errno = 0; /* strtoll doesn't set errno on success */
> > +        value = strtoll(token_get_value(token), NULL, 10);
> > +        if (errno != ERANGE) {
> > +            obj = QOBJECT(qint_from_int(value));
> > +            break;
> > +        }
> > +        /* fall through to JSON_FLOAT */
> > +    }
> >      case JSON_FLOAT:
> >          /* FIXME dependent on locale */
> >          obj = QOBJECT(qfloat_from_double(strtod(token_get_value(token), 
> > NULL)));
>