Re: [PATCH] kvm tools:Fix memory leakage in open all disks

[雷翔 <leixiang@xxxxxxxxxx>]

I also think this modification suggestion is better.
So I incorporated the modification suggestions into the patch,
hoping to be accepted.

On 2024/8/6 20:48, Alexandru Elisei wrote:
> Hi Will,
>
> On Mon, Aug 05, 2024 at 01:27:49PM +0100, Will Deacon wrote:
>> On Wed, Jul 10, 2024 at 06:00:53PM +0800, leixiang wrote:
>>> From 56b60cf70a0c5f7cdafe6804dabbe7112c10f7a1 Mon Sep 17 00:00:00 2001
>>> From: leixiang
>>> Date: Wed, 10 Jul 2024 17:45:51 +0800
>>> Subject: [PATCH v3] kvmtool:disk/core:Fix memory leakage in open all disks
>>>
>>> Fix memory leakage in disk/core disk_image__open_all when malloc disk failed,
>>> should free the disks that already malloced.
>>> And to avoid fields not being initialized in struct disk_image,
>>> replace malloc with calloc.
>>>
>>> Signed-off-by: Lei Xiang
>>> Suggested-by: Xie Ming
>>> ---
>>> disk/core.c | 8 +++++---
>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/disk/core.c b/disk/core.c
>>> index b00b0c0..a084cd4 100644
>>> --- a/disk/core.c
>>> +++ b/disk/core.c
>>> @@ -170,9 +170,11 @@ static struct disk_image **disk_image__open_all(struct kvm *kvm)
>>> wwpn = params[i].wwpn;
>>>
>>> if (wwpn) {
>>> - disks[i] = malloc(sizeof(struct disk_image));
>>> - if (!disks[i])
>>> - return ERR_PTR(-ENOMEM);
>>> + disks[i] = calloc(1, sizeof(struct disk_image));
>>> + if (!disks[i]) {
>>> + err = ERR_PTR(-ENOMEM);
>>> + goto error;
>>> + }
>>> disks[i]->wwpn = wwpn;
>>> continue;
>>
>> Hmm, I'm still not sure about this. I don't think we should call
>> disk_image__close() for disks that weren't allocated via
>> disk_image__open(). Using calloc() might work, but it feels fragile.
>>
>> Instead, can we change the error handling to do something like below?
>>
>> Will
>>
>> --->8
>>
>> diff --git a/disk/core.c b/disk/core.c
>> index b00b0c0..b543d44 100644
>> --- a/disk/core.c
>> +++ b/disk/core.c
>> @@ -171,8 +171,11 @@ static struct disk_image **disk_image__open_all(struct kvm *kvm)
>>
>> if (wwpn) {
>> disks[i] = malloc(sizeof(struct disk_image));
>> - if (!disks[i])
>> - return ERR_PTR(-ENOMEM);
>> + if (!disks[i]) {
>> + err = ERR_PTR(-ENOMEM);
>> + goto error;
>> + }
>> +
>> disks[i]->wwpn = wwpn;
>> continue;
>> }
>> @@ -191,9 +194,15 @@ static struct disk_image **disk_image__open_all(struct kvm *kvm)
>>
>> return disks;
>> error:
>> - for (i = 0; i >> - if (!IS_ERR_OR_NULL(disks[i]))
>> + for (i = 0; i >> + if (IS_ERR_OR_NULL(disks[i]))
>> + continue;
>> +
>> + if (disks[i]->wwpn)
>> + free(disks[i]);
>> + else
>> disk_image__close(disks[i]);
>> + }
>>
>> free(disks);
>> return err;
>>
>>
>>> }
>
> This looks much better than my suggestion.
>
> Thanks,
> Alex

 

 

----

 

 

 

Attachment: v4-0001-kvmtool-disk-core-Fix-memory-leakage-in-open-all-.patch
Description: Binary data