From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42672) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UxlKV-0003PS-5d for qemu-devel@nongnu.org; Fri, 12 Jul 2013 17:56:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UxlKS-0005LJ-Sj for qemu-devel@nongnu.org; Fri, 12 Jul 2013 17:56:43 -0400 Sender: fluxion Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <20130712104202.6a99c3f0@redhat.com> References: <20130712104202.6a99c3f0@redhat.com> Message-ID: <20130712215636.21868.56603@loki> Date: Fri, 12 Jul 2013 16:56:36 -0500 Subject: Re: [Qemu-devel] [PATCH] qapi: qapi-commands: fix possible leaks on visitor dealloc List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino , qemu-devel@nongnu.org Cc: pbonzini@redhat.com, aliguori@us.ibm.com, lersek@redhat.com, qemu-stable@nongnu.org Quoting Luiz Capitulino (2013-07-12 09:42:02) > In qmp-marshal.c the dealloc visitor calls use the same errp > pointer of the input visitor calls. This means that if any of > the input visitor calls fails, then the dealloc visitor will > return early, before freeing the object's memory. > = > Here's an example, consider this code: > = > int qmp_marshal_input_block_passwd(Monitor *mon, const QDict *qdict, QObj= ect **ret) > { > [...] > = > char * device =3D NULL; > char * password =3D NULL; > = > mi =3D qmp_input_visitor_new_strict(QOBJECT(args)); > v =3D qmp_input_get_visitor(mi); > visit_type_str(v, &device, "device", errp); > visit_type_str(v, &password, "password", errp); > qmp_input_visitor_cleanup(mi); > = > if (error_is_set(errp)) { > goto out; > } > qmp_block_passwd(device, password, errp); > = > out: > md =3D qapi_dealloc_visitor_new(); > v =3D qapi_dealloc_get_visitor(md); > visit_type_str(v, &device, "device", errp); > visit_type_str(v, &password, "password", errp); > qapi_dealloc_visitor_cleanup(md); > = > [...] > = > return 0; > } > = > Consider errp !=3D NULL when the out label is reached, we're going > to leak device and password. > = > This patch fixes this by always passing errp=3DNULL for dealloc > visitors, meaning that we always try to free them regardless of > any previous failure. The above example would then be: > = > out: > md =3D qapi_dealloc_visitor_new(); > v =3D qapi_dealloc_get_visitor(md); > visit_type_str(v, &device, "device", NULL); > visit_type_str(v, &password, "password", NULL); > qapi_dealloc_visitor_cleanup(md); > = > Signed-off-by: Luiz Capitulino > Reviewed-by: Laszlo Ersek Reviewed-by: Michael Roth > --- > = > o rfc > = > - Fixed missing spaces after ',' > - Reworded commitlog a bit > = > scripts/qapi-commands.py | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > = > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index e06332b..b12b696 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -128,12 +128,15 @@ bool has_%(argname)s =3D false; > = > def gen_visitor_input_block(args, obj, dealloc=3DFalse): > ret =3D "" > + errparg =3D 'errp' > + > if len(args) =3D=3D 0: > return ret > = > push_indent() > = > if dealloc: > + errparg =3D 'NULL' > ret +=3D mcgen(''' > md =3D qapi_dealloc_visitor_new(); > v =3D qapi_dealloc_get_visitor(md); > @@ -148,22 +151,22 @@ v =3D qmp_input_get_visitor(mi); > for argname, argtype, optional, structured in parse_args(args): > if optional: > ret +=3D mcgen(''' > -visit_start_optional(v, &has_%(c_name)s, "%(name)s", errp); > +visit_start_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s); > if (has_%(c_name)s) { > ''', > - c_name=3Dc_var(argname), name=3Dargname) > + c_name=3Dc_var(argname), name=3Dargname, errp= =3Derrparg) > push_indent() > ret +=3D mcgen(''' > -%(visitor)s(v, &%(c_name)s, "%(name)s", errp); > +%(visitor)s(v, &%(c_name)s, "%(name)s", %(errp)s); > ''', > c_name=3Dc_var(argname), name=3Dargname, argtype=3D= argtype, > - visitor=3Dtype_visitor(argtype)) > + visitor=3Dtype_visitor(argtype), errp=3Derrparg) > if optional: > pop_indent() > ret +=3D mcgen(''' > } > -visit_end_optional(v, errp); > -''') > +visit_end_optional(v, %(errp)s); > +''', errp=3Derrparg) > = > if dealloc: > ret +=3D mcgen(''' > @@ -194,7 +197,7 @@ static void qmp_marshal_output_%(c_name)s(%(c_ret_typ= e)s ret_in, QObject **ret_o > } > qmp_output_visitor_cleanup(mo); > v =3D qapi_dealloc_get_visitor(md); > - %(visitor)s(v, &ret_in, "unused", errp); > + %(visitor)s(v, &ret_in, "unused", NULL); > qapi_dealloc_visitor_cleanup(md); > } > ''', > -- = > 1.8.1.4