Luiz Capitulino <lcapitul...@redhat.com> writes:
> 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?
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.
