Re: [Qemu-devel] [PATCH] qapi: fix null pointer dereference on invalid parameter

[Luiz Capitulino]

On Wed, 07 May 2014 18:55:26 +0200
Markus Armbruster <address@hidden> wrote:

> Peter Lieven <address@hidden> 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: address@hidden
> >>> Signed-off-by: Peter Lieven <address@hidden>
> >>> ---
> >>>   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 <address@hidden>
> >>
> >> 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!