We have two flavors of vararg usage in qtest; make it clear that
qmp() has different semantics than hmp(), and let the compiler
enforce that hmp() is used correctly. Since qmp() only accepts
a subset of printf flags (namely, those that our JSON parser
understands), I figured that it is probably not worth adding a
format attribute to qmp() at this time.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/libqtest.h | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 38bc1e9..762ed13 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -50,7 +50,8 @@ void qtest_quit(QTestState *s);
 /**
  * qtest_qmp_discard_response:
  * @s: #QTestState instance to operate on.
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; only recognizes formats
+ * understood by json-lexer.c
  *
  * Sends a QMP message to QEMU and consumes the response.
  */
@@ -59,7 +60,8 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
 /**
  * qtest_qmp:
  * @s: #QTestState instance to operate on.
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; only recognizes formats
+ * understood by json-lexer.c
  *
  * Sends a QMP message to QEMU and returns the response.
  */
@@ -134,14 +136,14 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
 /**
  * qtest_hmp:
  * @s: #QTestState instance to operate on.
- * @fmt...: HMP command to send to QEMU
+ * @fmt...: HMP command to send to QEMU, passed through sprintf()
  *
  * Send HMP command to QEMU via QMP's human-monitor-command.
  * QMP events are discarded.
  *
  * Returns: the command's output.  The caller should g_free() it.
  */
-char *qtest_hmp(QTestState *s, const char *fmt, ...);
+char *qtest_hmp(QTestState *s, const char *fmt, ...) GCC_FMT_ATTR(2, 3);

 /**
  * qtest_hmpv:
@@ -535,7 +537,8 @@ static inline void qtest_end(void)

 /**
  * qmp:
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; only recognizes formats
+ * understood by json-lexer.c
  *
  * Sends a QMP message to QEMU and returns the response.
  */
@@ -543,7 +546,8 @@ QDict *qmp(const char *fmt, ...);

 /**
  * qmp_async:
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; only recognizes formats
+ * understood by json-lexer.c
  *
  * Sends a QMP message to QEMU and leaves the response in the stream.
  */
@@ -551,7 +555,8 @@ void qmp_async(const char *fmt, ...);

 /**
  * qmp_discard_response:
- * @fmt...: QMP message to send to qemu
+ * @fmt...: QMP message to send to qemu; only recognizes formats
+ * understood by json-lexer.c
  *
  * Sends a QMP message to QEMU and consumes the response.
  */
@@ -592,13 +597,13 @@ static inline QDict *qmp_eventwait_ref(const char *event)

 /**
  * hmp:
- * @fmt...: HMP command to send to QEMU
+ * @fmt...: HMP command to send to QEMU, passed through printf()
  *
  * Send HMP command to QEMU via QMP's human-monitor-command.
  *
  * Returns: the command's output.  The caller should g_free() it.
  */
-char *hmp(const char *fmt, ...);
+char *hmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);

 /**
  * get_irq:
-- 
2.9.4

Eric Blake <eblake@redhat.com> writes:

> We have two flavors of vararg usage in qtest; make it clear that
> qmp() has different semantics than hmp(), and let the compiler
> enforce that hmp() is used correctly. Since qmp() only accepts
> a subset of printf flags (namely, those that our JSON parser
> understands), I figured that it is probably not worth adding a
> format attribute to qmp() at this time.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/libqtest.h | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 38bc1e9..762ed13 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -50,7 +50,8 @@ void qtest_quit(QTestState *s);
>  /**
>   * qtest_qmp_discard_response:
>   * @s: #QTestState instance to operate on.
> - * @fmt...: QMP message to send to qemu
> + * @fmt...: QMP message to send to qemu; only recognizes formats
> + * understood by json-lexer.c
>   *
>   * Sends a QMP message to QEMU and consumes the response.
>   */

These formats are chosen so that gcc can help us check them.  Please add
GCC_FMT_ATTR().  Precedence: qobject_from_jsonf().

Where are the "formats understood by json-lexer.c" documented?

More of the same below.

