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

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> 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?

You could also replace Peter's commit message by mine.  I ran
git-revert, double-checked the patch is the same, worked Peter's message
into the commit message, including his S-o-b, and topped it off with
R-bys.

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

The result should be the same.