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

[Het Gala]

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