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) {
-- 
2.22.3

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

On 28/11/23 12:46 pm, Markus Armbruster wrote:
> 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:
Yeah, I made the changes in v2 patchset.
>> ---
>>   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) {
> 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().

Markus, sorry if I was not able to put point clearly, what I meant is 
that the local 'channel' variable used in qmp_migrate() i.e.

'MigrationChannel *channel = NULL', is defined in qmp_migrate() and if 
the user opts for 'uri' then '@channels' coming from hmp_migrate() will 
be NULL, and then migrate_uri_parse() will populate memory into 
'channel', and that is not getting freed after it's use is over.

I think, that is where memory leak might be happening ?


Regards,

Het Gala

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

> On 28/11/23 12:46 pm, Markus Armbruster wrote:
>> 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:
> Yeah, I made the changes in v2 patchset.
>>> ---
>>>   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) {
>> 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().
>
> Markus, sorry if I was not able to put point clearly, what I meant is that the local 'channel' variable used in qmp_migrate() i.e.
>
> 'MigrationChannel *channel = NULL', is defined in qmp_migrate() and if the user opts for 'uri' then '@channels' coming from hmp_migrate() will be NULL, and then migrate_uri_parse() will populate memory into 'channel', and that is not getting freed after it's use is over.
>
> I think, that is where memory leak might be happening ?

Aha!

    if (uri && has_channels) {
        error_setg(errp, "'uri' and 'channels' arguments are mutually "
                   "exclusive; exactly one of the two should be present in "
                   "'migrate' qmp command ");
        return;
    } else if (channels) {
        /* To verify that Migrate channel list has only item */
        if (channels->next) {
            error_setg(errp, "Channel list has more than one entries");
            return;
        }
        channel = channels->value;
    } else if (uri) {
        /* caller uses the old URI syntax */
        if (!migrate_uri_parse(uri, &channel, errp)) {
            return;
        }
    } else {
        error_setg(errp, "neither 'uri' or 'channels' argument are "
                   "specified in 'migrate' qmp command ");
        return;
    }

At this point, @channel is either channels->value, or from
migrate_uri_parse().

We must not free in the former case, we must free in the latter case,

Before your patch, we don't free.  Memory leak in the latter case.

Afterwards, we free.  Double-free in the former case.

You could guard the free, like so:

    if (uri) {
        qapi_free_MigrationChannel(channel);
    }


By the way, I the conditional shown above is harder to understand than
necessary.  I like to get the errors out of the way at the beginning,
like this:

    if (uri && has_channels) {
        error_setg(errp, "'uri' and 'channels' arguments are mutually "
                   "exclusive; exactly one of the two should be present in "
                   "'migrate' qmp command ");
        return;
    }
    if (!uri && !has_channels) {
        error_setg(errp, "neither 'uri' or 'channels' argument are "
                   "specified in 'migrate' qmp command ");
        return;
    }

    if (channels) {
        /* To verify that Migrate channel list has only item */

Or even

    if (!uri == !has_channels) {
        error_setg(errp, "need either 'uri' or 'channels' argument")
        return;
    }

Suggestion, not demand.  If you do it, separate patch.

On 28/11/23 3:29 pm, Markus Armbruster wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> On 28/11/23 12:46 pm, Markus Armbruster wrote:
>>> 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:
>> Yeah, I made the changes in v2 patchset.
>>>> ---
>>>>    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) {
>>> 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().
>> Markus, sorry if I was not able to put point clearly, what I meant is that the local 'channel' variable used in qmp_migrate() i.e.
>>
>> 'MigrationChannel *channel = NULL', is defined in qmp_migrate() and if the user opts for 'uri' then '@channels' coming from hmp_migrate() will be NULL, and then migrate_uri_parse() will populate memory into 'channel', and that is not getting freed after it's use is over.
>>
>> I think, that is where memory leak might be happening ?
> Aha!
>
>      if (uri && has_channels) {
>          error_setg(errp, "'uri' and 'channels' arguments are mutually "
>                     "exclusive; exactly one of the two should be present in "
>                     "'migrate' qmp command ");
>          return;
>      } else if (channels) {
>          /* To verify that Migrate channel list has only item */
>          if (channels->next) {
>              error_setg(errp, "Channel list has more than one entries");
>              return;
>          }
>          channel = channels->value;
>      } else if (uri) {
>          /* caller uses the old URI syntax */
>          if (!migrate_uri_parse(uri, &channel, errp)) {
>              return;
>          }
>      } else {
>          error_setg(errp, "neither 'uri' or 'channels' argument are "
>                     "specified in 'migrate' qmp command ");
>          return;
>      }
>
> At this point, @channel is either channels->value, or from
> migrate_uri_parse().
>
> We must not free in the former case, we must free in the latter case,
>
> Before your patch, we don't free.  Memory leak in the latter case.
>
> Afterwards, we free.  Double-free in the former case.
>
> You could guard the free, like so:
>
>      if (uri) {
>          qapi_free_MigrationChannel(channel);
>      }

Yeah, you explained it right. The above solution seems fine to me.

I am just curious to ask this: can we use g_autoptr() for local vaiable 
'channel' and 'addr' ? As we are not passing these variables to the 
caller function, nor we are trying to transfer their ownership to 
another variable, so use of g_steal_pointer() also might not be required ?

>
> By the way, I the conditional shown above is harder to understand than
> necessary.  I like to get the errors out of the way at the beginning,
> like this:
>
>      if (uri && has_channels) {
>          error_setg(errp, "'uri' and 'channels' arguments are mutually "
>                     "exclusive; exactly one of the two should be present in "
>                     "'migrate' qmp command ");
>          return;
>      }
>      if (!uri && !has_channels) {
>          error_setg(errp, "neither 'uri' or 'channels' argument are "
>                     "specified in 'migrate' qmp command ");
>          return;
>      }
>
>      if (channels) {
>          /* To verify that Migrate channel list has only item */
>
> Or even
>
>      if (!uri == !has_channels) {
>          error_setg(errp, "need either 'uri' or 'channels' argument")
>          return;
>      }
>
> Suggestion, not demand.  If you do it, separate patch.
>
Yeah, I probably opted for 'if, else if' block because I found it easy 
to have all 4 options in that manner.

'!uri == !has_channels' is same as '!uri && !has_channels' right ?

Now looking at the Qemu code, it is better to have conditional 
statements the way you mentioned. Will do it in a separate patch.


Regards,

Het Gala

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

> On 28/11/23 3:29 pm, Markus Armbruster wrote:
>> Het Gala <het.gala@nutanix.com> writes:
>>
>>> On 28/11/23 12:46 pm, Markus Armbruster wrote:
>>>> 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:
>>> Yeah, I made the changes in v2 patchset.
>>>>> ---
>>>>>    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) {
>>>> 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().
>>> Markus, sorry if I was not able to put point clearly, what I meant is that the local 'channel' variable used in qmp_migrate() i.e.
>>>
>>> 'MigrationChannel *channel = NULL', is defined in qmp_migrate() and if the user opts for 'uri' then '@channels' coming from hmp_migrate() will be NULL, and then migrate_uri_parse() will populate memory into 'channel', and that is not getting freed after it's use is over.
>>>
>>> I think, that is where memory leak might be happening ?
>> Aha!
>>
>>      if (uri && has_channels) {
>>          error_setg(errp, "'uri' and 'channels' arguments are mutually "
>>                     "exclusive; exactly one of the two should be present in "
>>                     "'migrate' qmp command ");
>>          return;
>>      } else if (channels) {
>>          /* To verify that Migrate channel list has only item */
>>          if (channels->next) {
>>              error_setg(errp, "Channel list has more than one entries");
>>              return;
>>          }
>>          channel = channels->value;
>>      } else if (uri) {
>>          /* caller uses the old URI syntax */
>>          if (!migrate_uri_parse(uri, &channel, errp)) {
>>              return;
>>          }
>>      } else {
>>          error_setg(errp, "neither 'uri' or 'channels' argument are "
>>                     "specified in 'migrate' qmp command ");
>>          return;
>>      }
>>
>> At this point, @channel is either channels->value, or from
>> migrate_uri_parse().
>>
>> We must not free in the former case, we must free in the latter case,
>>
>> Before your patch, we don't free.  Memory leak in the latter case.
>>
>> Afterwards, we free.  Double-free in the former case.
>>
>> You could guard the free, like so:
>>
>>      if (uri) {
>>          qapi_free_MigrationChannel(channel);
>>      }
>
> Yeah, you explained it right. The above solution seems fine to me.
>
> I am just curious to ask this: can we use g_autoptr() for local vaiable 'channel' and 'addr' ? As we are not passing these variables to the caller function, nor we are trying to transfer their ownership to another variable, so use of g_steal_pointer() also might not be required ?

You could try something like

diff --git a/migration/migration.c b/migration/migration.c
index 28a34c9068..7faa9c2ebd 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1932,7 +1932,7 @@ void qmp_migrate(const char *uri, bool has_channels,
     bool resume_requested;
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
-    MigrationChannel *channel = NULL;
+    g_autoptr(MigrationChannel) channel = NULL;
     MigrationAddress *addr = NULL;
 
     /*
@@ -1949,18 +1949,18 @@ void qmp_migrate(const char *uri, bool has_channels,
             error_setg(errp, "Channel list has more than one entries");
             return;
         }
-        channel = channels->value;
+        addr = channels->value->addr;
     } else if (uri) {
         /* caller uses the old URI syntax */
         if (!migrate_uri_parse(uri, &channel, errp)) {
             return;
         }
+        addr = channel->addr;
     } else {
         error_setg(errp, "neither 'uri' or 'channels' argument are "
                    "specified in 'migrate' qmp command ");
         return;
     }
-    addr = channel->addr;
 
     /* transport mechanism not suitable for migration? */
     if (!migration_channels_and_transport_compatible(addr, errp)) {

Untested.

>>
>> By the way, I the conditional shown above is harder to understand than
>> necessary.  I like to get the errors out of the way at the beginning,
>> like this:
>>
>>      if (uri && has_channels) {
>>          error_setg(errp, "'uri' and 'channels' arguments are mutually "
>>                     "exclusive; exactly one of the two should be present in "
>>                     "'migrate' qmp command ");
>>          return;
>>      }
>>      if (!uri && !has_channels) {
>>          error_setg(errp, "neither 'uri' or 'channels' argument are "
>>                     "specified in 'migrate' qmp command ");
>>          return;
>>      }
>>
>>      if (channels) {
>>          /* To verify that Migrate channel list has only item */
>>
>> Or even
>>
>>      if (!uri == !has_channels) {
>>          error_setg(errp, "need either 'uri' or 'channels' argument")
>>          return;
>>      }
>>
>> Suggestion, not demand.  If you do it, separate patch.
>>
> Yeah, I probably opted for 'if, else if' block because I found it easy to have all 4 options in that manner.
>
> '!uri == !has_channels' is same as '!uri && !has_channels' right ?

No.  It's "either both are null/false, or both are non-null/true".

> Now looking at the Qemu code, it is better to have conditional statements the way you mentioned. Will do it in a separate patch.
>
>
> Regards,
>
> Het Gala