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

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

[Zongmin Zhou]

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;
+        qapi_free_SocketAddress(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

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

[Daniel P. Berrangé]

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


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 :|

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

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

[Zongmin Zhou]

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/migration.c b/migration/migration.c
index 28a34c9068..9bdbcdaf49 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;
+        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

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

[Juan Quintela]

Zongmin Zhou <zhouzongmin@kylinos.cn> 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 | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 28a34c9068..9bdbcdaf49 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;
> +        g_free(saddr);
>      } else if (strstart(uri, "file:", NULL)) {
>          addr->transport = MIGRATION_ADDRESS_TYPE_FILE;
>          addr->u.file.filename = g_strdup(uri + strlen("file:"));

Once that we are here, can we move the declaration of saddr to this
block, so we are sure that we don't use saddr anywhere?

As Peter said, putting a comment why we don't use
qapi_free_SocketAddress() will be a good idea.

Later, Juan.

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

[Zongmin Zhou]

On 2023/11/16 22:19, Juan Quintela wrote:
> Zongmin Zhou <zhouzongmin@kylinos.cn> 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 | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 28a34c9068..9bdbcdaf49 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;
>> +        g_free(saddr);
>>       } else if (strstart(uri, "file:", NULL)) {
>>           addr->transport = MIGRATION_ADDRESS_TYPE_FILE;
>>           addr->u.file.filename = g_strdup(uri + strlen("file:"));
> Once that we are here, can we move the declaration of saddr to this
> block, so we are sure that we don't use saddr anywhere?
do you mean to do the changes below at this block?
SocketAddress *saddr = socket_parse(uri, errp);
That sounds good and make it clear that 'saddr' is only used on this block.
> As Peter said, putting a comment why we don't use
> qapi_free_SocketAddress() will be a good idea.

I have put some comments on patch v2 to explain

why just free 'saddr' itself without doing a deep free on the contents 
of the SocketAddress .

Maybe need to explicit clarify why g_free is used instead of 
qapi_free_SocketAddress()?


Best regards!

>
> Later, Juan.
>

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

[Peter Xu]

On Fri, Nov 17, 2023 at 10:51:18AM +0800, Zongmin Zhou wrote:
> > As Peter said, putting a comment why we don't use
> > qapi_free_SocketAddress() will be a good idea.
> 
> I have put some comments on patch v2 to explain

Normally we use "comment" to represent direct comment in the code.  You
explained it in the "commit message". :)

That explanation is good enough to me, you can add a summary comment in the
code too.  Something like:

  /* Don't free the objects inside; their ownership moved to "addr" */

-- 
Peter Xu

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

[Zongmin Zhou]

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(-)

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

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 :|

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

[Peter Xu]

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>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

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

[Zongmin Zhou]

Ping? Has this been forgotten?

Best regards!

On 2023/11/20 22:01, Peter Xu wrote:
> 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>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>

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

[Peter Xu]

On Wed, Nov 29, 2023 at 10:09:12AM +0800, Zongmin Zhou wrote:
> Ping? Has this been forgotten?

Juan,

Would you send a pull to include this?

Thanks!

-- 
Peter Xu

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

[Het Gala]

On 29/11/23 8:17 pm, Peter Xu wrote:

> On Wed, Nov 29, 2023 at 10:09:12AM +0800, Zongmin Zhou wrote:
>> Ping? Has this been forgotten?
> Juan,
>
> Would you send a pull to include this?
>
> Thanks!
>
Peter, Juan - Could you also pull a similar patch related to memory leak 
together, it has been tested and reviewed by Markus already. ref - 
https://lists.gnu.org/archive/html/qemu-devel/2023-11/msg05948.htm. TIA !

Regards,
Het Gala