Re: [Qemu-devel] [PATCH v2 1/4] qmp-event: Avoid qobject_from_jsonf("%"PRId64)

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> The qobject_from_jsonf() function implements a pseudo-printf
> language for creating a QObject; however, it is hard-coded to
> only parse a subset of formats understood by -Wformat, and is
> not a straight synonym to bare printf().  In particular, any
> use of an int64_t integer works only if the system's
> definition of PRId64 matches what the parser expects; which
> works on glibc (%lld or %ld depending on 32- vs. 64-bit) and
> mingw (%I64d), but not on Mac OS (%qd).  Rather than enhance
> the parser, it is just as easy to use 'long long', which we
> know always works.  There are few enough callers of
> qobject_from_json[fv]() that it is easy to audit that this is
> the only non-testsuite caller that was actually relying on
> this particular conversion.
>
> Reported by: G 3 <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v2: keep qobject_from_jsonf for now, but switch to %lld
> ---
>  qapi/qmp-event.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/qapi/qmp-event.c b/qapi/qmp-event.c
> index 8bba165..e7c8755 100644
> --- a/qapi/qmp-event.c
> +++ b/qapi/qmp-event.c
> @@ -35,21 +35,12 @@ static void timestamp_put(QDict *qdict)
>      int err;
>      QObject *obj;
>      qemu_timeval tv;
> -    int64_t sec, usec;
>
>      err = qemu_gettimeofday(&tv);
> -    if (err < 0) {
> -        /* Put -1 to indicate failure of getting host time */
> -        sec = -1;
> -        usec = -1;
> -    } else {
> -        sec = tv.tv_sec;
> -        usec = tv.tv_usec;
> -    }
> -
> -    obj = qobject_from_jsonf("{ 'seconds': %" PRId64 ", "
> -                             "'microseconds': %" PRId64 " }",
> -                             sec, usec);
> +    /* Put -1 to indicate failure of getting host time */
> +    obj = qobject_from_jsonf("{ 'seconds': %lld, 'microseconds': %lld }",
> +                             err < 0 ? -1LL : tv.tv_sec,
> +                             err < 0 ? -1LL : tv.tv_usec);
>      qdict_put_obj(qdict, "timestamp", obj);
>  }

The ternaries must both yield long long for this to work.  They will,
unless their last operand's rank is greater than long long's, or their
last operand is unsigned long long.  The latter case should be harmless
in practice.  The former case would be weird.  Hmm.

I think it's best to be a bit more explicit here.  Let's cast the last
operands to long long, or use long long variables.  Your choice.