Re: [PATCH v3] migration: free 'saddr' since be no longer used

[Daniel P . Berrangé]

On Mon, Nov 20, 2023 at 11:14:28AM +0800, Zongmin Zhou wrote:
> Since socket_parse() will allocate memory for 'saddr',and its value
> will pass to 'addr' that allocated by migrate_uri_parse(),
> then 'saddr' will no longer used,need to free.
> But due to 'saddr->u' is shallow copying the contents of the union,
> the members of this union containing allocated strings,and will be used after 
> that.
> So just free 'saddr' itself without doing a deep free on the contents of the 
> SocketAddress.
> 
> Fixes: 72a8192e225c ("migration: convert migration 'uri' into 
> 'MigrateAddress'")
> Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
> ---
>  migration/migration.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

and we want this fix in the next -rc release, since the memleak is a regression.

> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 28a34c9068..1832dad618 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -462,7 +462,6 @@ bool migrate_uri_parse(const char *uri, MigrationChannel 
> **channel,
>  {
>      g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1);
>      g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
> -    SocketAddress *saddr = NULL;
>      InetSocketAddress *isock = &addr->u.rdma;
>      strList **tail = &addr->u.exec.args;
>  
> @@ -487,12 +486,14 @@ bool migrate_uri_parse(const char *uri, 
> MigrationChannel **channel,
>                  strstart(uri, "vsock:", NULL) ||
>                  strstart(uri, "fd:", NULL)) {
>          addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET;
> -        saddr = socket_parse(uri, errp);
> +        SocketAddress *saddr = socket_parse(uri, errp);
>          if (!saddr) {
>              return false;
>          }
>          addr->u.socket.type = saddr->type;
>          addr->u.socket.u = saddr->u;
> +        /* Don't free the objects inside; their ownership moved to "addr" */
> +        g_free(saddr);
>      } else if (strstart(uri, "file:", NULL)) {
>          addr->transport = MIGRATION_ADDRESS_TYPE_FILE;
>          addr->u.file.filename = g_strdup(uri + strlen("file:"));
> -- 
> 2.34.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|