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

leixiang
2024-07-10T08:18:09Z

[-- Attachment #1: Type: text/plain, Size: 2680 bytes --]

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@kylinos.cn>
>> Suggested-by: Xie Ming <xieming@kylinos.cn>
>> ---
>> 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
>>
>>

[-- Attachment #2: v2-0001-kvmtool-disk-core-Fix-memory-leakage-in-open-all-.patch --]
[-- Type: text/x-patch, Size: 954 bytes --]

From d1babe256904a24f4c9dcedd063a7d5ae9f40927 Mon Sep 17 00:00:00 2001
From: leixiang <leixiang@kylinos.cn>
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@kylinos.cn>
Suggested-by: Xie Ming <xieming@kylinos.cn>
---
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)