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

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

[Peter Lieven]

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(-)

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);
+    }
 }
 
 static void qapi_dealloc_type_int(Visitor *v, int64_t *obj, const char *name,
-- 
1.7.9.5

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1697 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Peter Lieven]

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.

Peter

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

[Markus Armbruster]

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:

    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: qemu-stable@nongnu.org
    Signed-off-by: Peter Lieven <pl@kamp.de>
    Reviewed-by: Eric Blake <eblake@redhat.com>
    Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

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

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

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

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.