Het Gala <[email protected]> writes:
> On 28/11/23 3:29 pm, Markus Armbruster wrote:
>> 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);
>> }
>
> 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
