[Qemu-devel] [PATCH v3 0/3] Fix some qapi assert()

[Qemu-devel] [PATCH v3 0/3] Fix some qapi assert()

[Marc-André Lureau]

Hi,

The following series fixes 2 triggerable asserts in qmp code:
- object-add assert() without props
- input visitor may assert() on missing parameter

v3:
- fix docker build
- rename qemu-qmp-test -> test-qemu-qmp
- add r-b tags

Marc-André Lureau (3):
  qmp: fix object-add assert() without props
  qapi: fix crash when a parameter is missing
  tests: start generic qemu-qmp tests

 qapi/qmp-input-visitor.c | 11 +++++++++
 qmp.c                    |  8 +++++--
 tests/test-qemu-qmp.c    | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/Makefile.include   |  2 ++
 4 files changed, 80 insertions(+), 2 deletions(-)
 create mode 100644 tests/test-qemu-qmp.c

-- 
2.10.0

[Qemu-devel] [PATCH v3 1/3] qmp: fix object-add assert() without props

[Marc-André Lureau]

Since commit ad739706bbadee49, user_creatable_add_type() expects to be
given a qdict. However, if object-add is called without props, you reach
the assert: "qemu/qom/object_interfaces.c:115: user_creatable_add_type:
Assertion `qdict' failed.", because the qdict isn't created in this
case (it's optional).

Furthermore, qmp_input_visitor_new() is not meant to be called without a
dict, and a further commit will assert in this situation.

If none given, create an empty qdict in qmp to avoid the
user_creatable_add_type() assert(qdict).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qmp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/qmp.c b/qmp.c
index 6733463..b6de482 100644
--- a/qmp.c
+++ b/qmp.c
@@ -665,7 +665,7 @@ void qmp_add_client(const char *protocol, const char *fdname,
 void qmp_object_add(const char *type, const char *id,
                     bool has_props, QObject *props, Error **errp)
 {
-    const QDict *pdict = NULL;
+    QDict *pdict;
     Visitor *v;
     Object *obj;
 
@@ -675,14 +675,18 @@ void qmp_object_add(const char *type, const char *id,
             error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
             return;
         }
+        QINCREF(pdict);
+    } else {
+        pdict = qdict_new();
     }
 
-    v = qmp_input_visitor_new(props, true);
+    v = qmp_input_visitor_new(QOBJECT(pdict), true);
     obj = user_creatable_add_type(type, id, pdict, v, errp);
     visit_free(v);
     if (obj) {
         object_unref(obj);
     }
+    QDECREF(pdict);
 }
 
 void qmp_object_del(const char *id, Error **errp)
-- 
2.10.0

[Qemu-devel] [PATCH v3 2/3] qapi: fix crash when a parameter is missing

[Marc-André Lureau]

Calling:

{ "execute": "qom-set",
  "arguments": { "path": "/machine", "property": "rtc-time" } }

Will crash with:

qapi/qapi-visit-core.c:277: visit_type_any: Assertion `!err != !*obj'
failed

Clear the obj and return an error.

The patch also fixes a similar potential crash in qmp_input_type_null()
by checking qmp_input_get_object() returned a valid qobj.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qmp-input-visitor.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 64dd392..fc91e74 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -338,6 +338,12 @@ static void qmp_input_type_any(Visitor *v, const char *name, QObject **obj,
     QmpInputVisitor *qiv = to_qiv(v);
     QObject *qobj = qmp_input_get_object(qiv, name, true);
 
+    if (!qobj) {
+        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
+        *obj = NULL;
+        return;
+    }
+
     qobject_incref(qobj);
     *obj = qobj;
 }
@@ -347,6 +353,11 @@ static void qmp_input_type_null(Visitor *v, const char *name, Error **errp)
     QmpInputVisitor *qiv = to_qiv(v);
     QObject *qobj = qmp_input_get_object(qiv, name, true);
 
+    if (!qobj) {
+        error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
+        return;
+    }
+
     if (qobject_type(qobj) != QTYPE_QNULL) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "null");
-- 
2.10.0

[Qemu-devel] [PATCH v3 3/3] tests: start generic qemu-qmp tests

[Marc-André Lureau]

These 2 tests exhibit two qmp bugs fixed by the previous patches.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
---
 tests/test-qemu-qmp.c  | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/Makefile.include |  2 ++
 2 files changed, 63 insertions(+)
 create mode 100644 tests/test-qemu-qmp.c

