Het Gala <[email protected]> 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<[email protected]> 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.
