| Message ID | 20170714190827.4083-6-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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 <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 <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.
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:
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(-)