Re: [Qemu-devel] [PATCH 08/10] qapi: add strict mode to input visitor

[Laurent Desnogues]

Hello,

On Thu, Mar 22, 2012 at 12:51 PM, Paolo Bonzini <address@hidden> wrote:
> While QMP in general is designed so that it is possible to ignore
> unknown arguments, in the case of the QMP server it is better to
> reject them to detect bad clients.  In fact, we're already doing
> this at the top level in the argument checker.  To extend this to
> complex structures, add a mode to the input visitor where it checks
> for unvisited keys and raises an error if it finds one.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  qapi/qmp-input-visitor.c |   48 +++++++++-
>  qapi/qmp-input-visitor.h |    2 +
>  test-qmp-input-strict.c  |  234 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/Makefile           |    5 +-
>  4 files changed, 285 insertions(+), 4 deletions(-)
>  create mode 100644 test-qmp-input-strict.c
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 97a0186..eb09874 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -24,6 +24,7 @@ typedef struct StackObject
>  {
>     QObject *obj;
>     const QListEntry *entry;
> +    GHashTable *h;
>  } StackObject;
>
>  struct QmpInputVisitor
> @@ -31,6 +32,7 @@ struct QmpInputVisitor
>     Visitor visitor;
>     StackObject stack[QIV_STACK_SIZE];
>     int nb_stack;
> +    bool strict;
>  };
>
>  static QmpInputVisitor *to_qiv(Visitor *v)
> @@ -45,6 +47,9 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>
>     if (qobj) {
>         if (name && qobject_type(qobj) == QTYPE_QDICT) {
> +            if (qiv->stack[qiv->nb_stack - 1].h) {
> +                g_hash_table_remove(qiv->stack[qiv->nb_stack - 1].h, name);
> +            }
>             return qdict_get(qobject_to_qdict(qobj), name);
>         } else if (qiv->stack[qiv->nb_stack - 1].entry) {
>             return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
> @@ -54,20 +59,47 @@ static QObject *qmp_input_get_object(QmpInputVisitor *qiv,
>     return qobj;
>  }
>
> +static void qdict_add_key(const char *key, QObject *obj, void *opaque)
> +{
> +    GHashTable *h = opaque;
> +    g_hash_table_insert(h, (gpointer) key, NULL);
> +}
> +
>  static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
>  {
> -    qiv->stack[qiv->nb_stack].obj = obj;
> -    qiv->stack[qiv->nb_stack].entry = NULL;
> -    qiv->nb_stack++;
> +    GHashTable *h;
>
>     if (qiv->nb_stack >= QIV_STACK_SIZE) {
>         error_set(errp, QERR_BUFFER_OVERRUN);
>         return;
>     }
> +
> +    qiv->stack[qiv->nb_stack].obj = obj;
> +    qiv->stack[qiv->nb_stack].entry = NULL;
> +    qiv->stack[qiv->nb_stack].h = NULL;
> +
> +    if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
> +        h = g_hash_table_new(g_str_hash, g_str_equal);
> +        qdict_iter(qobject_to_qdict(obj), qdict_add_key, h);
> +        qiv->stack[qiv->nb_stack].h = h;
> +    }
> +
> +    qiv->nb_stack++;
>  }
>
>  static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
>  {
> +    GHashTableIter iter;

GHashTableIter is alas not available in the glib (2.12) that
the distros we use at work run.  Is there a workaround for
this issue?

Thanks,

Laurent

> +    gpointer key;
> +
> +    if (qiv->strict && qiv->stack[qiv->nb_stack - 1].h) {
> +        g_hash_table_iter_init(&iter, qiv->stack[qiv->nb_stack - 1].h);
> +        if (g_hash_table_iter_next(&iter, &key, NULL)) {
> +            error_set(errp, QERR_QMP_EXTRA_MEMBER, (char *) key);
> +        }
> +        g_hash_table_unref(qiv->stack[qiv->nb_stack - 1].h);
> +    }
> +
>     assert(qiv->nb_stack > 0);
>     qiv->nb_stack--;
>  }
> @@ -257,3 +289,13 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
>
>     return v;
>  }
> +
> +QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj)
> +{
> +    QmpInputVisitor *v;
> +
> +    v = qmp_input_visitor_new(obj);
> +    v->strict = true;
> +
> +    return v;
> +}
> diff --git a/qapi/qmp-input-visitor.h b/qapi/qmp-input-visitor.h
> index 3f798f0..e0a48a5 100644
> --- a/qapi/qmp-input-visitor.h
> +++ b/qapi/qmp-input-visitor.h
> @@ -20,6 +20,8 @@
>  typedef struct QmpInputVisitor QmpInputVisitor;
>
>  QmpInputVisitor *qmp_input_visitor_new(QObject *obj);
> +QmpInputVisitor *qmp_input_visitor_new_strict(QObject *obj);
> +
>  void qmp_input_visitor_cleanup(QmpInputVisitor *v);
>
>  Visitor *qmp_input_get_visitor(QmpInputVisitor *v);
> diff --git a/test-qmp-input-strict.c b/test-qmp-input-strict.c
> new file mode 100644
> index 0000000..f6df8cb
> --- /dev/null
> +++ b/test-qmp-input-strict.c
> @@ -0,0 +1,234 @@
> +/*
> + * QMP Input Visitor unit-tests (strict mode).
> + *
> + * Copyright (C) 2011-2012 Red Hat Inc.
> + *
> + * Authors:
> + *  Luiz Capitulino <address@hidden>
> + *  Paolo Bonzini <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include <glib.h>
> +#include <stdarg.h>
> +
> +#include "qapi/qmp-input-visitor.h"
> +#include "test-qapi-types.h"
> +#include "test-qapi-visit.h"
> +#include "qemu-objects.h"
> +
> +typedef struct TestInputVisitorData {
> +    QObject *obj;
> +    QmpInputVisitor *qiv;
> +} TestInputVisitorData;
> +
> +static void validate_teardown(TestInputVisitorData *data,
> +                               const void *unused)
> +{
> +    qobject_decref(data->obj);
> +    data->obj = NULL;
> +
> +    if (data->qiv) {
> +        qmp_input_visitor_cleanup(data->qiv);
> +        data->qiv = NULL;
> +    }
> +}
> +
> +/* This is provided instead of a test setup function so that the JSON
> +   string used by the tests are kept in the test functions (and not
> +   int main()) */
> +static GCC_FMT_ATTR(2, 3)
> +Visitor *validate_test_init(TestInputVisitorData *data,
> +                             const char *json_string, ...)
> +{
> +    Visitor *v;
> +    va_list ap;
> +
> +    va_start(ap, json_string);
> +    data->obj = qobject_from_jsonv(json_string, &ap);
> +    va_end(ap);
> +
> +    g_assert(data->obj != NULL);
> +
> +    data->qiv = qmp_input_visitor_new_strict(data->obj);
> +    g_assert(data->qiv != NULL);
> +
> +    v = qmp_input_get_visitor(data->qiv);
> +    g_assert(v != NULL);
> +
> +    return v;
> +}
> +
> +typedef struct TestStruct
> +{
> +    int64_t integer;
> +    bool boolean;
> +    char *string;
> +} TestStruct;
> +
> +static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
> +                                  const char *name, Error **errp)
> +{
> +    visit_start_struct(v, (void **)obj, "TestStruct", name, 
> sizeof(TestStruct),
> +                       errp);
> +
> +    visit_type_int(v, &(*obj)->integer, "integer", errp);
> +    visit_type_bool(v, &(*obj)->boolean, "boolean", errp);
> +    visit_type_str(v, &(*obj)->string, "string", errp);
> +
> +    visit_end_struct(v, errp);
> +}
> +
> +static void test_validate_struct(TestInputVisitorData *data,
> +                                  const void *unused)
> +{
> +    TestStruct *p = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'integer': -42, 'boolean': true, 
> 'string': 'foo' }");
> +
> +    visit_type_TestStruct(v, &p, NULL, &errp);
> +    g_assert(!error_is_set(&errp));
> +    g_free(p->string);
> +    g_free(p);
> +}
> +
> +static void test_validate_struct_nested(TestInputVisitorData *data,
> +                                         const void *unused)
> +{
> +    UserDefNested *udp = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'string0': 'string0', 'dict1': { 
> 'string1': 'string1', 'dict2': { 'userdef1': { 'integer': 42, 'string': 
> 'string' }, 'string2': 'string2'}}}");
> +
> +    visit_type_UserDefNested(v, &udp, NULL, &errp);
> +    g_assert(!error_is_set(&errp));
> +    qapi_free_UserDefNested(udp);
> +}
> +
> +static void test_validate_list(TestInputVisitorData *data,
> +                                const void *unused)
> +{
> +    UserDefOneList *head = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "[ { 'string': 'string0', 'integer': 42 }, 
> { 'string': 'string1', 'integer': 43 }, { 'string': 'string2', 'integer': 44 
> } ]");
> +
> +    visit_type_UserDefOneList(v, &head, NULL, &errp);
> +    g_assert(!error_is_set(&errp));
> +    qapi_free_UserDefOneList(head);
> +}
> +
> +static void test_validate_union(TestInputVisitorData *data,
> +                                 const void *unused)
> +{
> +    UserDefUnion *tmp = NULL;
> +    Visitor *v;
> +    Error *errp = NULL;
> +
> +    v = validate_test_init(data, "{ 'type': 'b', 'data' : { 'integer': 42 } 
> }");
> +
> +    visit_type_UserDefUnion(v, &tmp, NULL, &errp);
> +    g_assert(!error_is_set(&errp));
> +    qapi_free_UserDefUnion(tmp);
> +}
> +
> +static void test_validate_fail_struct(TestInputVisitorData *data,
> +                                       const void *unused)
> +{
> +    TestStruct *p = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'integer': -42, 'boolean': true, 
> 'string': 'foo', 'extra': 42 }");
> +
> +    visit_type_TestStruct(v, &p, NULL, &errp);
> +    g_assert(error_is_set(&errp));
> +    if (p) {
> +        g_free(p->string);
> +    }
> +    g_free(p);
> +}
> +
> +static void test_validate_fail_struct_nested(TestInputVisitorData *data,
> +                                              const void *unused)
> +{
> +    UserDefNested *udp = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'string0': 'string0', 'dict1': { 
> 'string1': 'string1', 'dict2': { 'userdef1': { 'integer': 42, 'string': 
> 'string', 'extra': [42, 23, {'foo':'bar'}] }, 'string2': 'string2'}}}");
> +
> +    visit_type_UserDefNested(v, &udp, NULL, &errp);
> +    g_assert(error_is_set(&errp));
> +    qapi_free_UserDefNested(udp);
> +}
> +
> +static void test_validate_fail_list(TestInputVisitorData *data,
> +                                     const void *unused)
> +{
> +    UserDefOneList *head = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "[ { 'string': 'string0', 'integer': 42 }, 
> { 'string': 'string1', 'integer': 43 }, { 'string': 'string2', 'integer': 44, 
> 'extra': 'ggg' } ]");
> +
> +    visit_type_UserDefOneList(v, &head, NULL, &errp);
> +    g_assert(error_is_set(&errp));
> +    qapi_free_UserDefOneList(head);
> +}
> +
> +static void test_validate_fail_union(TestInputVisitorData *data,
> +                                      const void *unused)
> +{
> +    UserDefUnion *tmp = NULL;
> +    Error *errp = NULL;
> +    Visitor *v;
> +
> +    v = validate_test_init(data, "{ 'type': 'b', 'data' : { 'integer': 42 }, 
> 'extra': 'yyy' }");
> +
> +    visit_type_UserDefUnion(v, &tmp, NULL, &errp);
> +    g_assert(error_is_set(&errp));
> +    qapi_free_UserDefUnion(tmp);
> +}
> +
> +static void validate_test_add(const char *testpath,
> +                               TestInputVisitorData *data,
> +                               void (*test_func)(TestInputVisitorData *data, 
> const void *user_data))
> +{
> +    g_test_add(testpath, TestInputVisitorData, data, NULL, test_func,
> +               validate_teardown);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    TestInputVisitorData testdata;
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    validate_test_add("/visitor/input-strict/pass/struct",
> +                       &testdata, test_validate_struct);
> +    validate_test_add("/visitor/input-strict/pass/struct-nested",
> +                       &testdata, test_validate_struct_nested);
> +    validate_test_add("/visitor/input-strict/pass/list",
> +                       &testdata, test_validate_list);
> +    validate_test_add("/visitor/input-strict/pass/union",
> +                       &testdata, test_validate_union);
> +    validate_test_add("/visitor/input-strict/fail/struct",
> +                       &testdata, test_validate_fail_struct);
> +    validate_test_add("/visitor/input-strict/fail/struct-nested",
> +                       &testdata, test_validate_fail_struct_nested);
> +    validate_test_add("/visitor/input-strict/fail/list",
> +                       &testdata, test_validate_fail_list);
> +    validate_test_add("/visitor/input-strict/fail/union",
> +                       &testdata, test_validate_fail_union);
> +
> +    g_test_run();
> +
> +    return 0;
> +}
> diff --git a/tests/Makefile b/tests/Makefile
> index c78ade1..31349f7 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -15,7 +15,7 @@ check-qfloat: check-qfloat.o qfloat.o $(tools-obj-y)
>  check-qjson: check-qjson.o $(qobject-obj-y) $(tools-obj-y)
>  test-coroutine: test-coroutine.o qemu-timer-common.o async.o 
> $(coroutine-obj-y) $(tools-obj-y)
>
> -test-qmp-input-visitor.o test-qmp-output-visitor.o \
> +test-qmp-input-visitor.o test-qmp-output-visitor.o test-qmp-input-strict.o \
>  test-string-input-visitor.o test-string-output-visitor.o \
>        test-qmp-commands.o qemu-ga$(EXESUF): QEMU_CFLAGS += -I $(qapi-dir)
>
> @@ -36,6 +36,9 @@ test-string-output-visitor: test-string-output-visitor.o 
> $(qobject-obj-y) $(qapi
>  test-string-input-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c 
> test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
>  test-string-input-visitor: test-string-input-visitor.o $(qobject-obj-y) 
> $(qapi-obj-y) $(tools-obj-y) $(qapi-dir)/test-qapi-visit.o 
> $(qapi-dir)/test-qapi-types.o
>
> +test-qmp-input-strict.o: $(addprefix $(qapi-dir)/, test-qapi-types.c 
> test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
> +test-qmp-input-strict: test-qmp-input-strict.o $(qobject-obj-y) 
> $(qapi-obj-y) $(tools-obj-y) $(qapi-dir)/test-qapi-visit.o 
> $(qapi-dir)/test-qapi-types.o
> +
>  test-qmp-output-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c 
> test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) $(qapi-obj-y)
>  test-qmp-output-visitor: test-qmp-output-visitor.o $(qobject-obj-y) 
> $(qapi-obj-y) $(tools-obj-y) $(qapi-dir)/test-qapi-visit.o 
> $(qapi-dir)/test-qapi-types.o
>
> --
> 1.7.9.1
>
>
>