RE: [PATCH v3 3/3] vmstate: Fix memory leak in vmstate_handle_alloc()

[gaojinhao]

Thank you for review!

Jinhao Gao

-----Original Message-----
From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com] 
Sent: 2021年1月5日 19:18
To: gaojinhao <gaojinhao@huawei.com>
Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; Michael S . Tsirkin 
<mst@redhat.com>; David Gibson <david@gibson.dropbear.id.au>; Greg Kurz 
<groug@kaod.org>; Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan 
Berger <stefanb@linux.vnet.ibm.com>; Jason Wang <jasowang@redhat.com>; Juan 
Quintela <quintela@redhat.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>; 
zhukeqian <zhukeqian1@huawei.com>
Subject: Re: [PATCH v3 3/3] vmstate: Fix memory leak in vmstate_handle_alloc()

* Jinhao Gao (gaojinhao@huawei.com) wrote:
> Some memory allocated for fields having a flag of VMS_ALLOC in 
> SaveState may not free before VM load vmsd in migration. So we 
> pre-free memory before allocation in vmstate_handle_alloc() to avoid memleaks.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Yes, I think that's OK; it's actually pretty rare for this to happen; normally 
inwards migrations either succeed or fail and exit; doing multiple loads from 
snapshots is valid and I guess COLO hits this as well.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/vmstate.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/migration/vmstate.c b/migration/vmstate.c index 
> e9d2aef66b..873f76739f 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -70,6 +70,7 @@ static void vmstate_handle_alloc(void *ptr, const 
> VMStateField *field,
>          gsize size = vmstate_size(opaque, field);
>          size *= vmstate_n_elems(opaque, field);
>          if (size) {
> +            g_free(*(void **)ptr);
>              *(void **)ptr = g_malloc(size);
>          }
>      }
> --
> 2.23.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK