migration: fix QEMUFile leak

[Qemu-devel] [PATCH] migration: fix QEMUFile leak

Posted by Marc-André Lureau 7 years, 1 month ago

Spotted by ASAN while running:

$ tests/migration-test -p /x86_64/migration/postcopy/recovery

=================================================================
==18034==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 33864 byte(s) in 1 object(s) allocated from:
    #0 0x7f3da7f31e50 in calloc (/lib64/libasan.so.5+0xeee50)
    #1 0x7f3da644441d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5241d)
    #2 0x55af9db15440 in qemu_fopen_channel_input /home/elmarco/src/qemu/migration/qemu-file-channel.c:183
    #3 0x55af9db15413 in channel_get_output_return_path /home/elmarco/src/qemu/migration/qemu-file-channel.c:159
    #4 0x55af9db0d4ac in qemu_file_get_return_path /home/elmarco/src/qemu/migration/qemu-file.c:78
    #5 0x55af9dad5e4f in open_return_path_on_source /home/elmarco/src/qemu/migration/migration.c:2295
    #6 0x55af9dadb3bf in migrate_fd_connect /home/elmarco/src/qemu/migration/migration.c:3111
    #7 0x55af9dae1bf3 in migration_channel_connect /home/elmarco/src/qemu/migration/channel.c:91
    #8 0x55af9daddeca in socket_outgoing_migration /home/elmarco/src/qemu/migration/socket.c:108
    #9 0x55af9e13d3db in qio_task_complete /home/elmarco/src/qemu/io/task.c:158
    #10 0x55af9e13ca03 in qio_task_thread_result /home/elmarco/src/qemu/io/task.c:89
    #11 0x7f3da643b1ca in g_idle_dispatch gmain.c:5535

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

diff --git a/migration/migration.c b/migration/migration.c
index 05d0a7296a..d214ea12c8 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2268,7 +2268,10 @@ out:
              */
             if (postcopy_pause_return_path_thread(ms)) {
                 /* Reload rp, reset the rest */
-                rp = ms->rp_state.from_dst_file;
+                if (rp != ms->rp_state.from_dst_file) {
+                    qemu_fclose(rp);
+                    rp = ms->rp_state.from_dst_file;
+                }
                 ms->rp_state.error = false;
                 goto retry;
             }
-- 
2.19.0

Re: [Qemu-devel] [PATCH] migration: fix QEMUFile leak

Posted by Dr. David Alan Gilbert 7 years, 1 month ago

* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> Spotted by ASAN while running:
> 
> $ tests/migration-test -p /x86_64/migration/postcopy/recovery
> 
> =================================================================
> ==18034==ERROR: LeakSanitizer: detected memory leaks
> 
> Direct leak of 33864 byte(s) in 1 object(s) allocated from:
>     #0 0x7f3da7f31e50 in calloc (/lib64/libasan.so.5+0xeee50)
>     #1 0x7f3da644441d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5241d)
>     #2 0x55af9db15440 in qemu_fopen_channel_input /home/elmarco/src/qemu/migration/qemu-file-channel.c:183
>     #3 0x55af9db15413 in channel_get_output_return_path /home/elmarco/src/qemu/migration/qemu-file-channel.c:159
>     #4 0x55af9db0d4ac in qemu_file_get_return_path /home/elmarco/src/qemu/migration/qemu-file.c:78
>     #5 0x55af9dad5e4f in open_return_path_on_source /home/elmarco/src/qemu/migration/migration.c:2295
>     #6 0x55af9dadb3bf in migrate_fd_connect /home/elmarco/src/qemu/migration/migration.c:3111
>     #7 0x55af9dae1bf3 in migration_channel_connect /home/elmarco/src/qemu/migration/channel.c:91
>     #8 0x55af9daddeca in socket_outgoing_migration /home/elmarco/src/qemu/migration/socket.c:108
>     #9 0x55af9e13d3db in qio_task_complete /home/elmarco/src/qemu/io/task.c:158
>     #10 0x55af9e13ca03 in qio_task_thread_result /home/elmarco/src/qemu/io/task.c:89
>     #11 0x7f3da643b1ca in g_idle_dispatch gmain.c:5535
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  migration/migration.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 05d0a7296a..d214ea12c8 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2268,7 +2268,10 @@ out:
>               */
>              if (postcopy_pause_return_path_thread(ms)) {
>                  /* Reload rp, reset the rest */
> -                rp = ms->rp_state.from_dst_file;
> +                if (rp != ms->rp_state.from_dst_file) {
> +                    qemu_fclose(rp);
> +                    rp = ms->rp_state.from_dst_file;
> +                }
>                  ms->rp_state.error = false;
>                  goto retry;
>              }

Yep, I think that's right (cc'ing in Peter);


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> -- 
> 2.19.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: fix QEMUFile leak

Posted by Peter Xu 7 years, 1 month ago

