Re: [PATCH v2] libxc: fix memory leak in migration v2

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 26/07/2015 22:34, Wei Liu wrote:
> Originally there was only one counter to keep track of pages. It was
> used erroneously to keep track of how many pages were mapped and how
> many pages needed to be sent. In the end munmap(2) always had 0 as the
> length argument, which resulted in leaking the mapping.
>
> This problem was discovered on 32bit toolstack because 32bit applications
> have notably smaller address space. In fact this bug affects 64bit
> toolstack too.
>
> Use a separate counter to keep track of the number of mapped pages to
> solve this problem.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>
> v2:
> 1. Use nr_pages_mapped to produce smaller patch.
> 2. Fix typos in commit message.
> ---
>  tools/libxc/xc_sr_save.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
> index d63b783..1b6be2a 100644
> --- a/tools/libxc/xc_sr_save.c
> +++ b/tools/libxc/xc_sr_save.c
> @@ -84,7 +84,7 @@ static int write_batch(struct xc_sr_context *ctx)
>      void **guest_data = NULL;
>      void **local_pages = NULL;
>      int *errors = NULL, rc = -1;
> -    unsigned i, p, nr_pages = 0;
> +    unsigned i, p, nr_pages = 0, nr_pages_mapped = 0;
>      unsigned nr_pfns = ctx->save.nr_batch_pfns;
>      void *page, *orig_page;
>      uint64_t *rec_pfns = NULL;
> @@ -160,6 +160,7 @@ static int write_batch(struct xc_sr_context *ctx)
>              PERROR("Failed to map guest pages");
>              goto err;
>          }
> +        nr_pages_mapped = nr_pages;
>  
>          for ( i = 0, p = 0; i < nr_pfns; ++i )
>          {
> @@ -262,7 +263,7 @@ static int write_batch(struct xc_sr_context *ctx)
>   err:
>      free(rec_pfns);
>      if ( guest_mapping )
> -        munmap(guest_mapping, nr_pages * PAGE_SIZE);
> +        munmap(guest_mapping, nr_pages_mapped * PAGE_SIZE);
>      for ( i = 0; local_pages && i < nr_pfns; ++i )
>          free(local_pages[i]);
>      free(iov);