From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45146) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFSWS-0001QS-0q for qemu-devel@nongnu.org; Mon, 17 Feb 2014 13:02:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFSWL-0004Xe-U6 for qemu-devel@nongnu.org; Mon, 17 Feb 2014 13:02:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42355) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFSWL-0004XS-L2 for qemu-devel@nongnu.org; Mon, 17 Feb 2014 13:02:21 -0500 Message-ID: <1392660102.25748.34.camel@localhost.localdomain> From: Marcel Apfelbaum In-Reply-To: <53024915.9090702@redhat.com> References: <1392637972-24719-1-git-send-email-marcel.a@redhat.com> <53024915.9090702@redhat.com> Content-Type: text/plain; charset="UTF-8" Date: Mon, 17 Feb 2014 20:01:42 +0200 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qapi: output visitor crashes qemu if it encounters a NULL value List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: pbonzini@redhat.com, lcapitulino@redhat.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com On Mon, 2014-02-17 at 10:38 -0700, Eric Blake wrote: > On 02/17/2014 04:52 AM, Marcel Apfelbaum wrote: > > 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. > > Do you have an easy formula for reproducing the crash? Hi Eric, thank you for your review! In order to reproduce this you need to use object_property_get_str on an object with a string property with a null value. I don't know if in the current code base we have this scenario, but I am trying to QOMify the QemuMachine and properties as "kernel" may be NULL. Either way (if NULL properties are not wanted), IMHO it is recommended to cover such cases in order to avoid QEMU crash. > > > > > Signed-off-by: Marcel Apfelbaum > > --- > > qapi/qmp-output-visitor.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c > > index 74a5684..0562f49 100644 > > --- a/qapi/qmp-output-visitor.c > > +++ b/qapi/qmp-output-visitor.c > > @@ -66,6 +66,11 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov) > > static QObject *qmp_output_first(QmpOutputVisitor *qov) > > { > > QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack); > > + > > + if (!e) { > > + return NULL; > > + } > > + > > The code looks okay to me, but without a formula, my review is fairly weak: Appreciated, Marcel > > Reviewed-by: Eric Blake >