Dear Alex,
Thanks for your reply.
On 2024/7/10 16:27, Alexandru Elisei wrote:
> Hi,
>
> On Wed, Jul 10, 2024 at 04:12:37PM +0800, leixiang wrote:
>> 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.
>
> When and where is disk_image__new() called?
>
Sorry, I was ignored the 'continue' in the code flow.
There is no doubt that your suggestions are forward-looking,
and I have made changes to the patch according to your suggestions.
> Thanks,
> Alex
Thank you very much!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;
}
--
2.34.1