On Tue, Sep 25, 2018 at 10:37:38AM +0100, Dr. David Alan Gilbert wrote:
> * Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> > Spotted by ASAN while running:
> > 
> > $ tests/migration-test -p /x86_64/migration/postcopy/recovery
> > 
> > =================================================================
> > ==18034==ERROR: LeakSanitizer: detected memory leaks
> > 
> > Direct leak of 33864 byte(s) in 1 object(s) allocated from:
> >     #0 0x7f3da7f31e50 in calloc (/lib64/libasan.so.5+0xeee50)
> >     #1 0x7f3da644441d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5241d)
> >     #2 0x55af9db15440 in qemu_fopen_channel_input /home/elmarco/src/qemu/migration/qemu-file-channel.c:183
> >     #3 0x55af9db15413 in channel_get_output_return_path /home/elmarco/src/qemu/migration/qemu-file-channel.c:159
> >     #4 0x55af9db0d4ac in qemu_file_get_return_path /home/elmarco/src/qemu/migration/qemu-file.c:78
> >     #5 0x55af9dad5e4f in open_return_path_on_source /home/elmarco/src/qemu/migration/migration.c:2295
> >     #6 0x55af9dadb3bf in migrate_fd_connect /home/elmarco/src/qemu/migration/migration.c:3111
> >     #7 0x55af9dae1bf3 in migration_channel_connect /home/elmarco/src/qemu/migration/channel.c:91
> >     #8 0x55af9daddeca in socket_outgoing_migration /home/elmarco/src/qemu/migration/socket.c:108
> >     #9 0x55af9e13d3db in qio_task_complete /home/elmarco/src/qemu/io/task.c:158
> >     #10 0x55af9e13ca03 in qio_task_thread_result /home/elmarco/src/qemu/io/task.c:89
> >     #11 0x7f3da643b1ca in g_idle_dispatch gmain.c:5535
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  migration/migration.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 05d0a7296a..d214ea12c8 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2268,7 +2268,10 @@ out:
> >               */
> >              if (postcopy_pause_return_path_thread(ms)) {
> >                  /* Reload rp, reset the rest */
> > -                rp = ms->rp_state.from_dst_file;
> > +                if (rp != ms->rp_state.from_dst_file) {
> > +                    qemu_fclose(rp);
> > +                    rp = ms->rp_state.from_dst_file;
> > +                }
> >                  ms->rp_state.error = false;
> >                  goto retry;
> >              }
> 
> Yep, I think that's right (cc'ing in Peter);
> 
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Thanks for the report and the fix.  I think the "if" will always be
true, so imho we can even drop that check.  Anyway:

Reviewed-by: Peter Xu <peterx@redhat.com>

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH] migration: fix QEMUFile leak

Posted by Dr. David Alan Gilbert 7 years, 1 month ago

* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> Spotted by ASAN while running:
> 
> $ tests/migration-test -p /x86_64/migration/postcopy/recovery
> 
> =================================================================
> ==18034==ERROR: LeakSanitizer: detected memory leaks
> 
> Direct leak of 33864 byte(s) in 1 object(s) allocated from:
>     #0 0x7f3da7f31e50 in calloc (/lib64/libasan.so.5+0xeee50)
>     #1 0x7f3da644441d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5241d)
>     #2 0x55af9db15440 in qemu_fopen_channel_input /home/elmarco/src/qemu/migration/qemu-file-channel.c:183
>     #3 0x55af9db15413 in channel_get_output_return_path /home/elmarco/src/qemu/migration/qemu-file-channel.c:159
>     #4 0x55af9db0d4ac in qemu_file_get_return_path /home/elmarco/src/qemu/migration/qemu-file.c:78
>     #5 0x55af9dad5e4f in open_return_path_on_source /home/elmarco/src/qemu/migration/migration.c:2295
>     #6 0x55af9dadb3bf in migrate_fd_connect /home/elmarco/src/qemu/migration/migration.c:3111
>     #7 0x55af9dae1bf3 in migration_channel_connect /home/elmarco/src/qemu/migration/channel.c:91
>     #8 0x55af9daddeca in socket_outgoing_migration /home/elmarco/src/qemu/migration/socket.c:108
>     #9 0x55af9e13d3db in qio_task_complete /home/elmarco/src/qemu/io/task.c:158
>     #10 0x55af9e13ca03 in qio_task_thread_result /home/elmarco/src/qemu/io/task.c:89
>     #11 0x7f3da643b1ca in g_idle_dispatch gmain.c:5535
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Queued

> ---
>  migration/migration.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 05d0a7296a..d214ea12c8 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2268,7 +2268,10 @@ out:
>               */
>              if (postcopy_pause_return_path_thread(ms)) {
>                  /* Reload rp, reset the rest */
> -                rp = ms->rp_state.from_dst_file;
> +                if (rp != ms->rp_state.from_dst_file) {
> +                    qemu_fclose(rp);
> +                    rp = ms->rp_state.from_dst_file;
> +                }
>                  ms->rp_state.error = false;
>                  goto retry;
>              }
> -- 
> 2.19.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK