Peter Xu <pet...@redhat.com> writes:
> On Thu, Nov 30, 2023 at 07:35:43PM +0100, Markus Armbruster wrote:
>> Peter Xu <pet...@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.g...@nutanix.com>
>> >> Suggested-by: Markus Armbruster <arm...@redhat.com>
>> >
>> > Reviewed-by: Peter Xu <pet...@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.
