Re: [PATCH for-5.0 2/3] qmp: fix leak on callbacks that return both value and error

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Direct leak of 4120 byte(s) in 1 object(s) allocated from:
>     #0 0x7fa114931887 in __interceptor_calloc (/lib64/libasan.so.6+0xb0887)
>     #1 0x7fa1144ad8f0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x588f0)
>     #2 0x561e3c9c8897 in qmp_object_add 
> /home/elmarco/src/qemu/qom/qom-qmp-cmds.c:291
>     #3 0x561e3cf48736 in qmp_dispatch 
> /home/elmarco/src/qemu/qapi/qmp-dispatch.c:155
>     #4 0x561e3c8efb36 in monitor_qmp_dispatch 
> /home/elmarco/src/qemu/monitor/qmp.c:145
>     #5 0x561e3c8f09ed in monitor_qmp_bh_dispatcher 
> /home/elmarco/src/qemu/monitor/qmp.c:234
>     #6 0x561e3d08c993 in aio_bh_call /home/elmarco/src/qemu/util/async.c:136
>     #7 0x561e3d08d0a5 in aio_bh_poll /home/elmarco/src/qemu/util/async.c:164
>     #8 0x561e3d0a535a in aio_dispatch 
> /home/elmarco/src/qemu/util/aio-posix.c:380
>     #9 0x561e3d08e3ca in aio_ctx_dispatch 
> /home/elmarco/src/qemu/util/async.c:298
>     #10 0x7fa1144a776e in g_main_context_dispatch 
> (/lib64/libglib-2.0.so.0+0x5276e)
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  qapi/qmp-dispatch.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index c30c7ff9e1..79347e0864 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -155,6 +155,8 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject 
> *request,
>      cmd->fn(args, &ret, &err);
>      qobject_unref(args);
>      if (err) {
> +        /* or assert(!ret) after reviewing all handlers: */
> +        qobject_unref(ret);
>          goto out;
>      }

Returning both a value and an error is wrong.  We should assert to flush
out these errors.  Doing that close to the release would be imprudent,
though.

The next patch fixes the one known instance of this error pattern.  If
we want to guard against leaks for unknown instances in 5.0, then this
patch is okay.  I wouldn't bother myself.

If we keep this patch, its new comment should say TODO.  Paolo, perhaps
you can still fix that up.