On Wed, 07 May 2014 18:55:26 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Peter Lieven <p...@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-sta...@nongnu.org
> >>> Signed-off-by: Peter Lieven <p...@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 <ebl...@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!