> @@ -59,7 +60,8 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
>  /**
>   * qtest_qmp:
>   * @s: #QTestState instance to operate on.
> - * @fmt...: QMP message to send to qemu
> + * @fmt...: QMP message to send to qemu; only recognizes formats
> + * understood by json-lexer.c
>   *
>   * Sends a QMP message to QEMU and returns the response.
>   */
> @@ -134,14 +136,14 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event);
>  /**
>   * qtest_hmp:
>   * @s: #QTestState instance to operate on.
> - * @fmt...: HMP command to send to QEMU
> + * @fmt...: HMP command to send to QEMU, passed through sprintf()

Not actually through sprintf(), but I get what you mean.  I like to
document such things as "Format arguments like vsprintf()."

>   *
>   * Send HMP command to QEMU via QMP's human-monitor-command.
>   * QMP events are discarded.
>   *
>   * Returns: the command's output.  The caller should g_free() it.
>   */
> -char *qtest_hmp(QTestState *s, const char *fmt, ...);
> +char *qtest_hmp(QTestState *s, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
>
>  /**
>   * qtest_hmpv:
> @@ -535,7 +537,8 @@ static inline void qtest_end(void)
>
>  /**
>   * qmp:
> - * @fmt...: QMP message to send to qemu
> + * @fmt...: QMP message to send to qemu; only recognizes formats
> + * understood by json-lexer.c
>   *
>   * Sends a QMP message to QEMU and returns the response.
>   */
> @@ -543,7 +546,8 @@ QDict *qmp(const char *fmt, ...);
>
>  /**
>   * qmp_async:
> - * @fmt...: QMP message to send to qemu
> + * @fmt...: QMP message to send to qemu; only recognizes formats
> + * understood by json-lexer.c
>   *
>   * Sends a QMP message to QEMU and leaves the response in the stream.
>   */
> @@ -551,7 +555,8 @@ void qmp_async(const char *fmt, ...);
>
>  /**
>   * qmp_discard_response:
> - * @fmt...: QMP message to send to qemu
> + * @fmt...: QMP message to send to qemu; only recognizes formats
> + * understood by json-lexer.c
>   *
>   * Sends a QMP message to QEMU and consumes the response.
>   */
> @@ -592,13 +597,13 @@ static inline QDict *qmp_eventwait_ref(const char *event)
>
>  /**
>   * hmp:
> - * @fmt...: HMP command to send to QEMU
> + * @fmt...: HMP command to send to QEMU, passed through printf()

Here, you claim printf().  Typo?

>   *
>   * Send HMP command to QEMU via QMP's human-monitor-command.
>   *
>   * Returns: the command's output.  The caller should g_free() it.
>   */
> -char *hmp(const char *fmt, ...);
> +char *hmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>
>  /**
>   * get_irq:

On 07/20/2017 05:10 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> We have two flavors of vararg usage in qtest; make it clear that
>> qmp() has different semantics than hmp(), and let the compiler
>> enforce that hmp() is used correctly. Since qmp() only accepts
>> a subset of printf flags (namely, those that our JSON parser
>> understands), I figured that it is probably not worth adding a
>> format attribute to qmp() at this time.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  tests/libqtest.h | 23 ++++++++++++++---------
>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/tests/libqtest.h b/tests/libqtest.h
>> index 38bc1e9..762ed13 100644
>> --- a/tests/libqtest.h
>> +++ b/tests/libqtest.h
>> @@ -50,7 +50,8 @@ void qtest_quit(QTestState *s);
>>  /**
>>   * qtest_qmp_discard_response:
>>   * @s: #QTestState instance to operate on.
>> - * @fmt...: QMP message to send to qemu
>> + * @fmt...: QMP message to send to qemu; only recognizes formats
>> + * understood by json-lexer.c
>>   *
>>   * Sends a QMP message to QEMU and consumes the response.
>>   */
> 
> These formats are chosen so that gcc can help us check them.  Please add
> GCC_FMT_ATTR().  Precedence: qobject_from_jsonf().

Will do.  (This patch was originally part of my larger series trying to
get rid of qobject_from_jsonf() - I may still succeed at that, but
separating the easy stuff from the controversial means that the easy
stuff needs tweaking).

> 
> Where are the "formats understood by json-lexer.c" documented?

Near the top of qobject/json-lexer.c:

 * Extension for vararg handling in JSON construction:
 *
 * %((l|ll|I64)?d|[ipsf])

Note that %i differs from %d (the former produces true/false, while the
latter produces "42" and friends - but it happens to "work" with gcc's
-Wformat checking, thanks to var-arg type promotion rules).

I could just spell it out directly (in fact, in the above-mentioned
larger series, I still kept qtest_qmp() able to understand %s, but
dropped all the other formats like %ld and %p).

>> + * @fmt...: HMP command to send to QEMU, passed through sprintf()
> 
> Not actually through sprintf(), but I get what you mean.  I like to
> document such things as "Format arguments like vsprintf()."

Works for me.

>> @@ -592,13 +597,13 @@ static inline QDict *qmp_eventwait_ref(const char *event)
>>
>>  /**
>>   * hmp:
>> - * @fmt...: HMP command to send to QEMU
>> + * @fmt...: HMP command to send to QEMU, passed through printf()
> 
> Here, you claim printf().  Typo?

Or inconsistent copy-and-paste.

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

On 07/20/2017 03:37 PM, Eric Blake wrote:
>>> + * @fmt...: QMP message to send to qemu; only recognizes formats
>>> + * understood by json-lexer.c
>>>   *
>>>   * Sends a QMP message to QEMU and consumes the response.
>>>   */
>>
>> These formats are chosen so that gcc can help us check them.  Please add
>> GCC_FMT_ATTR().  Precedence: qobject_from_jsonf().
> 
> Will do.  (This patch was originally part of my larger series trying to
> get rid of qobject_from_jsonf() - I may still succeed at that, but
> separating the easy stuff from the controversial means that the easy
> stuff needs tweaking).

Bleargh.  It's not that simple.

With printf-style, hmp("literal") and hmp("%s", "literal") produce the
same results.

But with json-lexer style, %s MODIFIES its input.
The original qmp("{'execute':\"foo\"}") sends a JSON object
 {'execute':"foo"}
but counterpart qmp("%s", "{'execute':'foo'}") sends a JSON string with
added \ escaping:
 "{'execute':\"foo\"}"

So adding the format immediately causes lots of warnings, such as:

  CC      tests/vhost-user-test.o
tests/vhost-user-test.c: In function ‘test_migrate’:
tests/vhost-user-test.c:668:5: error: format not a string literal and no
format arguments [-Werror=format-security]
     rsp = qmp(cmd);
     ^~~

  CC      tests/boot-order-test.o
tests/boot-order-test.c: In function ‘test_a_boot_order’:
tests/boot-order-test.c:46:26: error: zero-length gnu_printf format
string [-Werror=format-zero-length]
     qmp_discard_response("");   /* HACK: wait for event */
                          ^~

but we CAN'T rewrite those to qmp("%s", command).

Now you can sort-of understand why my larger series wanted to get rid of
qobject_from_jsonf(), since our use of GCC_FMT_ATTR() there is a lie?

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

Eric Blake <eblake@redhat.com> writes:

> On 07/20/2017 03:37 PM, Eric Blake wrote:
>>>> + * @fmt...: QMP message to send to qemu; only recognizes formats
>>>> + * understood by json-lexer.c
>>>>   *
>>>>   * Sends a QMP message to QEMU and consumes the response.
>>>>   */
>>>
>>> These formats are chosen so that gcc can help us check them.  Please add
>>> GCC_FMT_ATTR().  Precedence: qobject_from_jsonf().
>> 
>> Will do.  (This patch was originally part of my larger series trying to
>> get rid of qobject_from_jsonf() - I may still succeed at that, but
>> separating the easy stuff from the controversial means that the easy
>> stuff needs tweaking).
>
> Bleargh.  It's not that simple.
>
> With printf-style, hmp("literal") and hmp("%s", "literal") produce the
> same results.
>
> But with json-lexer style, %s MODIFIES its input.

Your assertion "MODIFIES its input" confused me briefly, because I read
it as "side effect on the argument string".  That would be bonkers.
What you mean is it doesn't emit the argument string unmodified.

> The original qmp("{'execute':\"foo\"}") sends a JSON object
>  {'execute':"foo"}
> but counterpart qmp("%s", "{'execute':'foo'}") sends a JSON string with
> added \ escaping:
>  "{'execute':\"foo\"}"
>
> So adding the format immediately causes lots of warnings, such as:
>
>   CC      tests/vhost-user-test.o
> tests/vhost-user-test.c: In function ‘test_migrate’:
> tests/vhost-user-test.c:668:5: error: format not a string literal and no
> format arguments [-Werror=format-security]
>      rsp = qmp(cmd);
>      ^~~

    cmd = g_strdup_printf("{ 'execute': 'migrate',"
                          "'arguments': { 'uri': '%s' } }",
                          uri);
    rsp = qmp(cmd);
    g_free(cmd);
    g_assert(qdict_haskey(rsp, "return"));
    QDECREF(rsp);

For this to work, @uri must not contain funny characters.

Shouldn't we simply use the built-in quoting here?

    rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
    g_assert(qdict_haskey(rsp, "return"));
    QDECREF(rsp);

>
>   CC      tests/boot-order-test.o
> tests/boot-order-test.c: In function ‘test_a_boot_order’:
> tests/boot-order-test.c:46:26: error: zero-length gnu_printf format
> string [-Werror=format-zero-length]
>      qmp_discard_response("");   /* HACK: wait for event */
>                           ^~

Should use qmp_eventwait().  Doesn't because it predates it.

> but we CAN'T rewrite those to qmp("%s", command).

Got more troublemakers?

> Now you can sort-of understand why my larger series wanted to get rid of
> qobject_from_jsonf(), since our use of GCC_FMT_ATTR() there is a lie?

I don't think it's a lie.  qobject_from_jsonf() satisfies the attribute
format(printf, ...) contract: it fetches varargs exactly like printf().
What it does with them is of no concern to this attribute.

On 07/21/2017 01:42 AM, Markus Armbruster wrote:
>> But with json-lexer style, %s MODIFIES its input.
> 
> Your assertion "MODIFIES its input" confused me briefly, because I read
> it as "side effect on the argument string".  That would be bonkers.
> What you mean is it doesn't emit the argument string unmodified.

Yes. I'm not sure how I could have worded that better; maybe "does not
do a straight passthrough of its input"

>> So adding the format immediately causes lots of warnings, such as:
>>
>>   CC      tests/vhost-user-test.o
>> tests/vhost-user-test.c: In function ‘test_migrate’:
>> tests/vhost-user-test.c:668:5: error: format not a string literal and no
>> format arguments [-Werror=format-security]
>>      rsp = qmp(cmd);
>>      ^~~
> 
>     cmd = g_strdup_printf("{ 'execute': 'migrate',"
>                           "'arguments': { 'uri': '%s' } }",
>                           uri);
>     rsp = qmp(cmd);
>     g_free(cmd);
>     g_assert(qdict_haskey(rsp, "return"));
>     QDECREF(rsp);
> 
> For this to work, @uri must not contain funny characters.
> 
> Shouldn't we simply use the built-in quoting here?

We can - but there are a lot of tests that have to be updated.

> 
>     rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
>     g_assert(qdict_haskey(rsp, "return"));
>     QDECREF(rsp);
> 
>>
>>   CC      tests/boot-order-test.o
>> tests/boot-order-test.c: In function ‘test_a_boot_order’:
>> tests/boot-order-test.c:46:26: error: zero-length gnu_printf format
>> string [-Werror=format-zero-length]
>>      qmp_discard_response("");   /* HACK: wait for event */
>>                           ^~
> 
> Should use qmp_eventwait().  Doesn't because it predates it.

We can - but there are a lot of tests that have to be updated.

> 
>> but we CAN'T rewrite those to qmp("%s", command).
> 
> Got more troublemakers?

When I worked on getting rid of qobject_from_jsonf(), I was able to
reduce the tests down to JUST using %s, which I then handled locally in
qmp() rather than relying on qobject_from_jsonf().  Going the opposite
direction and more fully relying on qobject_from_jsonf() by fixing
multiple tests that were using older idioms is also an approach (in the
opposite direction I originally took) - but the more work it is, the
less likely that this patch is 2.10 material.

> 
>> Now you can sort-of understand why my larger series wanted to get rid of
>> qobject_from_jsonf(), since our use of GCC_FMT_ATTR() there is a lie?
> 
> I don't think it's a lie.  qobject_from_jsonf() satisfies the attribute
> format(printf, ...) contract: it fetches varargs exactly like printf().
> What it does with them is of no concern to this attribute.

I still find it odd that you can't safely turn foo(var) into foo("%s", var).

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

Eric Blake <eblake@redhat.com> writes:

> On 07/21/2017 01:42 AM, Markus Armbruster wrote:
>>> But with json-lexer style, %s MODIFIES its input.
>> 
>> Your assertion "MODIFIES its input" confused me briefly, because I read
>> it as "side effect on the argument string".  That would be bonkers.
>> What you mean is it doesn't emit the argument string unmodified.
>
> Yes. I'm not sure how I could have worded that better; maybe "does not
> do a straight passthrough of its input"
>
>>> So adding the format immediately causes lots of warnings, such as:
>>>
>>>   CC      tests/vhost-user-test.o
>>> tests/vhost-user-test.c: In function ‘test_migrate’:
>>> tests/vhost-user-test.c:668:5: error: format not a string literal and no
>>> format arguments [-Werror=format-security]
>>>      rsp = qmp(cmd);
>>>      ^~~
>> 
>>     cmd = g_strdup_printf("{ 'execute': 'migrate',"
>>                           "'arguments': { 'uri': '%s' } }",
>>                           uri);
>>     rsp = qmp(cmd);
>>     g_free(cmd);
>>     g_assert(qdict_haskey(rsp, "return"));
>>     QDECREF(rsp);
>> 
>> For this to work, @uri must not contain funny characters.
>> 
>> Shouldn't we simply use the built-in quoting here?
>
> We can - but there are a lot of tests that have to be updated.

Not that many, see "[PATCH 0/9] tests: Clean up around qmp() and hmp()".
Its PATCH 4/9 makes the case for built-in quoting:

    When you build QMP input manually like this

        cmd = g_strdup_printf("{ 'execute': 'migrate',"
                              "'arguments': { 'uri': '%s' } }",
                              uri);
        rsp = qmp(cmd);
        g_free(cmd);

    you're responsible for escaping the interpolated values for JSON.  Not
    done here, and therefore works only for sufficiently nice @uri.  For
    instance, if @uri contained a single "'", qobject_from_jsonv() would
    fail, qmp_fd_sendv() would misinterpret the failure as empty input and
    do nothing, and the test would hang waiting for a response that never
    comes.

    Leaving interpolation into JSON to qmp() is more robust:

        rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);

    It's also more concise.

