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

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

[leixiang]

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;
 			continue;
-- 
2.34.1

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

[Alexandru Elisei]

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;

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.

Thanks,
Alex

> -- 
> 2.34.1
> 
> 

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

[leixiang]

[-- 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)
 
 		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

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

[Alexandru Elisei]

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@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.

When and where is disk_image__new() called?

Thanks,
Alex

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

[leixiang]

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

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@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.
> 
> 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!

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

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

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

[Alexandru Elisei]

Hi,

On Wed, Jul 10, 2024 at 06:00:53PM +0800, leixiang wrote:
> 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@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.
> > 
> > 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. 

Great, thanks for checking, I was worried that there was something that I
missed.

Thanks,
Alex

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

[Will Deacon]

On Wed, Jul 10, 2024 at 06:00:53PM +0800, leixiang wrote:
> From 56b60cf70a0c5f7cdafe6804dabbe7112c10f7a1 Mon Sep 17 00:00:00 2001
> From: leixiang <leixiang@kylinos.cn>
> 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@kylinos.cn>
> Suggested-by: Xie Ming <xieming@kylinos.cn>
> ---
>  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;


>  		}
> -- 
> 2.34.1
> 

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

[Alexandru Elisei]

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

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

[雷翔]

[-- Attachment #1: Type: text/html, Size: 3802 bytes --]

[-- Attachment #2: v4-0001-kvmtool-disk-core-Fix-memory-leakage-in-open-all-.patch --]
[-- Type: application/octet-stream, Size: 1479 bytes --]

From f9ebbe412cffafbf06c738336ee45942d7c1977f Mon Sep 17 00:00:00 2001
From: leixiang <leixiang@kylinos.cn>
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@kylinos.cn>
Suggested-by: Xie Ming <xieming@kylinos.cn>
Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com>
Suggested-by: Will Deacon <will@kernel.org>
---
 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

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

[leixiang]

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

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

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

From f9ebbe412cffafbf06c738336ee45942d7c1977f Mon Sep 17 00:00:00 2001
From: leixiang <leixiang@kylinos.cn>
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@kylinos.cn>
Suggested-by: Xie Ming <xieming@kylinos.cn>
Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com>
Suggested-by: Will Deacon <will@kernel.org>
---
 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