[PATCH v4] migration: Plug memory leak with migration URIs

[PATCH v4] migration: Plug memory leak with migration URIs

[Het Gala]

migrate_uri_parse() allocates memory to 'channel' if the user
opts for old syntax - uri, which is leaked because there is no
code for freeing 'channel'.
So, free channel to avoid memory leak in case where 'channels'
is empty and uri parsing is required.

Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow")
Signed-off-by: Het Gala <het.gala@nutanix.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
---
 migration/migration.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 28a34c9068..34340f3440 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -515,7 +515,7 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
                                           MigrationChannelList *channels,
                                           Error **errp)
 {
-    MigrationChannel *channel = NULL;
+    g_autoptr(MigrationChannel) channel = NULL;
     MigrationAddress *addr = NULL;
     MigrationIncomingState *mis = migration_incoming_get_current();
 
@@ -533,18 +533,18 @@ static void qemu_start_incoming_migration(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-incoming' qmp command ");
         return;
     }
-    addr = channel->addr;
 
     /* transport mechanism not suitable for migration? */
     if (!migration_channels_and_transport_compatible(addr, errp)) {
@@ -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)) {
-- 
2.22.3

Re: [PATCH v4] migration: Plug memory leak with migration URIs

[Markus Armbruster]

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

> migrate_uri_parse() allocates memory to 'channel' if the user
> opts for old syntax - uri, which is leaked because there is no
> code for freeing 'channel'.
> So, free channel to avoid memory leak in case where 'channels'
> is empty and uri parsing is required.
>
> Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow")
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>

Tested-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v4] migration: Plug memory leak with migration URIs

[Peter Xu]

On Wed, Nov 29, 2023 at 08:43:01PM +0000, Het Gala wrote:
> migrate_uri_parse() allocates memory to 'channel' if the user
> opts for old syntax - uri, which is leaked because there is no
> code for freeing 'channel'.
> So, free channel to avoid memory leak in case where 'channels'
> is empty and uri parsing is required.
> 
> Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow")
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>

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

> @@ -533,18 +533,18 @@ static void qemu_start_incoming_migration(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-incoming' qmp command ");
>          return;
>      }
> -    addr = channel->addr;

Why these "addr" lines need change?  Won't that behave the same as before?

Thanks,

-- 
Peter Xu

Re: [PATCH v4] migration: Plug memory leak with migration URIs

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Nov 29, 2023 at 08:43:01PM +0000, Het Gala wrote:
>> migrate_uri_parse() allocates memory to 'channel' if the user
>> opts for old syntax - uri, which is leaked because there is no
>> code for freeing 'channel'.
>> So, free channel to avoid memory leak in case where 'channels'
>> is empty and uri parsing is required.
>> 
>> Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow")
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
>> @@ -533,18 +533,18 @@ static void qemu_start_incoming_migration(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-incoming' qmp command ");
>>          return;
>>      }
>> -    addr = channel->addr;
>
> Why these "addr" lines need change?  Won't that behave the same as before?

In the first case, @channel is now null.  If we left the assignment to
@addr alone, it would crash.  Clearer now?

Re: [PATCH v4] migration: Plug memory leak with migration URIs

[Peter Xu]

On Thu, Nov 30, 2023 at 07:35:43PM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Nov 29, 2023 at 08:43:01PM +0000, Het Gala wrote:
> >> migrate_uri_parse() allocates memory to 'channel' if the user
> >> opts for old syntax - uri, which is leaked because there is no
> >> code for freeing 'channel'.
> >> So, free channel to avoid memory leak in case where 'channels'
> >> is empty and uri parsing is required.
> >> 
> >> Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow")
> >> Signed-off-by: Het Gala <het.gala@nutanix.com>
> >> Suggested-by: Markus Armbruster <armbru@redhat.com>
> >
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> >
> >> @@ -533,18 +533,18 @@ static void qemu_start_incoming_migration(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-incoming' qmp command ");
> >>          return;
> >>      }
> >> -    addr = channel->addr;
> >
> > Why these "addr" lines need change?  Won't that behave the same as before?
> 
> In the first case, @channel is now null.  If we left the assignment to
> @addr alone, it would crash.  Clearer now?

Is it this one?

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

It returns already?

Thanks,

-- 
Peter Xu

Re: [PATCH v4] migration: Plug memory leak with migration URIs

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Nov 30, 2023 at 07:35:43PM +0100, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Wed, Nov 29, 2023 at 08:43:01PM +0000, Het Gala wrote:
>> >> migrate_uri_parse() allocates memory to 'channel' if the user
>> >> opts for old syntax - uri, which is leaked because there is no
>> >> code for freeing 'channel'.
>> >> So, free channel to avoid memory leak in case where 'channels'
>> >> is empty and uri parsing is required.
>> >> 
>> >> Fixes: 5994024f ("migration: Implement MigrateChannelList to qmp migration flow")
>> >> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> >> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> >
>> > Reviewed-by: Peter Xu <peterx@redhat.com>
>> >
>> >> @@ -533,18 +533,18 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
      -    MigrationChannel *channel = NULL;
      +    g_autoptr(MigrationChannel) channel = NULL;
           MigrationAddress *addr = NULL;
           MigrationIncomingState *mis = migration_incoming_get_current();

           /*
            * Having preliminary checks for uri and channel
            */
           if (uri && has_channels) {
               error_setg(errp, "'uri' and 'channels' arguments are mutually "
                          "exclusive; exactly one of the two should be present in "
                          "'migrate-incoming' 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;
>> >> +        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-incoming' qmp command ");
>> >>          return;
>> >>      }
>> >> -    addr = channel->addr;
>> >
>> > Why these "addr" lines need change?  Won't that behave the same as before?
>> 
>> In the first case, @channel is now null.  If we left the assignment to
>> @addr alone, it would crash.  Clearer now?
>
> Is it this one?
>
>     if (uri && has_channels) {
>         error_setg(errp, "'uri' and 'channels' arguments are mutually "
>                    "exclusive; exactly one of the two should be present in "
>                    "'migrate-incoming' qmp command ");
>         return;
>     }
>
> It returns already?

I meant the first visible case, i.e. if (channels).  Sorry for being
less than clear!

The problem is to free the result of migrate_uri_parse().

The patch's solution is to use @channel *only* for holding that result,
so it can be g_autoptr: drop channel = channels->value from the if
(channels) conditional.

Since this breaks addr = channel->addr, we move that assignment into the
conditionals that reach it, which lets us unbreak it the if (channels)
one.

Re: [PATCH v4] migration: Plug memory leak with migration URIs

[Peter Xu]

On Fri, Dec 01, 2023 at 07:19:45AM +0100, Markus Armbruster wrote:
> I meant the first visible case, i.e. if (channels).  Sorry for being
> less than clear!
> 
> The problem is to free the result of migrate_uri_parse().
> 
> The patch's solution is to use @channel *only* for holding that result,
> so it can be g_autoptr: drop channel = channels->value from the if
> (channels) conditional.
> 
> Since this breaks addr = channel->addr, we move that assignment into the
> conditionals that reach it, which lets us unbreak it the if (channels)
> one.

My bad!  It also proved that my R-b was bold. :(  Thanks, Markus.
Since Juan's away, I'll prepare a pull.

-- 
Peter Xu