| Message ID | 20240820144429.320176-1-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [for-9.1?] migration/multifd: Free MultiFDRecvParams::data | expand |
Peter Maydell <peter.maydell@linaro.org> writes: > In multifd_recv_setup() we allocate (among other things) > * a MultiFDRecvData struct to multifd_recv_state::data > * a MultiFDRecvData struct to each multfd_recv_state->params[i].data > > (Then during execution we might swap these pointers around.) > > But in multifd_recv_cleanup() we free multifd_recv_state->data > in multifd_recv_cleanup_state() but we don't ever free the > multifd_recv_state->params[i].data. This results in a memory > leak reported by LeakSanitizer: > > (cd build/asan && \ > ASAN_OPTIONS="fast_unwind_on_malloc=0:strip_path_prefix=/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/../../" \ > QTEST_QEMU_BINARY=./qemu-system-x86_64 \ > ./tests/qtest/migration-test --tap -k -p /x86_64/migration/multifd/file/mapped-ram ) > [...] > Direct leak of 72 byte(s) in 3 object(s) allocated from: > #0 0x561cc0afcfd8 in __interceptor_calloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/qemu-system-x86_64+0x218efd8) (BuildId: be72e086d4e47b172b0a72779972213fd9916466) > #1 0x7f89d37acc50 in g_malloc0 debian/build/deb/../../../glib/gmem.c:161:13 > #2 0x561cc1e9c83c in multifd_recv_setup migration/multifd.c:1606:19 > #3 0x561cc1e68618 in migration_ioc_process_incoming migration/migration.c:972:9 > #4 0x561cc1e3ac59 in migration_channel_process_incoming migration/channel.c:45:9 > #5 0x561cc1e4fa0b in file_accept_incoming_migration migration/file.c:132:5 > #6 0x561cc30f2c0c in qio_channel_fd_source_dispatch io/channel-watch.c:84:12 > #7 0x7f89d37a3c43 in g_main_dispatch debian/build/deb/../../../glib/gmain.c:3419:28 > #8 0x7f89d37a3c43 in g_main_context_dispatch debian/build/deb/../../../glib/gmain.c:4137:7 > #9 0x561cc3b21659 in glib_pollfds_poll util/main-loop.c:287:9 > #10 0x561cc3b1ff93 in os_host_main_loop_wait util/main-loop.c:310:5 > #11 0x561cc3b1fb5c in main_loop_wait util/main-loop.c:589:11 > #12 0x561cc1da2917 in qemu_main_loop system/runstate.c:801:9 > #13 0x561cc3796c1c in qemu_default_main system/main.c:37:14 > #14 0x561cc3796c67 in main system/main.c:48:12 > #15 0x7f89d163bd8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 > #16 0x7f89d163be3f in __libc_start_main csu/../csu/libc-start.c:392:3 > #17 0x561cc0a79fa4 in _start (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/qemu-system-x86_64+0x210bfa4) (BuildId: be72e086d4e47b172b0a72779972213fd9916466) > > Direct leak of 24 byte(s) in 1 object(s) allocated from: > #0 0x561cc0afcfd8 in __interceptor_calloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/qemu-system-x86_64+0x218efd8) (BuildId: be72e086d4e47b172b0a72779972213fd9916466) > #1 0x7f89d37acc50 in g_malloc0 debian/build/deb/../../../glib/gmem.c:161:13 > #2 0x561cc1e9bed9 in multifd_recv_setup migration/multifd.c:1588:32 > #3 0x561cc1e68618 in migration_ioc_process_incoming migration/migration.c:972:9 > #4 0x561cc1e3ac59 in migration_channel_process_incoming migration/channel.c:45:9 > #5 0x561cc1e4fa0b in file_accept_incoming_migration migration/file.c:132:5 > #6 0x561cc30f2c0c in qio_channel_fd_source_dispatch io/channel-watch.c:84:12 > #7 0x7f89d37a3c43 in g_main_dispatch debian/build/deb/../../../glib/gmain.c:3419:28 > #8 0x7f89d37a3c43 in g_main_context_dispatch debian/build/deb/../../../glib/gmain.c:4137:7 > #9 0x561cc3b21659 in glib_pollfds_poll util/main-loop.c:287:9 > #10 0x561cc3b1ff93 in os_host_main_loop_wait util/main-loop.c:310:5 > #11 0x561cc3b1fb5c in main_loop_wait util/main-loop.c:589:11 > #12 0x561cc1da2917 in qemu_main_loop system/runstate.c:801:9 > #13 0x561cc3796c1c in qemu_default_main system/main.c:37:14 > #14 0x561cc3796c67 in main system/main.c:48:12 > #15 0x7f89d163bd8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 > #16 0x7f89d163be3f in __libc_start_main csu/../csu/libc-start.c:392:3 > #17 0x561cc0a79fa4 in _start (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/qemu-system-x86_64+0x210bfa4) (BuildId: be72e086d4e47b172b0a72779972213fd9916466) > > SUMMARY: AddressSanitizer: 96 byte(s) leaked in 4 allocation(s). > > Free the params[i].data too. > > Cc: qemu-stable@nongnu.org > Fixes: d117ed0699d41 ("migration/multifd: Allow receiving pages without packets") > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This bug was in the 9.0 release, so not a regression we absolutely > must fix for 9.1. I have also only tested this by running the > migration-test test suite. > > NB that the tests themselves have a pile of leaks; I'm about to send > a separate patchseries for those. > > migration/multifd.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/migration/multifd.c b/migration/multifd.c > index 552f9723c82..a6db05502aa 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -1357,6 +1357,8 @@ static void multifd_recv_cleanup_channel(MultiFDRecvParams *p) > qemu_mutex_destroy(&p->mutex); > qemu_sem_destroy(&p->sem_sync); > qemu_sem_destroy(&p->sem); > + g_free(p->data); > + p->data = NULL; > g_free(p->name); > p->name = NULL; > p->packet_len = 0; Thanks, Peter. Looks like I'm not helping myself: if [ $valgrind -eq 1 ]; then trap "rm ${BASEDIR}/valgrind-suppressions" EXIT run_valgrind "src" 1>&3 | grep "== " # run_valgrind "dst" 1>&3 | grep "== " fi Reviewed-by: Fabiano Rosas <farosas@suse.de>
diff --git a/migration/multifd.c b/migration/multifd.c index 552f9723c82..a6db05502aa 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -1357,6 +1357,8 @@ static void multifd_recv_cleanup_channel(MultiFDRecvParams *p) qemu_mutex_destroy(&p->mutex); qemu_sem_destroy(&p->sem_sync); qemu_sem_destroy(&p->sem); + g_free(p->data); + p->data = NULL; g_free(p->name); p->name = NULL; p->packet_len = 0;
In multifd_recv_setup() we allocate (among other things) * a MultiFDRecvData struct to multifd_recv_state::data * a MultiFDRecvData struct to each multfd_recv_state->params[i].data (Then during execution we might swap these pointers around.) But in multifd_recv_cleanup() we free multifd_recv_state->data in multifd_recv_cleanup_state() but we don't ever free the multifd_recv_state->params[i].data. This results in a memory leak reported by LeakSanitizer: (cd build/asan && \ ASAN_OPTIONS="fast_unwind_on_malloc=0:strip_path_prefix=/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/../../" \ QTEST_QEMU_BINARY=./qemu-system-x86_64 \ ./tests/qtest/migration-test --tap -k -p /x86_64/migration/multifd/file/mapped-ram ) [...] Direct leak of 72 byte(s) in 3 object(s) allocated from: #0 0x561cc0afcfd8 in __interceptor_calloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/qemu-system-x86_64+0x218efd8) (BuildId: be72e086d4e47b172b0a72779972213fd9916466) #1 0x7f89d37acc50 in g_malloc0 debian/build/deb/../../../glib/gmem.c:161:13 #2 0x561cc1e9c83c in multifd_recv_setup migration/multifd.c:1606:19 #3 0x561cc1e68618 in migration_ioc_process_incoming migration/migration.c:972:9 #4 0x561cc1e3ac59 in migration_channel_process_incoming migration/channel.c:45:9 #5 0x561cc1e4fa0b in file_accept_incoming_migration migration/file.c:132:5 #6 0x561cc30f2c0c in qio_channel_fd_source_dispatch io/channel-watch.c:84:12 #7 0x7f89d37a3c43 in g_main_dispatch debian/build/deb/../../../glib/gmain.c:3419:28 #8 0x7f89d37a3c43 in g_main_context_dispatch debian/build/deb/../../../glib/gmain.c:4137:7 #9 0x561cc3b21659 in glib_pollfds_poll util/main-loop.c:287:9 #10 0x561cc3b1ff93 in os_host_main_loop_wait util/main-loop.c:310:5 #11 0x561cc3b1fb5c in main_loop_wait util/main-loop.c:589:11 #12 0x561cc1da2917 in qemu_main_loop system/runstate.c:801:9 #13 0x561cc3796c1c in qemu_default_main system/main.c:37:14 #14 0x561cc3796c67 in main system/main.c:48:12 #15 0x7f89d163bd8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 #16 0x7f89d163be3f in __libc_start_main csu/../csu/libc-start.c:392:3 #17 0x561cc0a79fa4 in _start (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/qemu-system-x86_64+0x210bfa4) (BuildId: be72e086d4e47b172b0a72779972213fd9916466) Direct leak of 24 byte(s) in 1 object(s) allocated from: #0 0x561cc0afcfd8 in __interceptor_calloc (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/qemu-system-x86_64+0x218efd8) (BuildId: be72e086d4e47b172b0a72779972213fd9916466) #1 0x7f89d37acc50 in g_malloc0 debian/build/deb/../../../glib/gmem.c:161:13 #2 0x561cc1e9bed9 in multifd_recv_setup migration/multifd.c:1588:32 #3 0x561cc1e68618 in migration_ioc_process_incoming migration/migration.c:972:9 #4 0x561cc1e3ac59 in migration_channel_process_incoming migration/channel.c:45:9 #5 0x561cc1e4fa0b in file_accept_incoming_migration migration/file.c:132:5 #6 0x561cc30f2c0c in qio_channel_fd_source_dispatch io/channel-watch.c:84:12 #7 0x7f89d37a3c43 in g_main_dispatch debian/build/deb/../../../glib/gmain.c:3419:28 #8 0x7f89d37a3c43 in g_main_context_dispatch debian/build/deb/../../../glib/gmain.c:4137:7 #9 0x561cc3b21659 in glib_pollfds_poll util/main-loop.c:287:9 #10 0x561cc3b1ff93 in os_host_main_loop_wait util/main-loop.c:310:5 #11 0x561cc3b1fb5c in main_loop_wait util/main-loop.c:589:11 #12 0x561cc1da2917 in qemu_main_loop system/runstate.c:801:9 #13 0x561cc3796c1c in qemu_default_main system/main.c:37:14 #14 0x561cc3796c67 in main system/main.c:48:12 #15 0x7f89d163bd8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 #16 0x7f89d163be3f in __libc_start_main csu/../csu/libc-start.c:392:3 #17 0x561cc0a79fa4 in _start (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/asan/qemu-system-x86_64+0x210bfa4) (BuildId: be72e086d4e47b172b0a72779972213fd9916466) SUMMARY: AddressSanitizer: 96 byte(s) leaked in 4 allocation(s). Free the params[i].data too. Cc: qemu-stable@nongnu.org Fixes: d117ed0699d41 ("migration/multifd: Allow receiving pages without packets") Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- This bug was in the 9.0 release, so not a regression we absolutely must fix for 9.1. I have also only tested this by running the migration-test test suite. NB that the tests themselves have a pile of leaks; I'm about to send a separate patchseries for those. migration/multifd.c | 2 ++ 1 file changed, 2 insertions(+)