qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v3 09/12] tests/libqos/pci: Clean u


From: Markus Armbruster
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v3 09/12] tests/libqos/pci: Clean up string interpolation into QMP input
Date: Tue, 01 Aug 2017 07:40:21 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)

Eric Blake <address@hidden> writes:

> On 07/31/2017 02:29 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> On 07/28/2017 11:35 AM, Eric Blake wrote:
>>>>>> +    QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': 
>>>>>> '1M' }",
>>>>>> +                                             tmpshm);
>>>>>
>>>
>>>> Passing '%s' through qobject_from_jsonf() is generally wrong (it would
>>>> produce ''...'' instead of the intended '...').
>>>>
>>>> Looks like something to fix on the next round.
>>>>
>>>
>>> What's scary is that the testsuite didn't start failing.  That's not
>>> good; we'll want to figure out why the bug didn't impact the test.
>> 
>> Good idea.
>
> The intended result: the created QDict would include
> "shm":"/qtest-11387-3641365368" (or comparable string).  The actual
> result: the created QDict includes "shm":"%s" - a literal 2-character
> string, because the lexer does not do interpolation inside strings.  And
> apparently, naming your ivshmem shared memory a literal "%s" (rather
> than a name beginning with "/") is not a semantic error, so the test passes.
>
> In other words, qobject_from_jsonf() does NOT do % interpolation of
> anything embedded inside '', and basically blindly ignores the tmpshm
> vararg.

Actually, it recognizes conversion specifications in lexer state
IN_START.  The only other states where it accepts them are IN_DQ_STRING
and IN_SQ_STRING.  It chokes on '%' in all other states.  Therefore,
conversion specifications can be be silently ignored only in JSON
strings.

>          It would be neat if we could make qobject_from_jsonf() detect a
> mismatch in varargs, even though we don't (can't) require a NULL
> sentinel (we're limited to fitting in with -Wformat paradigms already).
> So while we can't flag it at compile time, I do think we can flag it at
> runtime: if, in qobject_from_jsonf() (but NOT in qobject_from_json()),
> we reject any JSON string from the lexer that contains "%" but not "%%",
> we will have caught all the places where gcc paired a % sequence with a
> vararg parameter even though our lexer did not make the same pairing.
> If we WANT a literal % in a string (rare, probably only in the
> testsuite), we write it as %% and then qobject_from_jsonf() interpolates
> it.

If we make it do that.

Note that the JSON parser then recognizing one subset of printf
conversion specifiers as values (%s, %ld, %lld, ...) and another subset
within strings (just %%).  Not exactly elegant, but it works.

We could let it accept more (all?) conversion specifiers within strings,
but that would bring back the temptation to code JSON injection flaws.

>      Then, since bare % is NOT valid JSON, we don't have to enhance the
> lexer to recognize anything new; our changes are limited to
> documentation and the vararg parser.  It still means we only get a
> runtime, rather than a compile-time, detection of misuse of % passed to
> the format argument, but it should be an easy enough proof of concept
> that such a runtime failure would have been enough to make ivshmem-test
> fail and flag the error during 'make check' rather than escaping review.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]