From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46399) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRslB-0006sj-Mi for qemu-devel@nongnu.org; Wed, 10 Sep 2014 21:01:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XRsl3-0003it-7u for qemu-devel@nongnu.org; Wed, 10 Sep 2014 21:01:17 -0400 Received: from e8.ny.us.ibm.com ([32.97.182.138]:35541) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRsl3-0003ip-4u for qemu-devel@nongnu.org; Wed, 10 Sep 2014 21:01:09 -0400 Received: from /spool/local by e8.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 10 Sep 2014 21:01:08 -0400 Received: from b01cxnp22034.gho.pok.ibm.com (b01cxnp22034.gho.pok.ibm.com [9.57.198.24]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 860B138C8059 for ; Wed, 10 Sep 2014 21:01:06 -0400 (EDT) Received: from d01av01.pok.ibm.com (d01av01.pok.ibm.com [9.56.224.215]) by b01cxnp22034.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id s8B116D58519946 for ; Thu, 11 Sep 2014 01:01:06 GMT Received: from d01av01.pok.ibm.com (localhost [127.0.0.1]) by d01av01.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s8B1166b006038 for ; Wed, 10 Sep 2014 21:01:06 -0400 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <1410352239-8705-1-git-send-email-famz@redhat.com> References: <1410352239-8705-1-git-send-email-famz@redhat.com> Message-ID: <20140911010104.32021.52493@loki> Date: Wed, 10 Sep 2014 20:01:04 -0500 Subject: Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Markus Armbruster , Luiz Capitulino , Stefan Hajnoczi , Paolo Bonzini Quoting Fam Zheng (2014-09-10 07:30:39) > We shouldn't do anything in the switch block in enum's visit_type_ > function, when the enum data's ->kind is not valid at all. This happens > when the dealloc visitor is called, after qmp input visitor returned > error. Comment is kind of confusing. Enum doesn't really have a .kind, this is more specifically regarding QAPI Union types who are missing a valid discriminator/.kind field. > = > Now, the input visitor will set ->kind to _MAX if the value is not > found, so that in dealloc, the switch block knows to skip calling into > specific type's visiting functions. Hmm, since QAPI input visitors handle the allocation for constructed types, and generally do so with g_malloc0, I wonder if we could possibly just make _INVALID correspond to 0 for discriminators? I think that way we could avoid needed to have input_type_enum pass the state along? In any case, since the fix doesn't break existing tests, could you break the iotest out into a separate patch? I also don't like that the iotest doesn't run during the normal make check, where I think we should try to capture all our QAPI tests. I've added a test to test-qmp-input-strict.c. Will send the patch shortly. If it looks reasonable could you add it to your series? > = > The added test case would trigger SIGSEGV without this fix. > = > This crash is introduced since commit b1de5f43 (QMP: Add support for > Archipelago). > = > Signed-off-by: Fam Zheng > --- > qapi/qapi-visit-core.c | 12 +++++------- > scripts/qapi-visit.py | 6 ++++++ > tests/qemu-iotests/087 | 17 +++++++++++++++++ > tests/qemu-iotests/087.out | 13 +++++++++++++ > 4 files changed, 41 insertions(+), 7 deletions(-) > = > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index 55f8d40..6c46e0e 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -276,23 +276,21 @@ void input_type_enum(Visitor *v, int *obj, const ch= ar *strings[], > = > visit_type_str(v, &enum_str, name, &local_err); > if (local_err) { > - error_propagate(errp, local_err); > - return; > + enum_str =3D NULL; > } > = > while (strings[value] !=3D NULL) { > - if (strcmp(strings[value], enum_str) =3D=3D 0) { > + if (enum_str && strcmp(strings[value], enum_str) =3D=3D 0) { > break; > } > value++; > } > = > - if (strings[value] =3D=3D NULL) { > - error_set(errp, QERR_INVALID_PARAMETER, enum_str); > - g_free(enum_str); > - return; > + if (!local_err && strings[value] =3D=3D NULL) { > + error_set(&local_err, QERR_INVALID_PARAMETER, enum_str); > } > = > g_free(enum_str); > *obj =3D value; > + error_propagate(errp, local_err); > } > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index c129697..dad7561 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -379,6 +379,12 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj,= const char *name, Error **e > c_name=3Dc_fun(key)) > = > ret +=3D mcgen(''' > + case %(enum_full_value)s: > + break; > +''', > + enum_full_value =3D generate_enum_full_value(disc_type, = 'MAX')) > + > + ret +=3D mcgen(''' > default: > abort(); > } > diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087 > index 82c56b1..d7454d1 100755 > --- a/tests/qemu-iotests/087 > +++ b/tests/qemu-iotests/087 > @@ -218,6 +218,23 @@ run_qemu < { "execute": "quit" } > EOF > = > +echo > +echo =3D=3D=3D Missing driver =3D=3D=3D > +echo > + > +_make_test_img -o encryption=3Don $size > +run_qemu -S < +{ "execute": "qmp_capabilities" } > +{ "execute": "blockdev-add", > + "arguments": { > + "options": { > + "id": "disk" > + } > + } > + } > +{ "execute": "quit" } > +EOF > + > # success, all done > echo "*** done" > rm -f $seq.full > diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out > index 7fbee3f..f16bad0 100644 > --- a/tests/qemu-iotests/087.out > +++ b/tests/qemu-iotests/087.out > @@ -64,4 +64,17 @@ QMP_VERSION > {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "even= t": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} > {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "even= t": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} > = > + > +=3D=3D=3D Missing driver =3D=3D=3D > + > +Formatting 'TEST_DIR/t.IMGFMT', fmt=3DIMGFMT size=3D134217728 encryption= =3Don = > +Testing: -S > +QMP_VERSION > +{"return": {}} > +{"error": {"class": "GenericError", "desc": "Invalid parameter type for = 'driver', expected: string"}} > +{"return": {}} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "even= t": "SHUTDOWN"} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "even= t": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "even= t": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} > + > *** done > -- = > 1.9.3