Dear Alex,
Thank you for your reply and suggestions.
On 2024/7/9 18:12, Alexandru Elisei wrote:
> Hi,
>
> Adding the kvmtool maintainers (you can find them in the README file).
>
> On Tue, Jun 18, 2024 at 03:52:47PM +0800, leixiang wrote:
>> 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>
>> ---
>> disk/core.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/disk/core.c b/disk/core.c
>> index dd2f258..affeece 100644
>> --- a/disk/core.c
>> +++ b/disk/core.c
>> @@ -195,8 +195,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;
>> disks[i]->tpgt = tpgt;
>
> Currently, the latest patch on branch master is ca31abf5d9c3 ("arm64: Allow
> the user to select the max SVE vector length"), and struct disk_image
> doesn't have a tpgt field. Did you write this patch on a local branch?
>
>> continue;
>
There is no doubt that you are correct, I had realize that I git clone a wrong repo.
> This is what the 'error' label does:
>
> error:
> for (i = 0; i < count; i++)
> if (!IS_ERR_OR_NULL(disks[i]))
> disk_image__close(disks[i]);
>
> free(disks);
> return err;
>
> And disk_image__close() ends up poking all sort of fields from struct
> disk_image, including dereferencing pointers embedded in the struct. If
> WWPN is specified for a disk, struct disk_image is allocated using malloc
> as above, the field wwwpn is set and the rest of the fields are left
> uninitialized. Because of this, calling disk_image__close() on a struct
> disk_image with wwpn can lead to all sorts of nasty things happening.
>
> May I suggest allocating disks[i] using calloc in the wwpn case to fix
> this? Ideally, you would have two patches:
>
> 1. A patch that changes the disk[i] allocation to calloc(), to prevent
> disk_image__close() accessing unitialized fields when disk_image__open()
> fails after initialized a WWPN disk.
>
> 2. This patch.
>
When the new disk_image is allocated successfully,
the fields will eventually be initialized by disk_image__new().
And disk_image__close() accessing fields also checked before use.
So I don't think it's necessary to replace malloc with calloc.
> Thanks,
> Alex
>
>> --
>> 2.34.1
>>
>>From d1babe256904a24f4c9dcedd063a7d5ae9f40927 Mon Sep 17 00:00:00 2001
From: leixiang <leixiang@xxxxxxxxxx>
Date: Wed, 10 Jul 2024 16:06:02 +0800
Subject: [PATCH v2] 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>
---
disk/core.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/disk/core.c b/disk/core.c
index b00b0c0..ce2224d 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;
}
--
2.34.1
