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

[Peter Xu]

On Wed, Nov 15, 2023 at 09:49:09AM +0000, Daniel P. Berrangé wrote:
> On Wed, Nov 15, 2023 at 11:27:39AM +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(),so free 'saddr' to avoid memory leak.
> > 
> > Fixes: 72a8192e225c ("migration: convert migration 'uri' into 
> > 'MigrateAddress'")
> > Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
> > ---
> >  migration/migration.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 28a34c9068..30ed4bf6b6 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -493,6 +493,7 @@ bool migrate_uri_parse(const char *uri, 
> > MigrationChannel **channel,
> >          }
> >          addr->u.socket.type = saddr->type;
> >          addr->u.socket.u = saddr->u;
> 
> 'saddr->u' is a union embedded in SocketAddress, containing:
> 
>     union { /* union tag is @type */
>         InetSocketAddressWrapper inet;
>         UnixSocketAddressWrapper q_unix;
>         VsockSocketAddressWrapper vsock;
>         StringWrapper fd;
>     } u;
> 
> THis assignment is *shallow* copying the contents of the union.
> 
> All the type specifics structs that are members of this union
> containing allocated strings, and with this shallow copy, we
> are stealing the pointers to these allocated strings
> 
> 
> > +        qapi_free_SocketAddress(saddr);
> 
> This meanwhle is doing a *deep* free of the contents of the
> SocketAddress, which includes all the pointers we just stole.
> 
> IOW, unless I'm mistaken somehow, this is going to cause a
> double-free

Right.  I think what we need is a g_free(saddr), with a comment explaining?

Or, is there better way to do that?  Something like a QAPI_CLONE() but not
exactly: we already have the object allocated.  We want to deep copy it to
the current object only on the fields but not the object itself.

-- 
Peter Xu