Re: [PATCH v2 2/3] savevm: Fix memory leak of vmstate_configuration

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Dec 28, 2020 at 05:00:52PM +0800, g00517791 wrote:
> From: Jinhao Gao <gaojinhao@huawei.com>
> 
> When VM migrate VMState of configuration, the fields(name and capabilities)
> of configuration having a flag of VMS_ALLOC need to allocate memory. If the
> src doesn't free memory of capabilities in SaveState after save VMState of
> configuration, or the dst doesn't free memory of name and capabilities in post
> load of configuration, it may result in memory leak of name and capabilities.
> We free memory in configuration_post_save and configuration_post_load func,
> which prevents memory leak.
> 
> Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  migration/savevm.c | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 5f937a2762..13f1a5dab7 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -314,6 +314,16 @@ static int configuration_pre_save(void *opaque)
>      return 0;
>  }
>  
> +static int configuration_post_save(void *opaque)
> +{
> +    SaveState *state = opaque;
> +
> +    g_free(state->capabilities);
> +    state->capabilities = NULL;
> +    state->caps_count = 0;
> +    return 0;
> +}
> +
>  static int configuration_pre_load(void *opaque)
>  {
>      SaveState *state = opaque;
> @@ -364,24 +374,36 @@ static int configuration_post_load(void *opaque, int version_id)
>  {
>      SaveState *state = opaque;
>      const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
> +    int ret = 0;
>  
>      if (strncmp(state->name, current_name, state->len) != 0) {
>          error_report("Machine type received is '%.*s' and local is '%s'",
>                       (int) state->len, state->name, current_name);
> -        return -EINVAL;
> +        ret = -EINVAL;
> +        goto out;
>      }
>  
>      if (state->target_page_bits != qemu_target_page_bits()) {
>          error_report("Received TARGET_PAGE_BITS is %d but local is %d",
>                       state->target_page_bits, qemu_target_page_bits());
> -        return -EINVAL;
> +        ret = -EINVAL;
> +        goto out;
>      }
>  
>      if (!configuration_validate_capabilities(state)) {
> -        return -EINVAL;
> +        ret = -EINVAL;
> +        goto out;
>      }
>  
> -    return 0;
> +out:
> +    g_free((void *)state->name);
> +    state->name = NULL;
> +    state->len = 0;
> +    g_free(state->capabilities);
> +    state->capabilities = NULL;
> +    state->caps_count = 0;
> +
> +    return ret;
>  }
>  
>  static int get_capability(QEMUFile *f, void *pv, size_t size,
> @@ -515,6 +537,7 @@ static const VMStateDescription vmstate_configuration = {
>      .pre_load = configuration_pre_load,
>      .post_load = configuration_post_load,
>      .pre_save = configuration_pre_save,
> +    .post_save = configuration_post_save,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(len, SaveState),
>          VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, len),
> -- 
> 2.23.0