From patchwork Wed Jun 15 17:37:51 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 9179013 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id B6BCE60573 for ; Wed, 15 Jun 2016 17:38:27 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A7B9627CEA for ; Wed, 15 Jun 2016 17:38:27 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 9BA4227F17; Wed, 15 Jun 2016 17:38:27 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 124C627CEA for ; Wed, 15 Jun 2016 17:38:27 +0000 (UTC) Received: from localhost ([::1]:44055 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDElm-0006wM-75 for patchwork-qemu-devel@patchwork.kernel.org; Wed, 15 Jun 2016 13:38:26 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34078) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDElP-0006um-L7 for qemu-devel@nongnu.org; Wed, 15 Jun 2016 13:38:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bDElJ-0002mY-LM for qemu-devel@nongnu.org; Wed, 15 Jun 2016 13:38:02 -0400 Received: from resqmta-po-05v.sys.comcast.net ([2001:558:fe16:19:96:114:154:164]:55725) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDElJ-0002mJ-DD for qemu-devel@nongnu.org; Wed, 15 Jun 2016 13:37:57 -0400 Received: from resomta-po-09v.sys.comcast.net ([96.114.154.233]) by resqmta-po-05v.sys.comcast.net with SMTP id DElAbCSpy3MWRDElIbkqyn; Wed, 15 Jun 2016 17:37:56 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=comcast.net; s=q20140121; t=1466012276; bh=HhMCKEezkkTxfR7H2QdVrInn9r+UvvPGey8j4RMoYgU=; h=Received:Received:From:To:Subject:Date:Message-Id; b=D22+9W/eOLRZ+RGy7OPeXgDBg0XHvdIzDgW7zXMsiH1I3yNXsqtLmknOWS374JKlR 7lGOa0sQTwl4MgIIl4WQPuZXumGtd3KZY5ADuWKVuWYnoE8GxsQR453riX5+MGxRvb 7DqMCEjQ9lrUUO4H4MEIAfVOVZGTAPvayUOwibQBMndmGSBnnE0SiPw++NQq2oiZsf ioGIn/Q0Gc8PlOx+l964PKewekOlNAPH+Qocaexcl3w6km+cD1Lvc66wUdV5jtWOOm rtLxwSvOX1PQU+ZG99DjEe3L9iv1rrGhz1W3H8I/7kuAMUL5lPNNQVtrinR536PWmE 2RsoY/sZ5wDvQ== Received: from red.redhat.com ([24.10.254.122]) by resomta-po-09v.sys.comcast.net with comcast id 75ds1t00o2fD5rL015dvRm; Wed, 15 Jun 2016 17:37:56 +0000 From: Eric Blake To: qemu-devel@nongnu.org Date: Wed, 15 Jun 2016 11:37:51 -0600 Message-Id: <1466012271-5204-1-git-send-email-eblake@redhat.com> X-Mailer: git-send-email 2.5.5 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2001:558:fe16:19:96:114:154:164 Subject: [Qemu-devel] [PATCH] qapi: Fix crash on missing alternate member of QAPI struct X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Michael Roth , qemu-stable@nongnu.org, mreitz@redhat.com, armbru@redhat.com Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP If a QAPI struct has a mandatory alternate member which is not present on input, the input visitor reports an error for the missing alternate without setting the discriminator, but the cleanup code for the struct still tries to use the dealloc visitor to clean up the alternate. Commit dbf11922 changed visit_start_alternate to set *obj to NULL when an error occurs, where it was previously left untouched. Thus, before the patch, the dealloc visitor is blindly trying to cleanup whatever branch corresponds to (*obj)->type == 0 (that is, QTYPE_NONE, because *obj still pointed to zeroed memory), which selects the default branch of the switch and sets an error, but this second error is ignored by the way the dealloc visitor is used; but after the patch, the attempt to switch dereferences NULL. When cleaning up after a partial object parse, we specifically check for !*obj after visit_start_struct() (see gen_visit_object()); doing the same for alternates fixes the crash. Enhance the testsuite to give coverage for both missing struct and missing alternate members. Also add an abort - we expect visit_start_alternate() to either set an error or to set (*obj)->type to a valid QType that corresponds to actual user input, and QTYPE_NONE should never be reachable from valid input. Had the abort() been in place earlier, we might have noticed the dealloc visitor dereferencing bogus zeroed memory prior to when commit dbf11922 forced our hand by setting *obj to NULL and causing a fault. Test case: {'execute':'blockdev-add', 'arguments':{'options':{'driver':'raw'}}} The choice of 'driver':'raw' selects a BlockdevOptionsGenericFormat struct, which has a mandatory 'file':'BlockdevRef' in QAPI. Since 'file' is missing as a sibling of 'driver', this should report a graceful error rather than fault. After this patch, we are back to: {"error": {"class": "GenericError", "desc": "Parameter 'file' is missing"}} Generated code in qapi-visit.c changes as: |@@ -2444,6 +2444,9 @@ void visit_type_BlockdevRef(Visitor *v, | if (err) { | goto out; | } |+ if (!*obj) { |+ goto out_obj; |+ } | switch ((*obj)->type) { | case QTYPE_QDICT: | visit_start_struct(v, name, NULL, 0, &err); |@@ -2459,10 +2462,13 @@ void visit_type_BlockdevRef(Visitor *v, | case QTYPE_QSTRING: | visit_type_str(v, name, &(*obj)->u.reference, &err); | break; |+ case QTYPE_NONE: |+ abort(); | default: | error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", | "BlockdevRef"); | } |+out_obj: | visit_end_alternate(v); Reported by Kashyap Chamarthy CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Tested-by: Kashyap Chamarthy --- scripts/qapi-visit.py | 6 ++++++ tests/test-qmp-input-visitor.c | 12 ++++++++++++ 2 files changed, 18 insertions(+) diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 70ea8ca..ffb635c 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -172,6 +172,9 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error if (err) { goto out; } + if (!*obj) { + goto out_obj; + } switch ((*obj)->type) { ''', c_name=c_name(name), promote_int=promote_int) @@ -206,10 +209,13 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error ''') ret += mcgen(''' + case QTYPE_NONE: + abort(); default: error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "%(name)s"); } +out_obj: visit_end_alternate(v); if (err && visit_is_input(v)) { qapi_free_%(c_name)s(*obj); diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index 3b6b39e..1a4585c 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -766,6 +766,8 @@ static void test_visitor_in_errors(TestInputVisitorData *data, Error *err = NULL; Visitor *v; strList *q = NULL; + UserDefTwo *r = NULL; + WrapAlternate *s = NULL; v = visitor_input_test_init(data, "{ 'integer': false, 'boolean': 'foo', " "'string': -42 }"); @@ -778,6 +780,16 @@ static void test_visitor_in_errors(TestInputVisitorData *data, visit_type_strList(v, NULL, &q, &err); error_free_or_abort(&err); assert(!q); + + v = visitor_input_test_init(data, "{ 'str':'hi' }"); + visit_type_UserDefTwo(v, NULL, &r, &err); + error_free_or_abort(&err); + assert(!r); + + v = visitor_input_test_init(data, "{ }"); + visit_type_WrapAlternate(v, NULL, &s, &err); + error_free_or_abort(&err); + assert(!s); } static void test_visitor_in_wrong_type(TestInputVisitorData *data,