From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:34024)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1Wi6ky-000554-Dj
	for qemu-devel@nongnu.org; Wed, 07 May 2014 14:39:56 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <lcapitulino@redhat.com>) id 1Wi6ku-0002Pz-13
	for qemu-devel@nongnu.org; Wed, 07 May 2014 14:39:52 -0400
Date: Wed, 7 May 2014 14:39:41 -0400
From: Luiz Capitulino <lcapitulino@redhat.com>
Message-ID: <20140507143941.389c074b@redhat.com>
In-Reply-To: <874n11wmtd.fsf@blackfin.pond.sub.org>
References: <1399422257-5912-1-git-send-email-pl@kamp.de>
	<5369A2E2.3010604@redhat.com> <536A404D.9090000@kamp.de>
	<874n11wmtd.fsf@blackfin.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH] qapi: fix null pointer dereference on
 invalid parameter
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-stable@nongnu.org, Peter Lieven <pl@kamp.de>, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>

On Wed, 07 May 2014 18:55:26 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Peter Lieven <pl@kamp.de> writes:
> 
> > On 07.05.2014 05:05, Eric Blake wrote:
> >> On 05/06/2014 06:24 PM, Peter Lieven wrote:
> >>> qemu segfaults if it receives an invalid parameter via a
> >>> qmp command instead of throwing an error.
> >>>
> >>> For example:
> >>> { "execute": "blockdev-add",
> >>>      "arguments": { "options" : { "driver": "invalid-driver" } } }
> >>>
> >>> CC: qemu-stable@nongnu.org
> >>> Signed-off-by: Peter Lieven <pl@kamp.de>
> >>> ---
> >>>   qapi/qapi-dealloc-visitor.c |    4 +++-
> >>>   1 file changed, 3 insertions(+), 1 deletion(-)
> >> Does this overlap with any of Markus' work? It emphasizes how bad it is
> >> that our visitor interface callbacks are undocumented on what they can
> >> be passed and are expected to return.
> >>
> >> https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg00225.html
> >>
> >>> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> >>> index d0ea118..dc53545 100644
> >>> --- a/qapi/qapi-dealloc-visitor.c
> >>> +++ b/qapi/qapi-dealloc-visitor.c
> >>> @@ -131,7 +131,9 @@ static void qapi_dealloc_end_list(Visitor *v, Error **errp)
> >>>   static void qapi_dealloc_type_str(Visitor *v, char **obj, const char *name,
> >>>                                     Error **errp)
> >>>   {
> >>> -    g_free(*obj);
> >>> +    if (obj) {
> >>> +        g_free(*obj);
> >>> +    }
> >>>   }
> >> As for solving a crash,
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >>
> >> But if Markus' cleanups solve the problem by guaranteeing that the
> >> cleanup visitor is never passed a NULL obj, then this would be a dead
> >> check.  I'm not familiar enough with the rest of the visitor code to
> >> know whether the caller is at fault, or whether other callers or visitor
> >> callbacks have the same bug.
> >>
> >
> >
> > Markus, can you advise please.
> 
> My series doesn't address this problem, and I can in fact reproduce the
> crash with it applied.
> 
> Your fix effectively reverts my commit 25a7017.  Let's turn it into a
> proper revert:

Which means that Peter has to repost as a real revert, right?

Peter, I'd appreciate if you do that shortly. I'd like to include that
fix in my next pull request.

Thanks!