| Message ID | 1399422257-5912-1-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
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.
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
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...
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!
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.
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,
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(-)