| Message ID | 20231129080624.161578-1-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] migration: free 'channel' after its use in migration.c | expand |
I'ld like to suggest a clearer subject: migration: Plug memory leak with migration URIs Het Gala <het.gala@nutanix.com> writes: > 'channel' in qmp_migrate() and qmp_migrate_incoming() is not > auto-freed. migrate_uri_parse() allocates memory to 'channel' if Not sure we need the first sentence. QMP commands never free their arguments. > the user opts for old syntax - uri, which is leaked because there > is no code for freeing 'channel'. > So, free channel to avoid memory leak in case where 'channels' > is empty and uri parsing is required. > Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp > migration flow") > > Signed-off-by: Het Gala <het.gala@nutanix.com> > Suggested-by: Markus Armbruster <armbru@redhat.com> Keep the Fixes: tag on a single line, and next to the other tags: [...] is empty and uri parsing is required. Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow") Signed-off-by: Het Gala <het.gala@nutanix.com> Suggested-by: Markus Armbruster <armbru@redhat.com> > --- > migration/migration.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 28a34c9068..34340f3440 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -515,7 +515,7 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, > MigrationChannelList *channels, > Error **errp) > { > - MigrationChannel *channel = NULL; > + g_autoptr(MigrationChannel) channel = NULL; > MigrationAddress *addr = NULL; > MigrationIncomingState *mis = migration_incoming_get_current(); > > @@ -533,18 +533,18 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, > error_setg(errp, "Channel list has more than one entries"); > return; > } > - channel = channels->value; > + addr = channels->value->addr; > } else if (uri) { > /* caller uses the old URI syntax */ > if (!migrate_uri_parse(uri, &channel, errp)) { > return; > } > + addr = channel->addr; > } else { > error_setg(errp, "neither 'uri' or 'channels' argument are " > "specified in 'migrate-incoming' qmp command "); > return; > } > - addr = channel->addr; > > /* transport mechanism not suitable for migration? */ > if (!migration_channels_and_transport_compatible(addr, errp)) { > @@ -1932,7 +1932,7 @@ void qmp_migrate(const char *uri, bool has_channels, > bool resume_requested; > Error *local_err = NULL; > MigrationState *s = migrate_get_current(); > - MigrationChannel *channel = NULL; > + g_autoptr(MigrationChannel) channel = NULL; > MigrationAddress *addr = NULL; > > /* > @@ -1949,18 +1949,18 @@ void qmp_migrate(const char *uri, bool has_channels, > error_setg(errp, "Channel list has more than one entries"); > return; > } > - channel = channels->value; > + addr = channels->value->addr; > } else if (uri) { > /* caller uses the old URI syntax */ > if (!migrate_uri_parse(uri, &channel, errp)) { > return; > } > + addr = channel->addr; > } else { > error_setg(errp, "neither 'uri' or 'channels' argument are " > "specified in 'migrate' qmp command "); > return; > } > - addr = channel->addr; > > /* transport mechanism not suitable for migration? */ > if (!migration_channels_and_transport_compatible(addr, errp)) { I tested this with an --enable-santizers build. Before the patch: $ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -incoming unix:123 ==3260873==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! QEMU 8.1.92 monitor - type 'help' for more information (qemu) q ================================================================= ==3260873==ERROR: LeakSanitizer: detected memory leaks Direct leak of 40 byte(s) in 1 object(s) allocated from: #0 0x7f0ba08ba097 in calloc (/lib64/libasan.so.8+0xba097) #1 0x7f0b9f4eb5b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) #2 0x55b446454dbe in migrate_uri_parse ../migration/migration.c:490 #3 0x55b4464557c9 in qemu_start_incoming_migration ../migration/migration.c:539 #4 0x55b446461687 in qmp_migrate_incoming ../migration/migration.c:1734 #5 0x55b4463df1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 #6 0x55b4463e4d8e in qemu_init ../system/vl.c:3753 #7 0x55b446f63ca9 in main ../system/main.c:47 #8 0x7f0b9d04a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) Direct leak of 16 byte(s) in 1 object(s) allocated from: #0 0x7f0ba08ba097 in calloc (/lib64/libasan.so.8+0xba097) #1 0x7f0b9f4eb5b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) #2 0x55b4464557c9 in qemu_start_incoming_migration ../migration/migration.c:539 #3 0x55b446461687 in qmp_migrate_incoming ../migration/migration.c:1734 #4 0x55b4463df1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 #5 0x55b4463e4d8e in qemu_init ../system/vl.c:3753 #6 0x55b446f63ca9 in main ../system/main.c:47 #7 0x7f0b9d04a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) Direct leak of 8 byte(s) in 1 object(s) allocated from: #0 0x7f0ba08bb1a8 in operator new(unsigned long) (/lib64/libasan.so.8+0xbb1a8) #1 0x7f0b9a9255b7 in _sub_I_65535_0.0 (/lib64/libtcmalloc_minimal.so.4+0xe5b7) Indirect leak of 48 byte(s) in 1 object(s) allocated from: #0 0x7f0ba08ba097 in calloc (/lib64/libasan.so.8+0xba097) #1 0x7f0b9f4eb5b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) #2 0x55b4464557c9 in qemu_start_incoming_migration ../migration/migration.c:539 #3 0x55b446461687 in qmp_migrate_incoming ../migration/migration.c:1734 #4 0x55b4463df1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 #5 0x55b4463e4d8e in qemu_init ../system/vl.c:3753 #6 0x55b446f63ca9 in main ../system/main.c:47 #7 0x7f0b9d04a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) Indirect leak of 4 byte(s) in 1 object(s) allocated from: #0 0x7f0ba08ba6af in __interceptor_malloc (/lib64/libasan.so.8+0xba6af) #1 0x7f0b9f4eb128 in g_malloc (/lib64/libglib-2.0.so.0+0x5f128) SUMMARY: AddressSanitizer: 116 byte(s) leaked in 5 allocation(s). Afterwards: ==3260526==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! QEMU 8.1.92 monitor - type 'help' for more information (qemu) q ================================================================= ==3260526==ERROR: LeakSanitizer: detected memory leaks Direct leak of 40 byte(s) in 1 object(s) allocated from: #0 0x7f97e54ba097 in calloc (/lib64/libasan.so.8+0xba097) #1 0x7f97e41c75b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) #2 0x55ae31b02dbe in migrate_uri_parse ../migration/migration.c:490 #3 0x55ae31b0382c in qemu_start_incoming_migration ../migration/migration.c:539 #4 0x55ae31b0f724 in qmp_migrate_incoming ../migration/migration.c:1734 #5 0x55ae31a8d1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 #6 0x55ae31a92d8e in qemu_init ../system/vl.c:3753 #7 0x55ae32611de2 in main ../system/main.c:47 #8 0x7f97e1c4a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) Direct leak of 8 byte(s) in 1 object(s) allocated from: #0 0x7f97e54bb1a8 in operator new(unsigned long) (/lib64/libasan.so.8+0xbb1a8) #1 0x7f97df6055b7 in _sub_I_65535_0.0 (/lib64/libtcmalloc_minimal.so.4+0xe5b7) SUMMARY: AddressSanitizer: 48 byte(s) leaked in 2 allocation(s). This confirms the patch succeeds at plugging leaks the -incoming path. It also shows there's one left in migrate_uri_parse(): bool migrate_uri_parse(const char *uri, MigrationChannel **channel, Error **errp) { g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1); g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1); SocketAddress *saddr = NULL; Useless initializer. InetSocketAddress *isock = &addr->u.rdma; strList **tail = &addr->u.exec.args; if (strstart(uri, "exec:", NULL)) { addr->transport = MIGRATION_ADDRESS_TYPE_EXEC; #ifdef WIN32 QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path())); QAPI_LIST_APPEND(tail, g_strdup("/c")); #else QAPI_LIST_APPEND(tail, g_strdup("/bin/sh")); QAPI_LIST_APPEND(tail, g_strdup("-c")); #endif QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:"))); } else if (strstart(uri, "rdma:", NULL)) { if (inet_parse(isock, uri + strlen("rdma:"), errp)) { qapi_free_InetSocketAddress(isock); return false; } addr->transport = MIGRATION_ADDRESS_TYPE_RDMA; } else if (strstart(uri, "tcp:", NULL) || strstart(uri, "unix:", NULL) || strstart(uri, "vsock:", NULL) || strstart(uri, "fd:", NULL)) { Aside: indentation is off. addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET; saddr = socket_parse(uri, errp); @saddr allocated. if (!saddr) { return false; } addr->u.socket.type = saddr->type; addr->u.socket.u = saddr->u; Members of @saddr copied into @addr. } else if (strstart(uri, "file:", NULL)) { addr->transport = MIGRATION_ADDRESS_TYPE_FILE; addr->u.file.filename = g_strdup(uri + strlen("file:")); if (file_parse_offset(addr->u.file.filename, &addr->u.file.offset, errp)) { return false; } } else { error_setg(errp, "unknown migration protocol: %s", uri); return false; } val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN; val->addr = g_steal_pointer(&addr); *channel = g_steal_pointer(&val); @saddr leaked. return true; } Obvious fix: g_free(saddr) right after copying its members. Another fix: make @saddr g_autofree, and keep the initializer. Separate patch. Would you like to take care of it? This one, preferably with the commit message improved: Tested-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Markus Armbruster <armbru@redhat.com> writes: > I'ld like to suggest a clearer subject: > > migration: Plug memory leak with migration URIs > > Het Gala <het.gala@nutanix.com> writes: > >> 'channel' in qmp_migrate() and qmp_migrate_incoming() is not >> auto-freed. migrate_uri_parse() allocates memory to 'channel' if > > Not sure we need the first sentence. QMP commands never free their > arguments. > >> the user opts for old syntax - uri, which is leaked because there >> is no code for freeing 'channel'. >> So, free channel to avoid memory leak in case where 'channels' >> is empty and uri parsing is required. >> Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp >> migration flow") >> >> Signed-off-by: Het Gala <het.gala@nutanix.com> >> Suggested-by: Markus Armbruster <armbru@redhat.com> > > Keep the Fixes: tag on a single line, and next to the other tags: > > [...] > is empty and uri parsing is required. > > Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow") > Signed-off-by: Het Gala <het.gala@nutanix.com> > Suggested-by: Markus Armbruster <armbru@redhat.com> > >> --- >> migration/migration.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index 28a34c9068..34340f3440 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -515,7 +515,7 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, >> MigrationChannelList *channels, >> Error **errp) >> { >> - MigrationChannel *channel = NULL; >> + g_autoptr(MigrationChannel) channel = NULL; >> MigrationAddress *addr = NULL; >> MigrationIncomingState *mis = migration_incoming_get_current(); >> >> @@ -533,18 +533,18 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, >> error_setg(errp, "Channel list has more than one entries"); >> return; >> } >> - channel = channels->value; >> + addr = channels->value->addr; >> } else if (uri) { >> /* caller uses the old URI syntax */ >> if (!migrate_uri_parse(uri, &channel, errp)) { >> return; >> } >> + addr = channel->addr; >> } else { >> error_setg(errp, "neither 'uri' or 'channels' argument are " >> "specified in 'migrate-incoming' qmp command "); >> return; >> } >> - addr = channel->addr; >> >> /* transport mechanism not suitable for migration? */ >> if (!migration_channels_and_transport_compatible(addr, errp)) { >> @@ -1932,7 +1932,7 @@ void qmp_migrate(const char *uri, bool has_channels, >> bool resume_requested; >> Error *local_err = NULL; >> MigrationState *s = migrate_get_current(); >> - MigrationChannel *channel = NULL; >> + g_autoptr(MigrationChannel) channel = NULL; >> MigrationAddress *addr = NULL; >> >> /* >> @@ -1949,18 +1949,18 @@ void qmp_migrate(const char *uri, bool has_channels, >> error_setg(errp, "Channel list has more than one entries"); >> return; >> } >> - channel = channels->value; >> + addr = channels->value->addr; >> } else if (uri) { >> /* caller uses the old URI syntax */ >> if (!migrate_uri_parse(uri, &channel, errp)) { >> return; >> } >> + addr = channel->addr; >> } else { >> error_setg(errp, "neither 'uri' or 'channels' argument are " >> "specified in 'migrate' qmp command "); >> return; >> } >> - addr = channel->addr; >> >> /* transport mechanism not suitable for migration? */ >> if (!migration_channels_and_transport_compatible(addr, errp)) { > > I tested this with an --enable-santizers build. Before the patch: > > $ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -incoming unix:123 > ==3260873==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! > QEMU 8.1.92 monitor - type 'help' for more information > (qemu) q > > ================================================================= > ==3260873==ERROR: LeakSanitizer: detected memory leaks > > Direct leak of 40 byte(s) in 1 object(s) allocated from: > #0 0x7f0ba08ba097 in calloc (/lib64/libasan.so.8+0xba097) > #1 0x7f0b9f4eb5b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) > #2 0x55b446454dbe in migrate_uri_parse ../migration/migration.c:490 > #3 0x55b4464557c9 in qemu_start_incoming_migration ../migration/migration.c:539 > #4 0x55b446461687 in qmp_migrate_incoming ../migration/migration.c:1734 > #5 0x55b4463df1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 > #6 0x55b4463e4d8e in qemu_init ../system/vl.c:3753 > #7 0x55b446f63ca9 in main ../system/main.c:47 > #8 0x7f0b9d04a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) > > Direct leak of 16 byte(s) in 1 object(s) allocated from: > #0 0x7f0ba08ba097 in calloc (/lib64/libasan.so.8+0xba097) > #1 0x7f0b9f4eb5b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) > #2 0x55b4464557c9 in qemu_start_incoming_migration ../migration/migration.c:539 > #3 0x55b446461687 in qmp_migrate_incoming ../migration/migration.c:1734 > #4 0x55b4463df1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 > #5 0x55b4463e4d8e in qemu_init ../system/vl.c:3753 > #6 0x55b446f63ca9 in main ../system/main.c:47 > #7 0x7f0b9d04a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) > > Direct leak of 8 byte(s) in 1 object(s) allocated from: > #0 0x7f0ba08bb1a8 in operator new(unsigned long) (/lib64/libasan.so.8+0xbb1a8) > #1 0x7f0b9a9255b7 in _sub_I_65535_0.0 (/lib64/libtcmalloc_minimal.so.4+0xe5b7) > > Indirect leak of 48 byte(s) in 1 object(s) allocated from: > #0 0x7f0ba08ba097 in calloc (/lib64/libasan.so.8+0xba097) > #1 0x7f0b9f4eb5b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) > #2 0x55b4464557c9 in qemu_start_incoming_migration ../migration/migration.c:539 > #3 0x55b446461687 in qmp_migrate_incoming ../migration/migration.c:1734 > #4 0x55b4463df1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 > #5 0x55b4463e4d8e in qemu_init ../system/vl.c:3753 > #6 0x55b446f63ca9 in main ../system/main.c:47 > #7 0x7f0b9d04a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) > > Indirect leak of 4 byte(s) in 1 object(s) allocated from: > #0 0x7f0ba08ba6af in __interceptor_malloc (/lib64/libasan.so.8+0xba6af) > #1 0x7f0b9f4eb128 in g_malloc (/lib64/libglib-2.0.so.0+0x5f128) > > SUMMARY: AddressSanitizer: 116 byte(s) leaked in 5 allocation(s). > > > Afterwards: > > ==3260526==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! > QEMU 8.1.92 monitor - type 'help' for more information > (qemu) q > > ================================================================= > ==3260526==ERROR: LeakSanitizer: detected memory leaks > > Direct leak of 40 byte(s) in 1 object(s) allocated from: > #0 0x7f97e54ba097 in calloc (/lib64/libasan.so.8+0xba097) > #1 0x7f97e41c75b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) > #2 0x55ae31b02dbe in migrate_uri_parse ../migration/migration.c:490 > #3 0x55ae31b0382c in qemu_start_incoming_migration ../migration/migration.c:539 > #4 0x55ae31b0f724 in qmp_migrate_incoming ../migration/migration.c:1734 > #5 0x55ae31a8d1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 > #6 0x55ae31a92d8e in qemu_init ../system/vl.c:3753 > #7 0x55ae32611de2 in main ../system/main.c:47 > #8 0x7f97e1c4a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) > > Direct leak of 8 byte(s) in 1 object(s) allocated from: > #0 0x7f97e54bb1a8 in operator new(unsigned long) (/lib64/libasan.so.8+0xbb1a8) > #1 0x7f97df6055b7 in _sub_I_65535_0.0 (/lib64/libtcmalloc_minimal.so.4+0xe5b7) > > SUMMARY: AddressSanitizer: 48 byte(s) leaked in 2 allocation(s). > > This confirms the patch succeeds at plugging leaks the -incoming path. > It also shows there's one left in migrate_uri_parse(): > > bool migrate_uri_parse(const char *uri, MigrationChannel **channel, > Error **errp) > { > g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1); > g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1); > SocketAddress *saddr = NULL; > > Useless initializer. > > InetSocketAddress *isock = &addr->u.rdma; > strList **tail = &addr->u.exec.args; > > if (strstart(uri, "exec:", NULL)) { > addr->transport = MIGRATION_ADDRESS_TYPE_EXEC; > #ifdef WIN32 > QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path())); > QAPI_LIST_APPEND(tail, g_strdup("/c")); > #else > QAPI_LIST_APPEND(tail, g_strdup("/bin/sh")); > QAPI_LIST_APPEND(tail, g_strdup("-c")); > #endif > QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:"))); > } else if (strstart(uri, "rdma:", NULL)) { > if (inet_parse(isock, uri + strlen("rdma:"), errp)) { > qapi_free_InetSocketAddress(isock); > return false; > } > addr->transport = MIGRATION_ADDRESS_TYPE_RDMA; > } else if (strstart(uri, "tcp:", NULL) || > strstart(uri, "unix:", NULL) || > strstart(uri, "vsock:", NULL) || > strstart(uri, "fd:", NULL)) { > > Aside: indentation is off. > > addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET; > saddr = socket_parse(uri, errp); > > @saddr allocated. > > if (!saddr) { > return false; > } > addr->u.socket.type = saddr->type; > addr->u.socket.u = saddr->u; > > Members of @saddr copied into @addr. > > } else if (strstart(uri, "file:", NULL)) { > addr->transport = MIGRATION_ADDRESS_TYPE_FILE; > addr->u.file.filename = g_strdup(uri + strlen("file:")); > if (file_parse_offset(addr->u.file.filename, &addr->u.file.offset, > errp)) { > return false; > } > } else { > error_setg(errp, "unknown migration protocol: %s", uri); > return false; > } > > val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN; > val->addr = g_steal_pointer(&addr); > *channel = g_steal_pointer(&val); > > @saddr leaked. > > return true; > } > > Obvious fix: g_free(saddr) right after copying its members. > > Another fix: make @saddr g_autofree, and keep the initializer. > > Separate patch. Would you like to take care of it? That is already being worked by someone else: [PATCH v3] migration: free 'saddr' since be no longer used https://lore.kernel.org/r/20231120031428.908295-1-zhouzongmin@kylinos.cn
On 29/11/23 6:21 pm, Markus Armbruster wrote: > I'ld like to suggest a clearer subject: > > migration: Plug memory leak with migration URIs Ack. Will update the commit message > Het Gala<het.gala@nutanix.com> writes: > >> 'channel' in qmp_migrate() and qmp_migrate_incoming() is not >> auto-freed. migrate_uri_parse() allocates memory to 'channel' if > Not sure we need the first sentence. QMP commands never free their > arguments. Ack. >> the user opts for old syntax - uri, which is leaked because there >> is no code for freeing 'channel'. >> So, free channel to avoid memory leak in case where 'channels' >> is empty and uri parsing is required. >> Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp >> migration flow") >> >> Signed-off-by: Het Gala<het.gala@nutanix.com> >> Suggested-by: Markus Armbruster<armbru@redhat.com> > Keep the Fixes: tag on a single line, and next to the other tags: > > [...] > is empty and uri parsing is required. > > Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow") > Signed-off-by: Het Gala<het.gala@nutanix.com> > Suggested-by: Markus Armbruster<armbru@redhat.com> Ack. [...] >> + addr = channels->value->addr; >> } else if (uri) { >> /* caller uses the old URI syntax */ >> if (!migrate_uri_parse(uri, &channel, errp)) { >> return; >> } >> + addr = channel->addr; >> } else { >> error_setg(errp, "neither 'uri' or 'channels' argument are " >> "specified in 'migrate' qmp command "); >> return; >> } >> - addr = channel->addr; >> >> /* transport mechanism not suitable for migration? */ >> if (!migration_channels_and_transport_compatible(addr, errp)) { > I tested this with an --enable-santizers build. Before the patch: > > $ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -incoming unix:123 > ==3260873==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! > QEMU 8.1.92 monitor - type 'help' for more information > (qemu) q > > ================================================================= > ==3260873==ERROR: LeakSanitizer: detected memory leaks > > Direct leak of 40 byte(s) in 1 object(s) allocated from: > #0 0x7f0ba08ba097 in calloc (/lib64/libasan.so.8+0xba097) > #1 0x7f0b9f4eb5b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) > #2 0x55b446454dbe in migrate_uri_parse ../migration/migration.c:490 > #3 0x55b4464557c9 in qemu_start_incoming_migration ../migration/migration.c:539 > #4 0x55b446461687 in qmp_migrate_incoming ../migration/migration.c:1734 > #5 0x55b4463df1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 > #6 0x55b4463e4d8e in qemu_init ../system/vl.c:3753 > #7 0x55b446f63ca9 in main ../system/main.c:47 > #8 0x7f0b9d04a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) > > Direct leak of 16 byte(s) in 1 object(s) allocated from: > #0 0x7f0ba08ba097 in calloc (/lib64/libasan.so.8+0xba097) > #1 0x7f0b9f4eb5b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) > #2 0x55b4464557c9 in qemu_start_incoming_migration ../migration/migration.c:539 > #3 0x55b446461687 in qmp_migrate_incoming ../migration/migration.c:1734 > #4 0x55b4463df1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 > #5 0x55b4463e4d8e in qemu_init ../system/vl.c:3753 > #6 0x55b446f63ca9 in main ../system/main.c:47 > #7 0x7f0b9d04a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) > > Direct leak of 8 byte(s) in 1 object(s) allocated from: > #0 0x7f0ba08bb1a8 in operator new(unsigned long) (/lib64/libasan.so.8+0xbb1a8) > #1 0x7f0b9a9255b7 in _sub_I_65535_0.0 (/lib64/libtcmalloc_minimal.so.4+0xe5b7) > > Indirect leak of 48 byte(s) in 1 object(s) allocated from: > #0 0x7f0ba08ba097 in calloc (/lib64/libasan.so.8+0xba097) > #1 0x7f0b9f4eb5b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) > #2 0x55b4464557c9 in qemu_start_incoming_migration ../migration/migration.c:539 > #3 0x55b446461687 in qmp_migrate_incoming ../migration/migration.c:1734 > #4 0x55b4463df1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 > #5 0x55b4463e4d8e in qemu_init ../system/vl.c:3753 > #6 0x55b446f63ca9 in main ../system/main.c:47 > #7 0x7f0b9d04a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) > > Indirect leak of 4 byte(s) in 1 object(s) allocated from: > #0 0x7f0ba08ba6af in __interceptor_malloc (/lib64/libasan.so.8+0xba6af) > #1 0x7f0b9f4eb128 in g_malloc (/lib64/libglib-2.0.so.0+0x5f128) > > SUMMARY: AddressSanitizer: 116 byte(s) leaked in 5 allocation(s). curious: how to get this stack trace ? I tried to configure and build qemu with --enable-santizers option, but on running the tests 'make -j test', I see: ==226453==LeakSanitizer has encountered a fatal error. ==226453==HINT: For debugging, try setting environment variable LSAN_OPTIONS=verbosity=1:log_threads=1 ==226453==HINT: LeakSanitizer does not work under ptrace (strace, gdb, etc) > Afterwards: > > ==3260526==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! > QEMU 8.1.92 monitor - type 'help' for more information > (qemu) q > > ================================================================= > ==3260526==ERROR: LeakSanitizer: detected memory leaks > > Direct leak of 40 byte(s) in 1 object(s) allocated from: > #0 0x7f97e54ba097 in calloc (/lib64/libasan.so.8+0xba097) > #1 0x7f97e41c75b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) > #2 0x55ae31b02dbe in migrate_uri_parse ../migration/migration.c:490 > #3 0x55ae31b0382c in qemu_start_incoming_migration ../migration/migration.c:539 > #4 0x55ae31b0f724 in qmp_migrate_incoming ../migration/migration.c:1734 > #5 0x55ae31a8d1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 > #6 0x55ae31a92d8e in qemu_init ../system/vl.c:3753 > #7 0x55ae32611de2 in main ../system/main.c:47 > #8 0x7f97e1c4a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) > > Direct leak of 8 byte(s) in 1 object(s) allocated from: > #0 0x7f97e54bb1a8 in operator new(unsigned long) (/lib64/libasan.so.8+0xbb1a8) > #1 0x7f97df6055b7 in _sub_I_65535_0.0 (/lib64/libtcmalloc_minimal.so.4+0xe5b7) > > SUMMARY: AddressSanitizer: 48 byte(s) leaked in 2 allocation(s). > > This confirms the patch succeeds at plugging leaks the -incoming path. > It also shows there's one left in migrate_uri_parse(): > > bool migrate_uri_parse(const char *uri, MigrationChannel **channel, [...] > Aside: indentation is off. > > addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET; > saddr = socket_parse(uri, errp); > > @saddr allocated. > > if (!saddr) { > return false; > } > addr->u.socket.type = saddr->type; > addr->u.socket.u = saddr->u; > > Members of @saddr copied into @addr. > > } else if (strstart(uri, "file:", NULL)) { > addr->transport = MIGRATION_ADDRESS_TYPE_FILE; > addr->u.file.filename = g_strdup(uri + strlen("file:")); > if (file_parse_offset(addr->u.file.filename, &addr->u.file.offset, > errp)) { > return false; > } > } else { > error_setg(errp, "unknown migration protocol: %s", uri); > return false; > } > > val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN; > val->addr = g_steal_pointer(&addr); > *channel = g_steal_pointer(&val); > > @saddr leaked. > > return true; > } > > Obvious fix: g_free(saddr) right after copying its members. > > Another fix: make @saddr g_autofree, and keep the initializer. > > Separate patch. Would you like to take care of it? > > This one, preferably with the commit message improved: > Tested-by: Markus Armbruster<armbru@redhat.com> > Reviewed-by: Markus Armbruster<armbru@redhat.com> Fabiano has already answered to your query. Regards, Het Gala
Het Gala <het.gala@nutanix.com> writes: > On 29/11/23 6:21 pm, Markus Armbruster wrote: >> I'ld like to suggest a clearer subject: >> >> migration: Plug memory leak with migration URIs > Ack. Will update the commit message >> Het Gala<het.gala@nutanix.com> writes: >> >>> 'channel' in qmp_migrate() and qmp_migrate_incoming() is not >>> auto-freed. migrate_uri_parse() allocates memory to 'channel' if >> >> Not sure we need the first sentence. QMP commands never free their >> arguments. > > Ack. > >>> the user opts for old syntax - uri, which is leaked because there >>> is no code for freeing 'channel'. >>> So, free channel to avoid memory leak in case where 'channels' >>> is empty and uri parsing is required. >>> Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp >>> migration flow") >>> >>> Signed-off-by: Het Gala<het.gala@nutanix.com> >>> Suggested-by: Markus Armbruster<armbru@redhat.com> >> >> Keep the Fixes: tag on a single line, and next to the other tags: >> >> [...] >> is empty and uri parsing is required. >> >> Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow") >> Signed-off-by: Het Gala<het.gala@nutanix.com> >> Suggested-by: Markus Armbruster<armbru@redhat.com> > > Ack. > > [...] > >>> + addr = channels->value->addr; >>> } else if (uri) { >>> /* caller uses the old URI syntax */ >>> if (!migrate_uri_parse(uri, &channel, errp)) { >>> return; >>> } >>> + addr = channel->addr; >>> } else { >>> error_setg(errp, "neither 'uri' or 'channels' argument are " >>> "specified in 'migrate' qmp command "); >>> return; >>> } >>> - addr = channel->addr; >>> /* transport mechanism not suitable for migration? */ >>> if (!migration_channels_and_transport_compatible(addr, errp)) { >> >> I tested this with an --enable-santizers build. Before the patch: >> >> $ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -incoming unix:123 >> ==3260873==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! >> QEMU 8.1.92 monitor - type 'help' for more information >> (qemu) q >> >> ================================================================= >> ==3260873==ERROR: LeakSanitizer: detected memory leaks >> >> Direct leak of 40 byte(s) in 1 object(s) allocated from: >> #0 0x7f0ba08ba097 in calloc (/lib64/libasan.so.8+0xba097) >> #1 0x7f0b9f4eb5b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) >> #2 0x55b446454dbe in migrate_uri_parse ../migration/migration.c:490 >> #3 0x55b4464557c9 in qemu_start_incoming_migration ../migration/migration.c:539 >> #4 0x55b446461687 in qmp_migrate_incoming ../migration/migration.c:1734 >> #5 0x55b4463df1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 >> #6 0x55b4463e4d8e in qemu_init ../system/vl.c:3753 >> #7 0x55b446f63ca9 in main ../system/main.c:47 >> #8 0x7f0b9d04a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) >> >> Direct leak of 16 byte(s) in 1 object(s) allocated from: >> #0 0x7f0ba08ba097 in calloc (/lib64/libasan.so.8+0xba097) >> #1 0x7f0b9f4eb5b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) >> #2 0x55b4464557c9 in qemu_start_incoming_migration ../migration/migration.c:539 >> #3 0x55b446461687 in qmp_migrate_incoming ../migration/migration.c:1734 >> #4 0x55b4463df1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 >> #5 0x55b4463e4d8e in qemu_init ../system/vl.c:3753 >> #6 0x55b446f63ca9 in main ../system/main.c:47 >> #7 0x7f0b9d04a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) >> >> Direct leak of 8 byte(s) in 1 object(s) allocated from: >> #0 0x7f0ba08bb1a8 in operator new(unsigned long) (/lib64/libasan.so.8+0xbb1a8) >> #1 0x7f0b9a9255b7 in _sub_I_65535_0.0 (/lib64/libtcmalloc_minimal.so.4+0xe5b7) >> >> Indirect leak of 48 byte(s) in 1 object(s) allocated from: >> #0 0x7f0ba08ba097 in calloc (/lib64/libasan.so.8+0xba097) >> #1 0x7f0b9f4eb5b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5f5b0) >> #2 0x55b4464557c9 in qemu_start_incoming_migration ../migration/migration.c:539 >> #3 0x55b446461687 in qmp_migrate_incoming ../migration/migration.c:1734 >> #4 0x55b4463df1c2 in qmp_x_exit_preconfig ../system/vl.c:2718 >> #5 0x55b4463e4d8e in qemu_init ../system/vl.c:3753 >> #6 0x55b446f63ca9 in main ../system/main.c:47 >> #7 0x7f0b9d04a54f in __libc_start_call_main (/lib64/libc.so.6+0x2754f) >> >> Indirect leak of 4 byte(s) in 1 object(s) allocated from: >> #0 0x7f0ba08ba6af in __interceptor_malloc (/lib64/libasan.so.8+0xba6af) >> #1 0x7f0b9f4eb128 in g_malloc (/lib64/libglib-2.0.so.0+0x5f128) >> >> SUMMARY: AddressSanitizer: 116 byte(s) leaked in 5 allocation(s). > > curious: how to get this stack trace ? I tried to configure and build qemu with --enable-santizers option, but on running the tests 'make -j test', I see: > > ==226453==LeakSanitizer has encountered a fatal error. ==226453==HINT: For debugging, try setting environment variable LSAN_OPTIONS=verbosity=1:log_threads=1 ==226453==HINT: LeakSanitizer does not work under ptrace (strace, gdb, etc) I built with $ ../configure --disable-werror --target-list=x86_64-softmmu --enable-debug --enable-sanitizers $ make then ran the manual test shown above. "make check" fails differently for me than it does for you. [...]
diff --git a/migration/migration.c b/migration/migration.c index 28a34c9068..34340f3440 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -515,7 +515,7 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, MigrationChannelList *channels, Error **errp) { - MigrationChannel *channel = NULL; + g_autoptr(MigrationChannel) channel = NULL; MigrationAddress *addr = NULL; MigrationIncomingState *mis = migration_incoming_get_current(); @@ -533,18 +533,18 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, error_setg(errp, "Channel list has more than one entries"); return; } - channel = channels->value; + addr = channels->value->addr; } else if (uri) { /* caller uses the old URI syntax */ if (!migrate_uri_parse(uri, &channel, errp)) { return; } + addr = channel->addr; } else { error_setg(errp, "neither 'uri' or 'channels' argument are " "specified in 'migrate-incoming' qmp command "); return; } - addr = channel->addr; /* transport mechanism not suitable for migration? */ if (!migration_channels_and_transport_compatible(addr, errp)) { @@ -1932,7 +1932,7 @@ void qmp_migrate(const char *uri, bool has_channels, bool resume_requested; Error *local_err = NULL; MigrationState *s = migrate_get_current(); - MigrationChannel *channel = NULL; + g_autoptr(MigrationChannel) channel = NULL; MigrationAddress *addr = NULL; /* @@ -1949,18 +1949,18 @@ void qmp_migrate(const char *uri, bool has_channels, error_setg(errp, "Channel list has more than one entries"); return; } - channel = channels->value; + addr = channels->value->addr; } else if (uri) { /* caller uses the old URI syntax */ if (!migrate_uri_parse(uri, &channel, errp)) { return; } + addr = channel->addr; } else { error_setg(errp, "neither 'uri' or 'channels' argument are " "specified in 'migrate' qmp command "); return; } - addr = channel->addr; /* transport mechanism not suitable for migration? */ if (!migration_channels_and_transport_compatible(addr, errp)) {
'channel' in qmp_migrate() and qmp_migrate_incoming() is not auto-freed. migrate_uri_parse() allocates memory to 'channel' if the user opts for old syntax - uri, which is leaked because there is no code for freeing 'channel'. So, free channel to avoid memory leak in case where 'channels' is empty and uri parsing is required. Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow") Signed-off-by: Het Gala <het.gala@nutanix.com> Suggested-by: Markus Armbruster <armbru@redhat.com> --- migration/migration.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)