From 3af0cd08a331f166f5073218dff6a9a0b989564a Mon Sep 17 00:00:00 2001 Message-Id: <3af0cd08a331f166f5073218dff6a9a0b989564a.1375709547.git.minovotn@redhat.com> In-Reply-To: <3351a61f3d3d17e088298983ea624dd8995fb1a5.1375709547.git.minovotn@redhat.com> References: <3351a61f3d3d17e088298983ea624dd8995fb1a5.1375709547.git.minovotn@redhat.com> From: Luiz Capitulino Date: Tue, 30 Jul 2013 21:47:02 +0200 Subject: [PATCH 4/4] qapi: qapi-commands: fix possible leaks on visitor dealloc RH-Author: Luiz Capitulino Message-id: <20130730174702.44448ce2@redhat.com> Patchwork-id: 52829 O-Subject: [RHEL6.5 qemu-kvm PATCH] qapi: qapi-commands: fix possible leaks on visitor dealloc Bugzilla: 990316 RH-Acked-by: Laszlo Ersek RH-Acked-by: Paolo Bonzini RH-Acked-by: Markus Armbruster Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=990316 Upstream-status: Merged 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, QObject **ret) { [...] char * device = NULL; char * password = NULL; mi = qmp_input_visitor_new_strict(QOBJECT(args)); v = 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 = qapi_dealloc_visitor_new(); v = 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 != NULL when the out label is reached, we're going to leak device and password. This patch fixes this by always passing errp=NULL for dealloc visitors, meaning that we always try to free them regardless of any previous failure. The above example would then be: out: md = qapi_dealloc_visitor_new(); v = 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 (cherry picked from commit 8f91ad8a1b4702966d91ea58cd90bbde1faea1b3) --- scripts/qapi-commands.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) Signed-off-by: Michal Novotny --- 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 5e86a17..0b3f039 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -128,12 +128,15 @@ bool has_%(argname)s = false; def gen_visitor_input_block(args, obj, dealloc=False): ret = "" + errparg = 'errp' + if len(args) == 0: return ret push_indent() if dealloc: + errparg = 'NULL' ret += mcgen(''' md = qapi_dealloc_visitor_new(); v = qapi_dealloc_get_visitor(md); @@ -148,22 +151,22 @@ v = qmp_input_get_visitor(mi); for argname, argtype, optional, structured in parse_args(args): if optional: ret += 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=c_var(argname), name=argname) + c_name=c_var(argname), name=argname, errp=errparg) push_indent() ret += mcgen(''' -%(visitor)s(v, &%(c_name)s, "%(name)s", errp); +%(visitor)s(v, &%(c_name)s, "%(name)s", %(errp)s); ''', c_name=c_var(argname), name=argname, argtype=argtype, - visitor=type_visitor(argtype)) + visitor=type_visitor(argtype), errp=errparg) if optional: pop_indent() ret += mcgen(''' } -visit_end_optional(v, errp); -''') +visit_end_optional(v, %(errp)s); +''', errp=errparg) if dealloc: ret += mcgen(''' @@ -194,7 +197,7 @@ static void qmp_marshal_output_%(c_name)s(%(c_ret_type)s ret_in, QObject **ret_o } qmp_output_visitor_cleanup(mo); v = 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.7.11.7