From: Markus Armbruster <armbru@redhat.com>
Leaving interpolation into JSON to qmp() is more robust than building
QMP input manually, as explained in the commit before previous.
The case in qpci_plug_device_test() is a bit complicated: it
interpolates several JSON object members, not just a value. Clean it
up by passing them in as QDict rather than string, so we can leave
interpolation to qmp() here and to qobject_from_jsonf() in callers.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <1500645206-13559-7-git-send-email-armbru@redhat.com>
[use qmp_cmd for a slightly smaller diff]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
tests/libqos/pci.h | 2 +-
tests/ivshmem-test.c | 10 +++++-----
tests/libqos/pci.c | 32 +++++++++++++++++---------------
tests/virtio-blk-test.c | 5 ++++-
4 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
index ed480614ff..c981061703 100644
--- a/tests/libqos/pci.h
+++ b/tests/libqos/pci.h
@@ -109,6 +109,6 @@ void qpci_iounmap(QPCIDevice *dev, QPCIBar addr);
QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr);
void qpci_plug_device_test(const char *driver, const char *id,
- uint8_t slot, const char *opts);
+ uint8_t slot, QDict *extra_args);
void qpci_unplug_acpi_device_test(const char *id, uint8_t slot);
#endif
diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
index 37763425ee..38044bb01c 100644
--- a/tests/ivshmem-test.c
+++ b/tests/ivshmem-test.c
@@ -14,6 +14,7 @@
#include "libqos/libqos-pc.h"
#include "libqos/libqos-spapr.h"
#include "libqtest.h"
+#include "qapi/qmp/qjson.h"
#include "qemu-common.h"
#define TMPSHMSIZE (1 << 20)
@@ -419,19 +420,18 @@ static void test_ivshmem_server_irq(void)
static void test_ivshmem_hotplug(void)
{
const char *arch = qtest_get_arch();
- gchar *opts;
+ QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
+ tmpshm);
qtest_start("");
- opts = g_strdup_printf("'shm': '%s', 'size': '1M'", tmpshm);
-
- qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);
+ qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP,
+ qobject_to_qdict(extra_args));
if (strcmp(arch, "ppc64") != 0) {
qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP);
}
qtest_end();
- g_free(opts);
}
static void test_ivshmem_memdev(void)
diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index 2dcdeade2a..2754412340 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -14,6 +14,7 @@
#include "libqos/pci.h"
#include "hw/pci/pci_regs.h"
+#include "qapi/qmp/qjson.h"
#include "qemu/host-utils.h"
void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
@@ -392,22 +393,23 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
}
void qpci_plug_device_test(const char *driver, const char *id,
- uint8_t slot, const char *opts)
+ uint8_t slot, QDict *extra_args)
{
- QDict *response;
- char *cmd;
-
- cmd = g_strdup_printf("{'execute': 'device_add',"
- " 'arguments': {"
- " 'driver': '%s',"
- " 'addr': '%d',"
- " %s%s"
- " 'id': '%s'"
- "}}", driver, slot,
- opts ? opts : "", opts ? "," : "",
- id);
- response = qmp(cmd);
- g_free(cmd);
+ char addr[8];
+ QDict *args, *response;
+
+ sprintf(addr, "%d", slot);
+ args = qobject_to_qdict(
+ qobject_from_jsonf("{ 'driver': %s, 'addr': %s, 'id': %s}",
+ driver, addr, id));
+
+ if (extra_args) {
+ qdict_join(args, extra_args, true);
+ QDECREF(extra_args);
+ }
+
+ response = qmp_cmd("device_add", QOBJECT(args));
+
g_assert(response);
g_assert(!qdict_haskey(response, "error"));
QDECREF(response);
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 0576cb16ba..64a48f40b2 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -16,6 +16,7 @@
#include "libqos/virtio-pci.h"
#include "libqos/virtio-mmio.h"
#include "libqos/malloc-generic.h"
+#include "qapi/qmp/qjson.h"
#include "qemu/bswap.h"
#include "standard-headers/linux/virtio_ids.h"
#include "standard-headers/linux/virtio_config.h"
@@ -658,12 +659,13 @@ static void pci_hotplug(void)
QVirtioPCIDevice *dev;
QOSState *qs;
const char *arch = qtest_get_arch();
+ QObject *extra_args = qobject_from_jsonf("{ 'drive': 'drive1' }");
qs = pci_test_start();
/* plug secondary disk */
qpci_plug_device_test("virtio-blk-pci", "drv1", PCI_SLOT_HP,
- "'drive': 'drive1'");
+ qobject_to_qdict(extra_args));
dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT_HP);
g_assert(dev);
@@ -674,6 +676,7 @@ static void pci_hotplug(void)
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
qpci_unplug_acpi_device_test("drv1", PCI_SLOT_HP);
}
+
qtest_shutdown(qs);
}
--
2.13.3
On Tue, Jul 25, 2017 at 04:15:20PM -0500, Eric Blake wrote:
> @@ -419,19 +420,18 @@ static void test_ivshmem_server_irq(void)
> static void test_ivshmem_hotplug(void)
> {
> const char *arch = qtest_get_arch();
> - gchar *opts;
> + QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
> + tmpshm);
Is there a difference between:
qobject_from_jsonf("{ 'shm': '%s' }", tmpshm);
and:
qobject_from_jsonf("{ 'shm': %s }", tmpshm);
?
Below you use %s instead of '%s'.
On 07/28/2017 08:05 AM, Stefan Hajnoczi wrote:
> On Tue, Jul 25, 2017 at 04:15:20PM -0500, Eric Blake wrote:
>> @@ -419,19 +420,18 @@ static void test_ivshmem_server_irq(void)
>> static void test_ivshmem_hotplug(void)
>> {
>> const char *arch = qtest_get_arch();
>> - gchar *opts;
>> + QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
>> + tmpshm);
>
> Is there a difference between:
>
> qobject_from_jsonf("{ 'shm': '%s' }", tmpshm);
>
> and:
>
> qobject_from_jsonf("{ 'shm': %s }", tmpshm);
Yes, and it's important.
sprintf("{ 'shm': '%s' }", tmpshm);
is the same as
qobject_from_jsonf("{ 'shm': %s }" 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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
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.
--
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/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.
On 07/31/2017 02:29 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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. 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. 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.
--
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/31/2017 02:29 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> 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.
Stefan Hajnoczi <stefanha@gmail.com> writes:
> On Tue, Jul 25, 2017 at 04:15:20PM -0500, Eric Blake wrote:
>> @@ -419,19 +420,18 @@ static void test_ivshmem_server_irq(void)
>> static void test_ivshmem_hotplug(void)
>> {
>> const char *arch = qtest_get_arch();
>> - gchar *opts;
>> + QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
>> + tmpshm);
>
> Is there a difference between:
>
> qobject_from_jsonf("{ 'shm': '%s' }", tmpshm);
>
> and:
>
> qobject_from_jsonf("{ 'shm': %s }", tmpshm);
>
> ?
>
> Below you use %s instead of '%s'.
Yes. %s interpolates a JSON string, enclosed in quotes, funny
characters quoted. Thus, the former is wrong. I screwed up the
conversion from g_strdup_printf(). Good catch!
© 2016 - 2026 Red Hat, Inc.