From patchwork Fri Jul 12 14:42:02 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luiz Capitulino X-Patchwork-Id: 258758 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 8415C2C0347 for ; Sat, 13 Jul 2013 00:50:14 +1000 (EST) Received: from localhost ([::1]:47474 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uxefj-0005Ys-Ph for incoming@patchwork.ozlabs.org; Fri, 12 Jul 2013 10:50:11 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39632) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UxefM-0005Lb-V8 for qemu-devel@nongnu.org; Fri, 12 Jul 2013 10:49:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UxefL-0004bk-LY for qemu-devel@nongnu.org; Fri, 12 Jul 2013 10:49:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:61694) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UxeYS-0002ET-C1; Fri, 12 Jul 2013 10:42:40 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r6CEg6d8022321 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 12 Jul 2013 10:42:07 -0400 Received: from localhost (ovpn-116-92.ams2.redhat.com [10.36.116.92]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r6CEg3J6002464; Fri, 12 Jul 2013 10:42:03 -0400 Date: Fri, 12 Jul 2013 10:42:02 -0400 From: Luiz Capitulino To: qemu-devel@nongnu.org Message-ID: <20130712104202.6a99c3f0@redhat.com> Organization: Red Hat Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: aliguori@us.ibm.com, mdroth@linux.vnet.ibm.com, qemu-stable@nongnu.org, pbonzini@redhat.com, lersek@redhat.com Subject: [Qemu-devel] [PATCH] qapi: qapi-commands: fix possible leaks on visitor dealloc X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 --- 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 = 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); } ''',