migration/migration.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
'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(-)
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)) {
--
2.22.3I'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>
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.
[...]
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
© 2016 - 2024 Red Hat, Inc.