From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] libxc: fix memory leak in migration v2 Date: Sun, 26 Jul 2015 17:47:27 +0100 Message-ID: <55B50F1F.7040803@citrix.com> References: <1437777627-15758-1-git-send-email-wei.liu2@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1437777627-15758-1-git-send-email-wei.liu2@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu , xen-devel@lists.xen.org Cc: Ian Jackson , Ian Campbell List-Id: xen-devel@lists.xenproject.org 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 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