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

[Markus Armbruster]

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:

    Revert "qapi: Clean up superfluous null check in qapi_dealloc_type_str()"
    
    This reverts commit 25a7017555f1b4aeb543b5d323ff4afb8f9c5437.
    
    Turns out the argument *can* be null: QEMU now 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>
    Reviewed-by: Eric Blake <address@hidden>
    Reviewed-by: Markus Armbruster <address@hidden>

The deallocation visitor is more special than I (naively) thought...