Re: [PATCH] 'channel' and 'addr' in qmp_migrate() are not auto-freed. migrate_uri_parse() allocates memory which is returned to 'channel', which is leaked because there is no code for freeing 'channel' or 'addr'. So, free addr and channel to avoid memory leak. 'addr' does shallow copying of channel->addr, hence free 'channel' itself and deep free contents of 'addr'

From: Markus Armbruster <armbru@redhat.com>
To: Het Gala <het.gala@nutanix.com>
Cc: qemu-devel@nongnu.org,  prerna.saxena@nutanix.com,
	 quintela@redhat.com, berrange@redhat.com,
	 peter.maydell@linaro.org,  farosas@suse.de
Subject: Re: [PATCH] 'channel' and 'addr' in qmp_migrate() are not auto-freed. migrate_uri_parse() allocates memory which is returned to 'channel', which is leaked because there is no code for freeing 'channel' or 'addr'. So, free addr and channel to avoid memory leak. 'addr' does shallow copying of channel->addr, hence free 'channel' itself and deep free contents of 'addr'
Date: Tue, 28 Nov 2023 08:16:55 +0100	[thread overview]
Message-ID: <87a5qy4aag.fsf@pond.sub.org> (raw)
In-Reply-To: <20231128062520.36456-1-het.gala@nutanix.com> (Het Gala's message of "Tue, 28 Nov 2023 06:25:20 +0000")

Your commit message is all in one line.  You need to format it like

     migration: Plug memory leak

    'channel' and 'addr' in qmp_migrate() are not auto-freed.
    migrate_uri_parse() allocates memory which is returned to 'channel',
    which is leaked because there is no code for freeing 'channel' or
    'addr'.  So, free addr and channel to avoid memory leak.  'addr'
    does shallow copying of channel->addr, hence free 'channel' itself
    and deep free contents of 'addr'.

Het Gala <het.gala@nutanix.com> writes:

> Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp
> migration flow")
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  migration/migration.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 28a34c9068..29efb51b62 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2004,6 +2004,8 @@ void qmp_migrate(const char *uri, bool has_channels,
>                            MIGRATION_STATUS_FAILED);
>          block_cleanup_parameters();
>      }
> +    g_free(channel);
> +    qapi_free_MigrationAddress(addr);
>  
>      if (local_err) {
>          if (!resume_requested) {

What makes you think there's a memory leak?  I can't see it.

qmp_migrate() has two callers:

1. qmp_marshal_migrate(), in generated qapi-commands-migration.c

   qmp_marshal_migrate() deserializes @args into @arg with a visitor.
   This allocates @arg and its members, including arg.channels.  It
   passes all the members of @arg to qmp_migrate().  And then it frees
   @arg and its members including arg.channels.

2. hmp_migrate()

   hmp_migrate() allocates @channel with migrate_uri_parse(), adds it to
   list @caps, passes @caps to qmp_migrate(), then frees @caps with
   qapi_free_MigrationChannelList().