diff --git a/tests/test-qemu-qmp.c b/tests/test-qemu-qmp.c
new file mode 100644
index 0000000..3cdee59
--- /dev/null
+++ b/tests/test-qemu-qmp.c
@@ -0,0 +1,61 @@
+#include "qemu/osdep.h"
+#include "libqtest.h"
+
+
+static void test_object_add_without_props(void)
+{
+    QDict *ret, *error;
+    const gchar *class, *desc;
+
+    ret = qmp("{'execute': 'object-add',"
+              " 'arguments': { 'qom-type': 'memory-backend-ram', 'id': 'ram1' } }");
+    g_assert_nonnull(ret);
+
+    error = qdict_get_qdict(ret, "error");
+    class = qdict_get_try_str(error, "class");
+    desc = qdict_get_try_str(error, "desc");
+
+    g_assert_cmpstr(class, ==, "GenericError");
+    g_assert_cmpstr(desc, ==, "can't create backend with size 0");
+
+    QDECREF(ret);
+}
+
+static void test_qom_set_without_value(void)
+{
+    QDict *ret, *error;
+    const gchar *class, *desc;
+
+    ret = qmp("{'execute': 'qom-set',"
+              " 'arguments': { 'path': '/machine', 'property': 'rtc-time' } }");
+    g_assert_nonnull(ret);
+
+    error = qdict_get_qdict(ret, "error");
+    class = qdict_get_try_str(error, "class");
+    desc = qdict_get_try_str(error, "desc");
+
+    g_assert_cmpstr(class, ==, "GenericError");
+    g_assert_cmpstr(desc, ==, "Parameter 'value' is missing");
+
+    QDECREF(ret);
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_start("");
+
+    qtest_add_func("/qemu-qmp/object-add-without-props",
+                   test_object_add_without_props);
+    qtest_add_func("/qemu-qmp/qom-set-without-value",
+                   test_qom_set_without_value);
+
+    ret = g_test_run();
+
+    qtest_end();
+
+    return ret;
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 6052a38..93f2ba1 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -296,6 +296,7 @@ check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
 check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
+check-qtest-generic-y += tests/test-qemu-qmp$(EXESUF)
 
 qapi-schema += alternate-any.json
 qapi-schema += alternate-array.json
@@ -634,6 +635,7 @@ tests/tpci200-test$(EXESUF): tests/tpci200-test.o
 tests/display-vga-test$(EXESUF): tests/display-vga-test.o
 tests/ipoctal232-test$(EXESUF): tests/ipoctal232-test.o
 tests/qom-test$(EXESUF): tests/qom-test.o
+tests/test-qemu-qmp$(EXESUF): tests/test-qemu-qmp.o
 tests/drive_del-test$(EXESUF): tests/drive_del-test.o $(libqos-pc-obj-y)
 tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o $(libqos-pc-obj-y)
 tests/nvme-test$(EXESUF): tests/nvme-test.o
-- 
2.10.0

Re: [Qemu-devel] [PATCH v3 3/3] tests: start generic qemu-qmp tests

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

On 09/22/2016 01:48 PM, Marc-André Lureau wrote:
> These 2 tests exhibit two qmp bugs fixed by the previous patches.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  tests/test-qemu-qmp.c  | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/Makefile.include |  2 ++
>  2 files changed, 63 insertions(+)
>  create mode 100644 tests/test-qemu-qmp.c
> 
> diff --git a/tests/test-qemu-qmp.c b/tests/test-qemu-qmp.c
> new file mode 100644
> index 0000000..3cdee59
> --- /dev/null
> +++ b/tests/test-qemu-qmp.c
> @@ -0,0 +1,61 @@
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +

No copyright blurb? The default GPLv2+ is fine, but being explicit never
hurts.

> +
> +static void test_object_add_without_props(void)
> +{
> +    QDict *ret, *error;
> +    const gchar *class, *desc;
> +
> +    ret = qmp("{'execute': 'object-add',"
> +              " 'arguments': { 'qom-type': 'memory-backend-ram', 'id': 'ram1' } }");
> +    g_assert_nonnull(ret);
> +
> +    error = qdict_get_qdict(ret, "error");
> +    class = qdict_get_try_str(error, "class");

This variable won't compile with C++; should you rename it to 'klass'
just in case, since we already have part of qemu being built by C++ for
the sake of qga on Windows?

> +    desc = qdict_get_try_str(error, "desc");
> +
> +    g_assert_cmpstr(class, ==, "GenericError");
> +    g_assert_cmpstr(desc, ==, "can't create backend with size 0");

A little bit fragile to be parsing for a particular error message
wording, but I guess I can live with it.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 3/3] tests: start generic qemu-qmp tests

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

On 09/22/2016 01:48 PM, Marc-André Lureau wrote:
> These 2 tests exhibit two qmp bugs fixed by the previous patches.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  tests/test-qemu-qmp.c  | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/Makefile.include |  2 ++
>  2 files changed, 63 insertions(+)
>  create mode 100644 tests/test-qemu-qmp.c

Please also update tests/.gitignore to ignore the new executable.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]