From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40722) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ba28U-0000eh-MQ for qemu-devel@nongnu.org; Wed, 17 Aug 2016 10:48:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ba28P-0001lm-K6 for qemu-devel@nongnu.org; Wed, 17 Aug 2016 10:48:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38344) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ba28P-0001lc-An for qemu-devel@nongnu.org; Wed, 17 Aug 2016 10:48:01 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8A5524E327 for ; Wed, 17 Aug 2016 14:48:00 +0000 (UTC) From: Markus Armbruster References: <20160810180235.32469-1-marcandre.lureau@redhat.com> <20160810180235.32469-13-marcandre.lureau@redhat.com> Date: Wed, 17 Aug 2016 16:47:57 +0200 In-Reply-To: <20160810180235.32469-13-marcandre.lureau@redhat.com> (marcandre lureau's message of "Wed, 10 Aug 2016 22:02:30 +0400") Message-ID: <87oa4rlaea.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 12/17] qapi: check invalid arguments on no-args commands List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: marcandre.lureau@redhat.com Cc: qemu-devel@nongnu.org marcandre.lureau@redhat.com writes: > From: Marc-Andr=C3=A9 Lureau > > 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? > Old/new diff: > static void qmp_marshal_stop(QDict *args, QObject **ret, Error **errp) > { > + Visitor *v =3D NULL; > Error *err =3D NULL; > - Please keep the blank line between declarations and statements. > - (void)args; > + if (args) { This code... > + v =3D 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 =3D 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 =3D NULL; Error *err =3D NULL; AddfdInfo *retval; - Visitor *v; q_obj_add_fd_arg arg =3D {0}; v =3D qmp_input_visitor_new(QOBJECT(args), true); Let's avoid this churn. > > Signed-off-by: Marc-Andr=C3=A9 Lureau > --- > 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 =3D qdict_new(); > + QDict *args =3D qdict_new(); > QObject *resp; >=20=20 > qdict_put_obj(req, "execute", QOBJECT(qstring_from_str("user_def_cmd= 2"))); > @@ -114,6 +115,20 @@ static void test_dispatch_cmd_failure(void) > assert(resp !=3D NULL); > assert(qdict_haskey(qobject_to_qdict(resp), "error")); >=20=20 > + qobject_decref(resp); > + QDECREF(req); > + > + /* check that with extra arguments it throws an error */ > + req =3D 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 =3D qmp_dispatch(QOBJECT(req)); > + assert(resp !=3D 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=3Dgen_marshal_proto(name)) >=20=20 >=20=20 > +def if_args(arg_type, block): > + ret =3D '' > + if not arg_type or arg_type.is_empty(): > + ret +=3D mcgen(''' > + if (args) { > +''') > + push_indent() > + ret +=3D block > + if not arg_type or arg_type.is_empty(): > + pop_indent() > + ret +=3D 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 =3D mcgen(''' >=20=20 > %(proto)s > { > + Visitor *v =3D NULL; > Error *err =3D NULL; > ''', > proto=3Dgen_marshal_proto(name)) > @@ -109,17 +126,23 @@ def gen_marshal(name, arg_type, boxed, ret_type): > ''', > c_type=3Dret_type.c_type()) >=20=20 > + visit_members =3D '' > if arg_type and not arg_type.is_empty(): > + visit_members =3D 'visit_type_%s_members(v, &arg, &err);' % \ > + arg_type.c_name() PEP8 prefers visit_members =3D ('visit_type_%s_members(v, &arg, &err);' % arg_type.c_name()) > ret +=3D mcgen(''' > - Visitor *v; > %(c_name)s arg =3D {0}; >=20=20 > +''', > + c_name=3Darg_type.c_name()) > + > + ret +=3D if_args(arg_type, mcgen(''' > v =3D 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=3Darg_type.c_name()) > - > - else: > - ret +=3D mcgen(''' > - > - (void)args; > -''') > + visit_members=3Dvisit_members)) >=20=20 > ret +=3D gen_call(name, arg_type, boxed, ret_type) >=20=20 > - # '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 +=3D mcgen(''' > + if arg_type and not arg_type.is_empty(): > + visit_members =3D mcgen('visit_type_%(c_name)s_members(v, &arg, = NULL);', > + c_name=3Darg_type.c_name()) I'm afraid you missed this instance of "mcgen()'s output fed to mcgen()". > + ret +=3D mcgen(''' >=20=20 > out: > -''') > - ret +=3D mcgen(''' > error_propagate(errp, err); > -''') > - if arg_type and not arg_type.is_empty(): > - ret +=3D mcgen(''' > visit_free(v); > +''') > + ret +=3D if_args(arg_type, mcgen(''' > v =3D 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=3Darg_type.c_name()) > + visit_members=3Dvisit_members)) >=20=20 > ret +=3D 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. 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=3Dgen_marshal_proto(name)) =20 =20 -def if_args(arg_type, block): - ret =3D '' - if not arg_type or arg_type.is_empty(): - ret +=3D mcgen(''' - if (args) { -''') - push_indent() - ret +=3D block - if not arg_type or arg_type.is_empty(): - pop_indent() - ret +=3D mcgen(''' - } -''') - return ret - - def gen_marshal(name, arg_type, boxed, ret_type): + have_args =3D arg_type and not arg_type.is_empty() + ret =3D mcgen(''' =20 %(proto)s { - Visitor *v =3D NULL; Error *err =3D NULL; ''', proto=3Dgen_marshal_proto(name)) @@ -126,17 +111,25 @@ def gen_marshal(name, arg_type, boxed, ret_type): ''', c_type=3Dret_type.c_type()) =20 - visit_members =3D '' - if arg_type and not arg_type.is_empty(): - visit_members =3D 'visit_type_%s_members(v, &arg, &err);' % \ - arg_type.c_name() + if have_args: + visit_members =3D ('visit_type_%s_members(v, &arg, &err);' + % arg_type.c_name()) ret +=3D mcgen(''' + Visitor *v; %(c_name)s arg =3D {0}; =20 ''', c_name=3Darg_type.c_name()) + else: + visit_members =3D '' + ret +=3D mcgen(''' + Visitor *v =3D NULL; =20 - ret +=3D if_args(arg_type, mcgen(''' + if (args) { +''') + push_indent() + + ret +=3D mcgen(''' v =3D 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=3Dvisit_members)) + visit_members=3Dvisit_members) + + if not have_args: + pop_indent() + ret +=3D mcgen(''' + } +''') =20 ret +=3D gen_call(name, arg_type, boxed, ret_type) =20 - if arg_type and not arg_type.is_empty(): - visit_members =3D mcgen('visit_type_%(c_name)s_members(v, &arg, NU= LL);', - c_name=3Darg_type.c_name()) ret +=3D mcgen(''' =20 out: error_propagate(errp, err); visit_free(v); ''') - ret +=3D if_args(arg_type, mcgen(''' + + if have_args: + visit_members =3D ('visit_type_%s_members(v, &arg, NULL);' + % arg_type.c_name()) + else: + visit_members =3D '' + ret +=3D mcgen(''' + if (args) { +''') + push_indent() + + ret +=3D mcgen(''' v =3D 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=3Dvisit_members)) + visit_members=3Dvisit_members) + + if not have_args: + pop_indent() + ret +=3D mcgen(''' + } +''') =20 ret +=3D mcgen(''' }