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

[Markus Armbruster <armbru@redhat.com>]

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.