Re: [Qemu-devel] [PATCH v4 06/22] qobject: Perform %% interpolation in qobject_from_jsonf()

[Eric Blake]

On 08/09/2017 04:06 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> We want -Wformat to catch blatant programming errors in format
>> strings that we pass to qobject_from_jsonf().  But if someone
>> were to pass a JSON string "'%s'" as the format string, gcc would
>> insist that it be paired with a char* argument, even though our
>> lexer does not recognize % sequences inside a JSON string.  You can
>> bypass -Wformat checking by passing the Unicode escape \u0025 for
>> %, but who wants to remember to type that?  So the solution is that
>> anywhere we are relying on -Wformat checking, the caller should
>> pass the usual printf %% escape sequence where a literal % is
>> wanted on output.
>>

>> +    bool double_quote = *ptr++ == '"';
>>
>>      str = qstring_new();
>> -    while (*ptr && 
>> -           ((double_quote && *ptr != '"') || (!double_quote && *ptr != 
>> '\''))) {
>> +    while (*ptr && *ptr != "'\""[double_quote]) {
> 
> Simpler:
> 
>        bool quote = *ptr++;
> 
> and then
> 
>        while (*ptr && *ptr != quote) {

Well, 'char quote' rather than 'bool quote', but yes, I like it.

> 
> Have you considered splitting the patch into one to simplify, and one to
> implement %%?

Will split.

>> @@ -455,13 +452,15 @@ static QObject *parse_escape(JSONParserContext *ctxt, 
>> va_list *ap)
>>  {
>>      JSONToken *token;
>>
>> -    if (ap == NULL) {
>> -        return NULL;
>> -    }
>> -
>>      token = parser_context_pop_token(ctxt);
>>      assert(token && token->type == JSON_ESCAPE);
>>
>> +    if (ap == NULL) {
>> +        parse_error(ctxt, token, "escape parsing for '%s' not requested",
>> +                    token->str);
>> +        return NULL;
>> +    }
>> +
> 
> When I manage to fat-finger a '%' into my QMP input, I now get this
> error message instead of "Invalid JSON syntax".  Makes me go "what is
> escape parsing, and how do I request it?"  Not an improvement, I'm
> afraid.

Pre-patch, I see:

$ qemu-kvm -nodefaults -nographic -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 91, "minor": 9, "major": 2},
"package": "(qemu-2.10.0-0.1.rc1.fc26)"}, "capabilities": []}}
{'execute':%s}
{"error": {"class": "GenericError", "desc": "JSON parse error, Missing
value in dict"}}
{'execute':%%}
{"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
{"error": {"class": "GenericError", "desc": "JSON parse error, expecting
value"}}

I find it odd that NOT calling parse_error() but still returning NULL
changes the behavior on what error message eventually gets emitted; but
I also agree that the QMP case should drive what error message (if any)
is needed in parse_escape().  I'll play with it some more (the parser's
error handling is weird).


>> +    /* In vararg form, %% must occur in strings */
>> +    /* qobject_from_jsonf("%%"); */
>> +    /* qobject_from_jsonf("{%%}"); */
>> +
>> +    /* In vararg form, strings must not use % except for %% */
>> +    /* qobject_from_jsonf("'%s'", "unpaired"); */
> 
> Could use g_test_trap_subprocess().  Not sure it's worth the bother.

I don't know - this is one case where proving we abort on invalid usage
might actually be a good validation of the contract.

> 
> I hate code in comments.  Better:
> 
>        /* The following intentionally cause assertion failures */
>    #if 0
>        /* In vararg form, %% must occur in strings */
>        qobject_from_jsonf("%%");

If I don't use the g_test_trap_subprocess() trick, then I can override
checkpatch's complaints about #if 0.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature