[PATCH for-5.0 0/3] Memory leak fixes

[PATCH for-5.0 0/3] Memory leak fixes

[Marc-André Lureau]

Hi,

A small series of leaks spotted by ASAN during "make check".

Marc-André Lureau (3):
  migration: fix cleanup_bh leak on resume
  qmp: fix leak on callbacks that return both value and error
  object-add: don't create return value if failed

 migration/migration.c | 7 ++++++-
 qapi/qmp-dispatch.c   | 2 ++
 qom/qom-qmp-cmds.c    | 2 +-
 3 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.26.0.rc2.42.g98cedd0233

[PATCH for-5.0 1/3] migration: fix cleanup_bh leak on resume

[Marc-André Lureau]

Since commit 8c6b0356b53977bcfdea5299db07884915425b0c ("util/async:
make bh_aio_poll() O(1)"), migration-test reveals a leak:

QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64
tests/qtest/migration-test  -p /x86_64/migration/postcopy/recovery
tests/qtest/libqtest.c:140: kill_qemu() tried to terminate QEMU
process but encountered exit status 1 (expected 0)

=================================================================
==2082571==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 40 byte(s) in 1 object(s) allocated from:
    #0 0x7f25971dfc58 in __interceptor_malloc (/lib64/libasan.so.5+0x10dc58)
    #1 0x7f2596d08358 in g_malloc (/lib64/libglib-2.0.so.0+0x57358)
    #2 0x560970d006f8 in qemu_bh_new /home/elmarco/src/qemu/util/main-loop.c:532
    #3 0x5609704afa02 in migrate_fd_connect
/home/elmarco/src/qemu/migration/migration.c:3407
    #4 0x5609704b6b6f in migration_channel_connect
/home/elmarco/src/qemu/migration/channel.c:92
    #5 0x5609704b2bfb in socket_outgoing_migration
/home/elmarco/src/qemu/migration/socket.c:108
    #6 0x560970b9bd6c in qio_task_complete /home/elmarco/src/qemu/io/task.c:196
    #7 0x560970b9aa97 in qio_task_thread_result
/home/elmarco/src/qemu/io/task.c:111
    #8 0x7f2596cfee3a  (/lib64/libglib-2.0.so.0+0x4de3a)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 migration/migration.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index c1d88ace7f..fa8cca14c7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3477,7 +3477,12 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
     bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
 
     s->expected_downtime = s->parameters.downtime_limit;
-    s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup_bh, s);
+    if (resume) {
+        assert(s->cleanup_bh);
+    } else {
+        assert(!s->cleanup_bh);
+        s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup_bh, s);
+    }
     if (error_in) {
         migrate_fd_error(s, error_in);
         migrate_fd_cleanup(s);
-- 
2.26.0.rc2.42.g98cedd0233

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

[Marc-André Lureau]

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;
     }
 
-- 
2.26.0.rc2.42.g98cedd0233

[PATCH for-5.0 3/3] object-add: don't create return value if failed

[Marc-André Lureau]

If object-add failed, no need to create a return value that may later
be leaked.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qom/qom-qmp-cmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 435193b036..6bd137ccbf 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -287,8 +287,8 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
     visit_free(v);
     if (obj) {
         object_unref(obj);
+        *ret_data = QOBJECT(qdict_new());
     }
-    *ret_data = QOBJECT(qdict_new());
 }
 
 void qmp_object_del(const char *id, Error **errp)
-- 
2.26.0.rc2.42.g98cedd0233

Re: [PATCH for-5.0 3/3] object-add: don't create return value if failed

[Philippe Mathieu-Daudé]

On 3/25/20 7:47 PM, Marc-André Lureau wrote:
> If object-add failed, no need to create a return value that may later
> be leaked.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   qom/qom-qmp-cmds.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index 435193b036..6bd137ccbf 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -287,8 +287,8 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
>       visit_free(v);
>       if (obj) {
>           object_unref(obj);
> +        *ret_data = QOBJECT(qdict_new());
>       }
> -    *ret_data = QOBJECT(qdict_new());
>   }
>   
>   void qmp_object_del(const char *id, Error **errp)
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH for-5.0 1/3] migration: fix cleanup_bh leak on resume

[Juan Quintela]

Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> Since commit 8c6b0356b53977bcfdea5299db07884915425b0c ("util/async:
> make bh_aio_poll() O(1)"), migration-test reveals a leak:
>
> QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64
> tests/qtest/migration-test  -p /x86_64/migration/postcopy/recovery
> tests/qtest/libqtest.c:140: kill_qemu() tried to terminate QEMU
> process but encountered exit status 1 (expected 0)
>
> =================================================================
> ==2082571==ERROR: LeakSanitizer: detected memory leaks
>
> Direct leak of 40 byte(s) in 1 object(s) allocated from:
>     #0 0x7f25971dfc58 in __interceptor_malloc (/lib64/libasan.so.5+0x10dc58)
>     #1 0x7f2596d08358 in g_malloc (/lib64/libglib-2.0.so.0+0x57358)
>     #2 0x560970d006f8 in qemu_bh_new /home/elmarco/src/qemu/util/main-loop.c:532
>     #3 0x5609704afa02 in migrate_fd_connect
> /home/elmarco/src/qemu/migration/migration.c:3407
>     #4 0x5609704b6b6f in migration_channel_connect
> /home/elmarco/src/qemu/migration/channel.c:92
>     #5 0x5609704b2bfb in socket_outgoing_migration
> /home/elmarco/src/qemu/migration/socket.c:108
>     #6 0x560970b9bd6c in qio_task_complete /home/elmarco/src/qemu/io/task.c:196
>     #7 0x560970b9aa97 in qio_task_thread_result
> /home/elmarco/src/qemu/io/task.c:111
>     #8 0x7f2596cfee3a  (/lib64/libglib-2.0.so.0+0x4de3a)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH for-5.0 3/3] object-add: don't create return value if failed

[Paolo Bonzini]

On 25/03/20 19:47, Marc-André Lureau wrote:
> If object-add failed, no need to create a return value that may later
> be leaked.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qom/qom-qmp-cmds.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index 435193b036..6bd137ccbf 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -287,8 +287,8 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
>      visit_free(v);
>      if (obj) {
>          object_unref(obj);
> +        *ret_data = QOBJECT(qdict_new());
>      }
> -    *ret_data = QOBJECT(qdict_new());
>  }
>  
>  void qmp_object_del(const char *id, Error **errp)
> 

It can be slightly simplified:

------------------- 8< ----------------------
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: [PATCH] object-add: don't create return value if failed
    
No need to return an empty value from object-add (it would also leak
if the command failed).  While at it, remove the "if" around object_unref
since object_unref handles NULL arguments just fine.
    
Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 435193b036..e47ebe8ed1 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -285,10 +285,7 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
     v = qobject_input_visitor_new(QOBJECT(qdict));
     obj = user_creatable_add_type(type, id, qdict, v, errp);
     visit_free(v);
-    if (obj) {
-        object_unref(obj);
-    }
-    *ret_data = QOBJECT(qdict_new());
+    object_unref(obj);
 }
 
 void qmp_object_del(const char *id, Error **errp)


I queued this patch and your other two.  Thanks,

Paolo

Re: [PATCH for-5.0 3/3] object-add: don't create return value if failed

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 25/03/20 19:47, Marc-André Lureau wrote:
>> If object-add failed, no need to create a return value that may later
>> be leaked.
>> 
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  qom/qom-qmp-cmds.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
>> index 435193b036..6bd137ccbf 100644
>> --- a/qom/qom-qmp-cmds.c
>> +++ b/qom/qom-qmp-cmds.c
>> @@ -287,8 +287,8 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
>>      visit_free(v);
>>      if (obj) {
>>          object_unref(obj);
>> +        *ret_data = QOBJECT(qdict_new());
>>      }
>> -    *ret_data = QOBJECT(qdict_new());
>>  }
>>  
>>  void qmp_object_del(const char *id, Error **errp)
>> 
>
> It can be slightly simplified:
>
> ------------------- 8< ----------------------
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH] object-add: don't create return value if failed
>     
> No need to return an empty value from object-add (it would also leak
> if the command failed).  While at it, remove the "if" around object_unref
> since object_unref handles NULL arguments just fine.
>     
> Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index 435193b036..e47ebe8ed1 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -285,10 +285,7 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp)
>      v = qobject_input_visitor_new(QOBJECT(qdict));
>      obj = user_creatable_add_type(type, id, qdict, v, errp);
>      visit_free(v);
> -    if (obj) {
> -        object_unref(obj);
> -    }
> -    *ret_data = QOBJECT(qdict_new());
> +    object_unref(obj);
>  }
>  
>  void qmp_object_del(const char *id, Error **errp)

Yes, that's better.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

> I queued this patch and your other two.  Thanks,
>
> Paolo

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

[Markus Armbruster]

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.