Re: [Qemu-devel] [PATCH v4 12/17] qapi: check invalid arguments on no-args commands

[Marc-André Lureau]

Hi

On Wed, Aug 17, 2016 at 6:49 PM Markus Armbruster <address@hidden> wrote:

> address@hidden writes:
>
> > From: Marc-André Lureau <address@hidden>
> >
> > The generated marshal functions do not visit arguments from commands
> > that take no arguments. Thus they fail to catch invalid
> > members. Visit the arguments, if provided, to throw an error in case of
> > invalid members.
> >
> > Currently, qmp_check_client_args() checks for invalid arguments and
> > correctly catches this case. When switching to qmp_dispatch() we want to
> > keep that behaviour. The commands using 'O' may have arbitrary
> > arguments, and must have 'gen': false in the qapi schema to skip the
> > generated checks.
>
> Explains why this isn't a bug fix for QMP.  What about QGA?
>

Sorry, I don't understand what you ask. I thought the above paragraph that
I added described the current QMP behaviour and why we want to keep it. And
yes, it'a fix for qga too (it's qapi)


>
> > Old/new diff:
> > static void qmp_marshal_stop(QDict *args, QObject **ret, Error **errp)
> >  {
> > +    Visitor *v = NULL;
> >      Error *err = NULL;
> > -
>
> Please keep the blank line between declarations and statements.
>



>
> > -    (void)args;
> > +    if (args) {
>
> This code...
>
> > +        v = qmp_input_visitor_new(QOBJECT(args), true);
> > +        visit_start_struct(v, NULL, NULL, 0, &err);
> > +        if (err) {
> > +            goto out;
> > +        }
> > +
> > +        if (!err) {
> > +            visit_check_struct(v, &err);
> > +        }
> > +        visit_end_struct(v, NULL);
> > +        if (err) {
> > +            goto out;
> > +        }
>
> ... is not indented in my build.
>
> > +    }
> >
> >      qmp_stop(&err);
> > +
> > +out:
> >      error_propagate(errp, err);
> > +    visit_free(v);
> > +    if (args) {
> > +        v = qapi_dealloc_visitor_new();
> > +        visit_start_struct(v, NULL, NULL, 0, NULL);
> > +
> > +        visit_end_struct(v, NULL);
> > +        visit_free(v);
>
> Likewise.
>
> > +    }
> >  }
> >
> > The new code closely resembles code for a command with arguments.
> > Differences:
> > - the visit of the argument and its cleanup struct don't visit any
> >   members (because there are none).
> > - the visit of the argument struct and its cleanup are conditional.
>
> Additional diff for all other qmp_marshal_FOO():
>
>   @@ -46,9 +46,9 @@ static void qmp_marshal_output_AddfdInfo
>
>    void qmp_marshal_add_fd(QDict *args, QObject **ret, Error **errp)
>    {
>   +    Visitor *v = NULL;
>        Error *err = NULL;
>        AddfdInfo *retval;
>   -    Visitor *v;
>        q_obj_add_fd_arg arg = {0};
>
>        v = qmp_input_visitor_new(QOBJECT(args), true);
>
> Let's avoid this churn.
>
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  tests/test-qmp-commands.c | 15 ++++++++++++++
> >  scripts/qapi-commands.py  | 53
> ++++++++++++++++++++++++++++++-----------------
> >  2 files changed, 49 insertions(+), 19 deletions(-)
> >
> > diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
> > index 261fd9e..81cbe54 100644
> > --- a/tests/test-qmp-commands.c
> > +++ b/tests/test-qmp-commands.c
> > @@ -106,6 +106,7 @@ static void test_dispatch_cmd(void)
> >  static void test_dispatch_cmd_failure(void)
> >  {
> >      QDict *req = qdict_new();
> > +    QDict *args = qdict_new();
> >      QObject *resp;
> >
> >      qdict_put_obj(req, "execute",
> QOBJECT(qstring_from_str("user_def_cmd2")));
> > @@ -114,6 +115,20 @@ static void test_dispatch_cmd_failure(void)
> >      assert(resp != NULL);
> >      assert(qdict_haskey(qobject_to_qdict(resp), "error"));
> >
> > +    qobject_decref(resp);
> > +    QDECREF(req);
> > +
> > +    /* check that with extra arguments it throws an error */
> > +    req = qdict_new();
> > +    qdict_put(args, "a", qint_from_int(66));
> > +    qdict_put(req, "arguments", args);
> > +
> > +    qdict_put_obj(req, "execute",
> QOBJECT(qstring_from_str("user_def_cmd")));
> > +
> > +    resp = qmp_dispatch(QOBJECT(req));
> > +    assert(resp != NULL);
> > +    assert(qdict_haskey(qobject_to_qdict(resp), "error"));
> > +
> >      qobject_decref(resp);
> >      QDECREF(req);
> >  }
> > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> > index eac64ce..9c79b18 100644
> > --- a/scripts/qapi-commands.py
> > +++ b/scripts/qapi-commands.py
> > @@ -94,11 +94,28 @@ def gen_marshal_decl(name):
> >                   proto=gen_marshal_proto(name))
> >
> >
> > +def if_args(arg_type, block):
> > +    ret = ''
> > +    if not arg_type or arg_type.is_empty():
> > +        ret += mcgen('''
> > +    if (args) {
> > +''')
> > +        push_indent()
> > +    ret += block
> > +    if not arg_type or arg_type.is_empty():
> > +        pop_indent()
> > +        ret += mcgen('''
> > +    }
> > +''')
>
> Since @block has already been formatted, the push_indent() /
> pop_indent() has no effect.  I guess that's why you did it with a lambda
> in v3.
>
> > +    return ret
> > +
> > +
> >  def gen_marshal(name, arg_type, boxed, ret_type):
> >      ret = mcgen('''
> >
> >  %(proto)s
> >  {
> > +    Visitor *v = NULL;
> >      Error *err = NULL;
> >  ''',
> >                  proto=gen_marshal_proto(name))
> > @@ -109,17 +126,23 @@ def gen_marshal(name, arg_type, boxed, ret_type):
> >  ''',
> >                       c_type=ret_type.c_type())
> >
> > +    visit_members = ''
> >      if arg_type and not arg_type.is_empty():
> > +        visit_members = 'visit_type_%s_members(v, &arg, &err);' % \
> > +                        arg_type.c_name()
>
> PEP8 prefers
>
>            visit_members = ('visit_type_%s_members(v, &arg, &err);'
>                              % arg_type.c_name())
>
> >          ret += mcgen('''
> > -    Visitor *v;
> >      %(c_name)s arg = {0};
> >
> > +''',
> > +                     c_name=arg_type.c_name())
> > +
> > +    ret += if_args(arg_type, mcgen('''
> >      v = qmp_input_visitor_new(QOBJECT(args), true);
> >      visit_start_struct(v, NULL, NULL, 0, &err);
> >      if (err) {
> >          goto out;
> >      }
> > -    visit_type_%(c_name)s_members(v, &arg, &err);
> > +    %(visit_members)s
> >      if (!err) {
> >          visit_check_struct(v, &err);
> >      }
> > @@ -128,35 +151,27 @@ def gen_marshal(name, arg_type, boxed, ret_type):
> >          goto out;
> >      }
> >  ''',
> > -                     c_name=arg_type.c_name())
> > -
> > -    else:
> > -        ret += mcgen('''
> > -
> > -    (void)args;
> > -''')
> > +                                   visit_members=visit_members))
> >
> >      ret += gen_call(name, arg_type, boxed, ret_type)
> >
> > -    # 'goto out' produced above for arg_type, and by gen_call() for
> ret_type
> > -    if (arg_type and not arg_type.is_empty()) or ret_type:
> > -        ret += mcgen('''
> > +    if arg_type and not arg_type.is_empty():
> > +        visit_members = mcgen('visit_type_%(c_name)s_members(v, &arg,
> NULL);',
> > +                              c_name=arg_type.c_name())
>
> I'm afraid you missed this instance of "mcgen()'s output fed to
> mcgen()".
>
> > +    ret += mcgen('''
> >
> >  out:
> > -''')
> > -    ret += mcgen('''
> >      error_propagate(errp, err);
> > -''')
> > -    if arg_type and not arg_type.is_empty():
> > -        ret += mcgen('''
> >      visit_free(v);
> > +''')
> > +    ret += if_args(arg_type, mcgen('''
> >      v = qapi_dealloc_visitor_new();
> >      visit_start_struct(v, NULL, NULL, 0, NULL);
> > -    visit_type_%(c_name)s_members(v, &arg, NULL);
> > +    %(visit_members)s
> >      visit_end_struct(v, NULL);
> >      visit_free(v);
> >  ''',
> > -                     c_name=arg_type.c_name())
> > +                                   visit_members=visit_members))
> >
> >      ret += mcgen('''
> >  }
>
> I append a diff from this one to a stupider solution.  It's slightly
> longer, but I find it a bit easier to understand.
>

ok, applied


>
>
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 9c79b18..2f603b0 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -94,28 +94,13 @@ def gen_marshal_decl(name):
>                   proto=gen_marshal_proto(name))
>
>
> -def if_args(arg_type, block):
> -    ret = ''
> -    if not arg_type or arg_type.is_empty():
> -        ret += mcgen('''
> -    if (args) {
> -''')
> -        push_indent()
> -    ret += block
> -    if not arg_type or arg_type.is_empty():
> -        pop_indent()
> -        ret += mcgen('''
> -    }
> -''')
> -    return ret
> -
> -
>  def gen_marshal(name, arg_type, boxed, ret_type):
> +    have_args = arg_type and not arg_type.is_empty()
> +
>      ret = mcgen('''
>
>  %(proto)s
>  {
> -    Visitor *v = NULL;
>      Error *err = NULL;
>  ''',
>                  proto=gen_marshal_proto(name))
> @@ -126,17 +111,25 @@ def gen_marshal(name, arg_type, boxed, ret_type):
>  ''',
>                       c_type=ret_type.c_type())
>
> -    visit_members = ''
> -    if arg_type and not arg_type.is_empty():
> -        visit_members = 'visit_type_%s_members(v, &arg, &err);' % \
> -                        arg_type.c_name()
> +    if have_args:
> +        visit_members = ('visit_type_%s_members(v, &arg, &err);'
> +                         % arg_type.c_name())
>          ret += mcgen('''
> +    Visitor *v;
>      %(c_name)s arg = {0};
>
>  ''',
>                       c_name=arg_type.c_name())
> +    else:
> +        visit_members = ''
> +        ret += mcgen('''
> +    Visitor *v = NULL;
>
> -    ret += if_args(arg_type, mcgen('''
> +    if (args) {
> +''')
> +        push_indent()
> +
> +    ret += mcgen('''
>      v = qmp_input_visitor_new(QOBJECT(args), true);
>      visit_start_struct(v, NULL, NULL, 0, &err);
>      if (err) {
> @@ -151,27 +144,47 @@ def gen_marshal(name, arg_type, boxed, ret_type):
>          goto out;
>      }
>  ''',
> -                                   visit_members=visit_members))
> +                 visit_members=visit_members)
> +
> +    if not have_args:
> +        pop_indent()
> +        ret += mcgen('''
> +    }
> +''')
>
>      ret += gen_call(name, arg_type, boxed, ret_type)
>
> -    if arg_type and not arg_type.is_empty():
> -        visit_members = mcgen('visit_type_%(c_name)s_members(v, &arg,
> NULL);',
> -                              c_name=arg_type.c_name())
>      ret += mcgen('''
>
>  out:
>      error_propagate(errp, err);
>      visit_free(v);
>  ''')
> -    ret += if_args(arg_type, mcgen('''
> +
> +    if have_args:
> +        visit_members = ('visit_type_%s_members(v, &arg, NULL);'
> +                         % arg_type.c_name())
> +    else:
> +        visit_members = ''
> +        ret += mcgen('''
> +    if (args) {
> +''')
> +        push_indent()
> +
> +    ret += mcgen('''
>      v = qapi_dealloc_visitor_new();
>      visit_start_struct(v, NULL, NULL, 0, NULL);
>      %(visit_members)s
>      visit_end_struct(v, NULL);
>      visit_free(v);
>  ''',
> -                                   visit_members=visit_members))
> +                 visit_members=visit_members)
> +
> +    if not have_args:
> +        pop_indent()
> +        ret += mcgen('''
> +    }
> +''')
>
>      ret += mcgen('''
>  }
>
> --
Marc-André Lureau