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

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

On 24/07/2015 23:40, 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 send. In the end munmap(2) always has 0 as the
> length argument, which resulted in leaking the mapping.
>
> This problem is discovered on 32bit toolstack because 32bit application
> has notably smaller address space. In fact this bug affects 64bit
> toolstack too.
>
> Use a separate counter to keep track of how many pages to send to solve
> this issue.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Yikes - good catch.  (This logic definitely worked at first, because I
had debugging which confirmed as much.  It must have gotten broken in
the 18 months of minor tweaks since it was originally written).

I believe the patch is correct.  However, can I recommend instead having
"nr_pages_mapped" which is set after the call to xc_map_foreign_bulk(). 
This will make a shorter and more-obviously correct patch.

Valgrind doesn't track mmap()/munmap() calls because of their typical
use for shared libraries.  However, it turns out that the
VALGRIND_{MALLOC,FREE}LIKE_BLOCK client requests can be used to mark
arbitrary blocks of memory as tracked by the standard memcheck
infrastructure.

For 4.7 (which happens to coincide with the splitting up of libxc), I
recommend introducing xc_unmap() and requiring that all uses of
xc_map_$FOO() use it.  This way, the client requests can be present in
the library, and all mappings of foreign memory can be tracked by
valgrind in debug builds.

~Andrew