* [Qemu-devel] [PATCH] qapi: fix null pointer dereference on invalid parameter
@ 2014-05-07 0:24 Peter Lieven
2014-05-07 3:05 ` Eric Blake
0 siblings, 1 reply; 6+ messages in thread
From: Peter Lieven @ 2014-05-07 0:24 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-stable, Peter Lieven, mdroth, lcapitulino
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: fix null pointer dereference on invalid parameter
2014-05-07 0:24 [Qemu-devel] [PATCH] qapi: fix null pointer dereference on invalid parameter Peter Lieven
@ 2014-05-07 3:05 ` Eric Blake
2014-05-07 14:16 ` Peter Lieven
0 siblings, 1 reply; 6+ messages in thread
From: Eric Blake @ 2014-05-07 3:05 UTC (permalink / raw)
To: Peter Lieven, qemu-devel
Cc: Markus Armbruster, lcapitulino, qemu-stable, mdroth
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: fix null pointer dereference on invalid parameter
2014-05-07 3:05 ` Eric Blake
@ 2014-05-07 14:16 ` Peter Lieven
2014-05-07 16:55 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Peter Lieven @ 2014-05-07 14:16 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: Markus Armbruster, lcapitulino, qemu-stable, mdroth
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: fix null pointer dereference on invalid parameter
2014-05-07 14:16 ` Peter Lieven
@ 2014-05-07 16:55 ` Markus Armbruster
2014-05-07 18:39 ` Luiz Capitulino
0 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2014-05-07 16:55 UTC (permalink / raw)
To: Peter Lieven; +Cc: qemu-devel, mdroth, qemu-stable, Paolo Bonzini, lcapitulino
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...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: fix null pointer dereference on invalid parameter
2014-05-07 16:55 ` Markus Armbruster
@ 2014-05-07 18:39 ` Luiz Capitulino
2014-05-07 19:32 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Luiz Capitulino @ 2014-05-07 18:39 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-stable, Peter Lieven, mdroth, qemu-devel, Paolo Bonzini
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!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: fix null pointer dereference on invalid parameter
2014-05-07 18:39 ` Luiz Capitulino
@ 2014-05-07 19:32 ` Markus Armbruster
0 siblings, 0 replies; 6+ messages in thread
From: Markus Armbruster @ 2014-05-07 19:32 UTC (permalink / raw)
To: Luiz Capitulino
Cc: qemu-devel, Paolo Bonzini, Peter Lieven, qemu-stable, mdroth
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-05-07 19:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-07 0:24 [Qemu-devel] [PATCH] qapi: fix null pointer dereference on invalid parameter Peter Lieven
2014-05-07 3:05 ` Eric Blake
2014-05-07 14:16 ` Peter Lieven
2014-05-07 16:55 ` Markus Armbruster
2014-05-07 18:39 ` Luiz Capitulino
2014-05-07 19:32 ` Markus Armbruster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).