Re: [PATCH v3] migration: free 'channel' after its use in migration.c

[Markus Armbruster]

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.

[...]