From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46909) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wi2eb-0007q9-LB for qemu-devel@nongnu.org; Wed, 07 May 2014 10:17:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wi2eX-0007J0-1f for qemu-devel@nongnu.org; Wed, 07 May 2014 10:17:01 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:50930 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wi2eW-0007Gm-Np for qemu-devel@nongnu.org; Wed, 07 May 2014 10:16:56 -0400 Message-ID: <536A404D.9090000@kamp.de> Date: Wed, 07 May 2014 16:16:45 +0200 From: Peter Lieven MIME-Version: 1.0 References: <1399422257-5912-1-git-send-email-pl@kamp.de> <5369A2E2.3010604@redhat.com> In-Reply-To: <5369A2E2.3010604@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qapi: fix null pointer dereference on invalid parameter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Markus Armbruster , lcapitulino@redhat.com, qemu-stable@nongnu.org, mdroth@linux.vnet.ibm.com 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 >> --- >> 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 > > 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. Peter