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@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;
>
>
> > }
This looks much better than my suggestion.
Thanks,
Alex
