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

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

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