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

[Alexandru Elisei <alexandru.elisei@arm.com>]

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