From: Philippe Waroquiers Date: Thu, 14 Jan 2016 20:23:11 +0000 (+0000) Subject: fix n-i-bz false positive leaks due to aspacemgr merging non heap segments with heap... X-Git-Tag: svn/VALGRIND_3_12_0~264 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=647642d093c7a8bf733016a4cb136e6414778ffa;p=thirdparty%2Fvalgrind.git fix n-i-bz false positive leaks due to aspacemgr merging non heap segments with heap segments. aspace mgr provides VG_(am_mmap_client_heap) that mmaps memory and marks it as being client heap memory. Marking superblock segments used for malloc/free as heap is critical for correct leak search: segments mmap-ed for malloc/free cannot be considered as part of the root set. On the other hand, other mmap-ed segments cannot be marked as client heap, otherwise these segments will not be part of the root set, and will not be scanned. aspacemgr merges adjacent segments when they have the same characteristics e.g. kind, RWX and isCH (is client heap) must be the same (see function maybe_merge_nsegments). However, VG_(am_mmap_client_heap) has a bug: * it first mmaps a normal segment (not marked as heap) using VG_(am_mmap_anon_float_client) * it then searches the segment that contains the just mmap-ed address and marks it as heap. The problem is that VG_(am_mmap_anon_float_client) has already possibly merged the new segment with a neighbour segment, without taking the to be marked isCH into account, as the newly allocated memory has not yet been marked as Client Heap. So, this results in some memory being marked as client heap, while it in fact is not client heap. This memory will then not be scanned by the leak search. The fix consists in having VG_(am_mmap_anon_float_client) and VG_(am_mmap_client_heap) calling a new function am_mmap_anon_float_client, which will mark (or not) the new segment as client heap *before* trying to merge it with neighbouring segments. Then the new (heap) segment will only be merged with neighbours that are also client heap segments. git-svn-id: svn://svn.valgrind.org/valgrind/trunk@15759 --- diff --git a/NEWS b/NEWS index d9a18c535b..07c750efc2 100644 --- a/NEWS +++ b/NEWS @@ -62,6 +62,7 @@ where XXXXXX is the bug number as listed below. n-i-bz Fix incorrect (or infinite loop) unwind on RHEL7 x86 32 bits n-i-bz massif --pages-as-heap=yes does not report peak caused by mmap+munmap +n-i-bz false positive leaks due to aspacemgr merging non heap segments with heap segments. Release 3.11.0 (22 September 2015) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/coregrind/m_aspacemgr/aspacemgr-linux.c b/coregrind/m_aspacemgr/aspacemgr-linux.c index 7ce3c996ea..0a8f6753d3 100644 --- a/coregrind/m_aspacemgr/aspacemgr-linux.c +++ b/coregrind/m_aspacemgr/aspacemgr-linux.c @@ -2476,7 +2476,7 @@ SysRes VG_(am_mmap_anon_fixed_client) ( Addr start, SizeT length, UInt prot ) /* Map anonymously at an unconstrained address for the client, and update the segment array accordingly. */ -SysRes VG_(am_mmap_anon_float_client) ( SizeT length, Int prot ) +static SysRes am_mmap_anon_float_client ( SizeT length, Int prot, Bool isCH ) { SysRes sres; NSegment seg; @@ -2524,12 +2524,17 @@ SysRes VG_(am_mmap_anon_float_client) ( SizeT length, Int prot ) seg.hasR = toBool(prot & VKI_PROT_READ); seg.hasW = toBool(prot & VKI_PROT_WRITE); seg.hasX = toBool(prot & VKI_PROT_EXEC); + seg.isCH = isCH; add_segment( &seg ); AM_SANITY_CHECK; return sres; } +SysRes VG_(am_mmap_anon_float_client) ( SizeT length, Int prot ) +{ + return am_mmap_anon_float_client (length, prot, False /* isCH */); +} /* Map anonymously at an unconstrained address for V, and update the segment array accordingly. This is fundamentally how V allocates @@ -2738,21 +2743,13 @@ SysRes VG_(am_shared_mmap_file_float_valgrind) fd, offset ); } -/* Convenience wrapper around VG_(am_mmap_anon_float_client) which also +/* Similar to VG_(am_mmap_anon_float_client) but also marks the segment as containing the client heap. This is for the benefit of the leak checker which needs to be able to identify such segments so as not to use them as sources of roots during leak checks. */ SysRes VG_(am_mmap_client_heap) ( SizeT length, Int prot ) { - SysRes res = VG_(am_mmap_anon_float_client)(length, prot); - - if (! sr_isError(res)) { - Addr addr = sr_Res(res); - Int ix = find_nsegment_idx(addr); - - nsegments[ix].isCH = True; - } - return res; + return am_mmap_anon_float_client (length, prot, True /* isCH */); } /* --- --- munmap helper --- --- */ diff --git a/coregrind/pub_core_aspacemgr.h b/coregrind/pub_core_aspacemgr.h index 003d94e44b..2e846724e9 100644 --- a/coregrind/pub_core_aspacemgr.h +++ b/coregrind/pub_core_aspacemgr.h @@ -254,7 +254,7 @@ extern SysRes VG_(am_mmap_file_float_valgrind) extern SysRes VG_(am_shared_mmap_file_float_valgrind) ( SizeT length, UInt prot, Int fd, Off64T offset ); -/* Convenience wrapper around VG_(am_mmap_anon_float_client) which also +/* Similar to VG_(am_mmap_anon_float_client) but also marks the segment as containing the client heap. */ extern SysRes VG_(am_mmap_client_heap) ( SizeT length, Int prot );