From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59445) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wlwgb-0006Hs-FJ for qemu-devel@nongnu.org; Sun, 18 May 2014 04:43:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WlwgW-00061Z-L0 for qemu-devel@nongnu.org; Sun, 18 May 2014 04:43:13 -0400 Message-ID: <1400402568.12073.5.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Sun, 18 May 2014 11:42:48 +0300 In-Reply-To: <20140514203849.3192.91896@loki> References: <1399473780-20374-1-git-send-email-marcel.a@redhat.com> <1399473780-20374-3-git-send-email-marcel.a@redhat.com> <53725800.3030404@suse.de> <53726D96.8030404@redhat.com> <5373A11E.90005@suse.de> <1400088577.14303.45.camel@localhost.localdomain> <20140514142516.20269803@redhat.com> <20140514203849.3192.91896@loki> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/4] qapi: output visitor crashes qemu if it encounters a NULL value List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: mst@redhat.com, aik@ozlabs.ru, qemu-devel@nongnu.org, Luiz Capitulino , blauwirbel@gmail.com, jcmvbkbc@gmail.com, edgar.iglesias@gmail.com, gxt@mprc.pku.edu.cn, peter.chubb@nicta.com.au, proljc@gmail.com, agraf@suse.de, scottwood@freescale.com, borntraeger@de.ibm.com, hpoussin@reactos.org, aliguori@amazon.com, lersek@redhat.com, chouteau@adacore.com, jan.kiszka@web.de, stefanha@redhat.com, cornelia.huck@de.ibm.com, peter.crosthwaite@xilinx.com, mark.langsdorf@calxeda.com, armbru@redhat.com, michael@walle.cc, qemu-ppc@nongnu.org, pbonzini@redhat.com, Andreas =?ISO-8859-1?Q?F=E4rber?= , aurelien@aurel32.net On Wed, 2014-05-14 at 15:38 -0500, Michael Roth wrote: > Quoting Luiz Capitulino (2014-05-14 13:25:16) > > On Wed, 14 May 2014 20:29:37 +0300 > > Marcel Apfelbaum wrote: > >=20 > > > On Wed, 2014-05-14 at 19:00 +0200, Andreas F=C3=A4rber wrote: > > > > Am 13.05.2014 21:08, schrieb Eric Blake: > > > > > On 05/13/2014 11:36 AM, Andreas F=C3=A4rber wrote: > > > > >> Am 07.05.2014 16:42, schrieb Marcel Apfelbaum: > > > > >>> A NULL value is not added to visitor's stack, but there is no > > > > >>> check for that when the visitor tries to return that value, > > > > >>> leading to Qemu crash. > > > > >>>=20 > > > > >>> Reviewed-by: Eric Blake Signed-off-by: > > > > >>> Marcel Apfelbaum > > > > >>=20 > > > > >> Where does the Rb come from on this v1? Is it in any tree > > > > >> already? > > > > >>=20 > > > > >=20 > > > > > The (weak) R-b was here:=20 > > > > > https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg02861.= html > > > >=20 > > > > Thanks. > > > > >=20 > > > > So Luiz was okay with it too, but his last message seems to be > > > > indicating this needs to be fixed somewhere else, too: > > > >=20 > > > > https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg05228.ht= ml > > > > https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg00217.ht= ml > > > >=20 > > > > Can/should that be addressed as a follow-up? Or is there a test c= ase > > > > that breaks? > > > Simple and "popular" test case: the user does not use the -kernel-c= mdline parameter. > > > The patch is needed because otherwise the main function will fail > > > if no value is passed by the user to string parameters.=20 > > >=20 > > > Regarding Luiz's concern, it can be a follow-up as I am not aware o= f > > > any problem with that. > >=20 > > My concern was that I wasn't sure if this is the right fix for the is= sue > > or if it's papering over the real bug. I quickly checked the code and= it > > seemed to make sense, but I didn't have time to study it deeper. >=20 > Not sure the fix is bad or not, but the cause might be a little more su= btle > than NULL string values as mentioned in the other thread. QmpOutputVisi= tor > encodes NULL strings as "" via qmp_output_type_str(), so the problem do= esn't > seem to lie there: it shouldn't generate NULL values on the stack. >=20 > I think the real issue is that object_property_get_str() actually calls= an > accessor via property_get_str to get the string, then explicitly *skips= * > the call to visit_type_str() if it is NULL (as it would be in the case = of, > say, kernel_cmdline option being NULL). So I wonder if maybe the real i= ssue > we're fixing is a corner case where you call qmp_output_get_qobject() o= n > an "empty" QmpOutputVisitor. >=20 > Surprised that's not covered by tests, but didn't see any coverage doin= g > a cursory glance. Actually, might as well just add one.. >=20 > diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-vi= sitor.c > index e073d83..f190eaa 100644 > --- a/tests/test-qmp-output-visitor.c > +++ b/tests/test-qmp-output-visitor.c > @@ -434,6 +434,17 @@ static void test_visitor_out_union(TestOutputVisit= orData *data, > QDECREF(qdict); > } > =20 > +static void test_visitor_out_empty(TestOutputVisitorData *data, > + const void *unused) > +{ > + QObject *arg; > + QDict *qdict; > + > + arg =3D qmp_output_get_qobject(data->qov); > + qdict =3D qobject_to_qdict(arg); > + QDECREF(qdict); > +} > + > static void init_native_list(UserDefNativeListUnion *cvalue) > { > int i; > @@ -782,6 +793,8 @@ int main(int argc, char **argv) > &out_visitor_data, test_visitor_out_list_q= api_free); > output_visitor_test_add("/visitor/output/union", > &out_visitor_data, test_visitor_out_union)= ; > + output_visitor_test_add("/visitor/output/empty", > + &out_visitor_data, test_visitor_out_empty)= ; > output_visitor_test_add("/visitor/output/native_list/int", > &out_visitor_data, test_visitor_out_native= _list_int); > output_visitor_test_add("/visitor/output/native_list/int8", >=20 > mdroth@loki:~/w/qemu-build$ tests/test-qmp-output-visitor=20 > /visitor/output/int: OK > /visitor/output/bool: OK > /visitor/output/number: OK > /visitor/output/string: OK > /visitor/output/no-string: OK > /visitor/output/enum: OK > /visitor/output/enum-errors: OK > /visitor/output/struct: OK > /visitor/output/struct-nested: OK > /visitor/output/struct-errors: OK > /visitor/output/list: OK > /visitor/output/list-qapi-free: OK > /visitor/output/union: OK > /visitor/output/empty: Segmentation fault (core dumped) >=20 > So I guess the question is whether we should support converting an empt= y > QmpOutputVisitor to a QObject. I would say yes, and that a NULL value i= s > probably the most reasonable value. >=20 > I would ask that commit/code is a little more explicit about what corne= r case > is being handled though, and that something like the above unit test be > included with the series. Hi Michael, Thanks for the test, of course I'll add it, may I add your Signed-off-by? Marcel >=20 > >=20 > > We could ask Michael Roth or Anthony, but I wouldn't hold this series > > because of that. Here's my ACK if you need it: > >=20 > > Acked-by: Luiz Capitulino