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

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

[Wei Liu]

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>
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);
-- 
2.1.4

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

[Andrew Cooper]

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);

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

[Ian Campbell]

On Sun, 2015-07-26 at 22:36 +0100, Andrew Cooper wrote:
> 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>

acked and applied, thanks all.