Eric Blake <ebl...@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.