Re: [Qemu-devel] [PATCH v3 10/15] qapi: check invalid arguments on no-args commands

[Marc-André Lureau]

Hi

----- Original Message -----
> 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.
> 
> Begs the question why this isn't a bug fix :)
> 
> For QMP, it isn't, because check_client_args_type() does the same work.
> It's called by handle_qmp_command() via qmp_check_client_args().  You
> remove it in PATCH 12, and that's why you tighten the marshaling
> functions' invalid arguments checking now.
> 
> What about QGA?
> 
> Back to QMP.  check_client_args_type() checks against the args_type
> defined in qmp-commands.hx.  Your generated code checks against the
> schema.  Good, because the args_type duplicate schema information, and
> your series gets rid of it.  However, there's a funny special case we
> need to consider: args_type character 'O' accepts arbitary arguments.
> check_client_args_type() skips the check for unknown arguments then.
> This requires commands using 'O' to have 'gen': false in the schema.
> This is the case.
> 
> The commit message should explain these things.

ok

> 
> > static void qmp_marshal_stop(QDict *args, QObject **ret, Error **errp)
> >  {
> > +    Visitor *v = NULL;
> >      Error *err = NULL;
> > -
> > -    (void)args;
> > +    if (args) {
> > +        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;
> > +        }
> > +    }
> >
> >      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);
> > +    }
> >  }
> 
> The new code closely resembles code for a command with arguments.
> That's a good sign.  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.
> 
> I think that's also worth explaining in the commit message.
> 
> However, this is not the only change to generated code: marshalers for
> commands with arguments also change, like this:
> 
>    static void qmp_marshal_netdev_del(QDict *args, QObject **ret, Error
>    **errp)
>    {
>   +    Visitor *v = NULL;
>        Error *err = NULL;
>   -    Visitor *v;
>        q_obj_netdev_del_arg arg = {0};
> 
>        v = qmp_input_visitor_new(QOBJECT(args), true);
> 
> The initialization is dead for commands with arguments.  For commands
> without arguments, it suppresses a bogus "may be used uninitialized"
> warning.  Oh well.
> 
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  tests/test-qmp-commands.c | 15 ++++++++++++++
> >  scripts/qapi-commands.py  | 51
> >  ++++++++++++++++++++++++++++++-----------------
> >  2 files changed, 48 insertions(+), 18 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
> 
> You remembered to add a test.  Appreciated!
> 
> > @@ -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")));
> 
> Matches how the other request objects are built.  qobject_from_json()
> could be simpler, though.  Your choice.
> 
> > +
> > +    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 11aa54b..735e463 100644
> > --- a/scripts/qapi-commands.py
> > +++ b/scripts/qapi-commands.py
> > @@ -97,11 +97,27 @@ def gen_marshal_decl(name, export):
> >                   proto=gen_marshal_proto(name, export))
> >  
> >  
> > +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, export):
> >      ret = mcgen('''
> >  
> >  %(proto)s
> >  {
> > +    Visitor *v = NULL;
> >      Error *err = NULL;
> >  ''',
> >                  proto=gen_marshal_proto(name, export))
> > @@ -112,17 +128,23 @@ def gen_marshal(name, arg_type, boxed, ret_type,
> > export):
> >  ''',
> >                       c_type=ret_type.c_type())
> >  
> > +    visit_members = ''
> >      if arg_type and not arg_type.is_empty():
> > +        visit_members = mcgen('visit_type_%(c_name)s_members(v, &arg,
> > &err);',
> > +                            c_name=arg_type.c_name())
> >          ret += mcgen('''
> > -    Visitor *v;
> >      %(c_name)s arg = {0};
> >  
> > +''',
> > +                     c_name=arg_type.c_name())
> > +
> > +    ret += if_args(arg_type, lambda: 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
> 
> Uh, visit_members is output of mcgen(), so this feeds output of mcgen()
> to mcgen() again.  Not good, because any interpolations done by the
> second mcgen() are almost certainly not wanted.  Likewise indentation.
> 
> The easiest fix is to say
> 
>         visit_members = ('visit_type_%(c_name)s_members(v, &arg, &err);'
>                          % arg_type.c_name())
> 

ok

> >      if (!err) {
> >          visit_check_struct(v, &err);
> >      }
> > @@ -131,35 +153,28 @@ def gen_marshal(name, arg_type, boxed, ret_type,
> > export):
> >          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
> 
> I think this comment becomes wrong, and should be dropped.
> 
> > -    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())
> > +    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, lambda: 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('''
> >  }
> 
> Encapsulating the conditional in if_args() and passing it lambda
> arguments is kind of cute, but is it worthwhile?  To find out, I rewrote
> this part the stupidest way I could, diff appended.  Turns out it's the
> same amount of code, only stupider code.
> 
> It also avoids the dead initialization of v, because it's easy to do.
> 
> I'm fine with clever when it saves repetition, but when it doesn't, I'll
> have stupid, please.

Well the differences is that in case there are no command args, then you can 
pass NULL to the function, while if there are expected args, you must pass args 
(or it will fail to report missing args actually). The if_args() wasn't just 
for adding "if (args)" but also to check if there are expected args (see also 
the diff before/after) 

> 
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 11aa54b..2e6cdb9 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -117,12 +117,26 @@ def gen_marshal(name, arg_type, boxed, ret_type,
> export):
>      Visitor *v;
>      %(c_name)s arg = {0};
>  
> +''',
> +                     c_name=arg_type.c_name())
> +        visit_members = ('visit_type_%s_members(v, &arg, &err);'
> +                         % arg_type.c_name())
> +    else:
> +        ret += mcgen('''
> +    Visitor *v = NULL;
> +
> +    if (args) {
> +''')
> +        push_indent();
> +        visit_members = ''
> +
> +    ret += 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);
>      }
> @@ -131,35 +145,39 @@ def gen_marshal(name, arg_type, boxed, ret_type,
> export):
>          goto out;
>      }
>  ''',
> -                     c_name=arg_type.c_name())
> -
> -    else:
> +                 visit_members=visit_members)
> +    if not arg_type or arg_type.is_empty():
> +        pop_indent()
>          ret += mcgen('''
> -
> -    (void)args;
> +    }
>  ''')
>  
>      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('''
> +    ret += mcgen('''
>  
>  out:
> -''')
> -    ret += mcgen('''
>      error_propagate(errp, err);
>  ''')
> -    if arg_type and not arg_type.is_empty():
> +    if not arg_type or arg_type.is_empty():
>          ret += mcgen('''
> +    if (args) {
> +''')
> +        push_indent();
> +    ret += mcgen('''
>      visit_free(v);
>      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)
> +    if not arg_type or arg_type.is_empty():
> +        pop_indent()
> +        ret += mcgen('''
> +    }
> +''')
>  
>      ret += mcgen('''
>  }
>