In short, using printf() & similar to format JSON is a JSON injection
vulnerability waiting to happen.  Special case: g_strdup_printf() to
format input for qobject_from_jsonf() & friends.  Leaving the job to
qobject_from_jsonf() is more robust.

>> 
>>     rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
>>     g_assert(qdict_haskey(rsp, "return"));
>>     QDECREF(rsp);
>> 
>>>
>>>   CC      tests/boot-order-test.o
>>> tests/boot-order-test.c: In function ‘test_a_boot_order’:
>>> tests/boot-order-test.c:46:26: error: zero-length gnu_printf format
>>> string [-Werror=format-zero-length]
>>>      qmp_discard_response("");   /* HACK: wait for event */
>>>                           ^~
>> 
>> Should use qmp_eventwait().  Doesn't because it predates it.
>
> We can - but there are a lot of tests that have to be updated.

Also not that many, see same series.

>>> but we CAN'T rewrite those to qmp("%s", command).
>> 
>> Got more troublemakers?
>
> When I worked on getting rid of qobject_from_jsonf(), I was able to
> reduce the tests down to JUST using %s, which I then handled locally in
> qmp() rather than relying on qobject_from_jsonf().  Going the opposite
> direction and more fully relying on qobject_from_jsonf() by fixing
> multiple tests that were using older idioms is also an approach (in the
> opposite direction I originally took) - but the more work it is, the
> less likely that this patch is 2.10 material.

No need to worry about 2.10, I think.

>>> Now you can sort-of understand why my larger series wanted to get rid of
>>> qobject_from_jsonf(), since our use of GCC_FMT_ATTR() there is a lie?
>> 
>> I don't think it's a lie.  qobject_from_jsonf() satisfies the attribute
>> format(printf, ...) contract: it fetches varargs exactly like printf().
>> What it does with them is of no concern to this attribute.
>
> I still find it odd that you can't safely turn foo(var) into foo("%s", var).

For me, the protection against JSON injection bugs is well worth it.