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

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

Posted by Zongmin Zhou 1 year ago

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

Posted by Juan Quintela 1 year ago

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

Posted by Zongmin Zhou 1 year ago

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

Posted by Peter Xu 1 year ago

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

Posted by Zongmin Zhou 1 year ago

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