QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"Juan Quintela" <quintela@redhat.com>,
	"Michael Roth" <mdroth@linux.vnet.ibm.com>,
	qemu-devel@nongnu.org, "Paolo Bonzini" <pbonzini@redhat.com>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [PATCH for-5.0 2/3] qmp: fix leak on callbacks that return both value and error
Date: Mon, 30 Mar 2020 16:59:57 +0200
Message-ID: <87pnctubv6.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20200325184723.2029630-3-marcandre.lureau@redhat.com> (=?utf-8?Q?=22Marc-Andr=C3=A9?= Lureau"'s message of "Wed, 25 Mar 2020 19:47:22 +0100")

Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com>
> ---
>  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.



  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-25 18:47 [PATCH for-5.0 0/3] Memory leak fixes Marc-André Lureau
2020-03-25 18:47 ` [PATCH for-5.0 1/3] migration: fix cleanup_bh leak on resume Marc-André Lureau
2020-03-26  2:40   ` Juan Quintela
2020-03-25 18:47 ` [PATCH for-5.0 2/3] qmp: fix leak on callbacks that return both value and error Marc-André Lureau
2020-03-30 14:59   ` Markus Armbruster [this message]
2020-03-25 18:47 ` [PATCH for-5.0 3/3] object-add: don't create return value if failed Marc-André Lureau
2020-03-25 20:43   ` Philippe Mathieu-Daudé
2020-03-26  9:42   ` Paolo Bonzini
2020-03-30 14:48     ` Markus Armbruster

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87pnctubv6.fsf@dusky.pond.sub.org \
    --to=armbru@redhat.com \
    --cc=berrange@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=marcandre.lureau@redhat.com \
    --cc=mdroth@linux.vnet.ibm.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=quintela@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git