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 <leixiang@xxxxxxxxxx>
>>> 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 <leixiang@xxxxxxxxxx>
>>> Suggested-by: Xie Ming <xieming@xxxxxxxxxx>
>>> ---
>>> 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 < count; i++)
>> - if (!IS_ERR_OR_NULL(disks[i]))
>> + for (i = 0; i < count; 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,
> AlexFrom f9ebbe412cffafbf06c738336ee45942d7c1977f Mon Sep 17 00:00:00 2001
From: leixiang <leixiang@xxxxxxxxxx>
Date: Wed, 10 Jul 2024 17:45:51 +0800
Subject: [PATCH v4] 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.
Signed-off-by: Lei Xiang <leixiang@xxxxxxxxxx>
Suggested-by: Xie Ming <xieming@xxxxxxxxxx>
Suggested-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
Suggested-by: Will Deacon <will@xxxxxxxxxx>
---
disk/core.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/disk/core.c b/disk/core.c
index b00b0c0..91db277 100644
--- a/disk/core.c
+++ b/disk/core.c
@@ -171,8 +171,10 @@ 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,10 +193,15 @@ static struct disk_image **disk_image__open_all(struct kvm *kvm)
return disks;
error:
- for (i = 0; i < count; i++)
- if (!IS_ERR_OR_NULL(disks[i]))
- disk_image__close(disks[i]);
+ for (i = 0; i < count; 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;
}
--
2.34.1
