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