|
From: <sv...@va...> - 2016-01-14 20:23:19
|
Author: philippe
Date: Thu Jan 14 20:23:11 2016
New Revision: 15759
Log:
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.
Modified:
trunk/NEWS
trunk/coregrind/m_aspacemgr/aspacemgr-linux.c
trunk/coregrind/pub_core_aspacemgr.h
Modified: trunk/NEWS
==============================================================================
--- trunk/NEWS (original)
+++ trunk/NEWS Thu Jan 14 20:23:11 2016
@@ -62,6 +62,7 @@
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)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Modified: trunk/coregrind/m_aspacemgr/aspacemgr-linux.c
==============================================================================
--- trunk/coregrind/m_aspacemgr/aspacemgr-linux.c (original)
+++ trunk/coregrind/m_aspacemgr/aspacemgr-linux.c Thu Jan 14 20:23:11 2016
@@ -2476,7 +2476,7 @@
/* 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 @@
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 @@
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 --- --- */
Modified: trunk/coregrind/pub_core_aspacemgr.h
==============================================================================
--- trunk/coregrind/pub_core_aspacemgr.h (original)
+++ trunk/coregrind/pub_core_aspacemgr.h Thu Jan 14 20:23:11 2016
@@ -254,7 +254,7 @@
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 );
|
|
From: Julian S. <js...@ac...> - 2016-01-19 10:46:23
|
Hi Philippe, Well done for spotting this. Question: is this a new bug (regression), or something that has always been there? I am unclear. J On 14/01/16 21:23, sv...@va... wrote: > Author: philippe > Date: Thu Jan 14 20:23:11 2016 > New Revision: 15759 > > Log: > fix n-i-bz false positive leaks due to aspacemgr merging non heap segments with heap segments. |
|
From: Philippe W. <phi...@sk...> - 2016-01-19 20:28:57
|
On Tue, 2016-01-19 at 11:46 +0100, Julian Seward wrote: > Hi Philippe, > > Well done for spotting this. Question: is this a new bug (regression), > or something that has always been there? I am unclear. This is an old bug. It was already existing at least since 3.9.0 (even if the involved function has changed of name since then). Philippe |