Menu
▾
▴
[Valgrind-developers] Valgrind: r15716 - in /trunk: NEWS coregrind/m_signals.c coregrind/pub_core_libcsignal.h include/pub_tool_libcsignal.h include/pub_tool_signals.h memcheck/mc_leakcheck.c memcheck/tests/leak-segv-jmp.c memcheck/tests/leak-segv-jmp.stderr.exp
|
From: <sv...@va...> - 2015-10-22 19:14:39
|
Author: philippe
Date: Thu Oct 22 20:14:30 2015
New Revision: 15716
Log:
Fix 353891 Assert 'bad_scanned_addr < VG_ROUNDDN(start+len, sizeof(Addr))' failed
All memory dereferences during leak search are checked either with
aspacemgr or using the VA-bits.
So, in theory, no memory fault should occur.
However, the leak search is done so as to resist to e.g.
- desynchronisation between the real pages mapped and the aspacemgr state.
- client pages mprotected against reading
- any other reason why dereferencing a client address would fail.
So, the function lc_scan_memory installs a fault catcher that
is called if a memory fault signal is raised during memory scan.
However, memory dereference is also done in the function heuristic_reachedness.
So, this function must also resist to memory fault.
This patch also installs a fault catcher for the function heuristic_reachedness.
More in details, the following changes are done:
* pub_tool_signal.h and m_signals.c :
VG_(set_fault_catcher) now returns the previously set fault catcher.
This is needed so that heuristic_reachedness/lc_scan_memory can save
and restore the previous fault catcher.
* mc_leakcheck.c:
Addition of leak_search_fault_catcher that contains the common
code for the (currently 2) fault catchers used during leak search.
* Modification of heuristic_reachedness and lc_scan_memory:
Add 2 (small) specific fault catcher that are calling the common
leak_search_fault_catcher.
* The way sigprocmask is handled has been changed:
Before this patch, lc_scan_memory was saving/restoring the procsigmask
for each scanned block (and was restoring it when the fault catcher
was longjmp-ing back to lc_scan_memory in case of SEGV or BUS.
This was causing 2 system calls for each block scanned.
Now, lc_scan_memory and heuristic_reachedness are not saving/restoring
the procmask: the work to reset the sigprocmask is only done
in leak_search_fault_catcher. This is more efficient as no syscall
anymore is done during leak search, except for (normally) unfrequent
SIGSEGV/BUS. It is also simpler as signal handling is now done at
a single place.
It is ok to reset the procmask (in fact, just remove the caught signal
from the process sigmask) as during leak search, no other activity than
the leak search is on-going, and so no other SEGV/BUS can be received
while the handler runs.
This gives moderate speed improvements for applications allocating a lot of
blocks (about 10% improvement when leak searching in 1 million small blocks).
Test case (slightly modified) by Matthias Schwarzott.
Modified:
trunk/NEWS
trunk/coregrind/m_signals.c
trunk/coregrind/pub_core_libcsignal.h
trunk/include/pub_tool_libcsignal.h
trunk/include/pub_tool_signals.h
trunk/memcheck/mc_leakcheck.c
trunk/memcheck/tests/leak-segv-jmp.c
trunk/memcheck/tests/leak-segv-jmp.stderr.exp
Modified: trunk/NEWS
==============================================================================
--- trunk/NEWS (original)
+++ trunk/NEWS Thu Oct 22 20:14:30 2015
@@ -36,6 +36,7 @@
353370 don't advertise RDRAND in cpuid for Core-i7-4910-like avx2 machine
353398 WARNING: unhandled amd64-solaris syscall: 207
353680 s390x: Crash with certain glibc versions due to non-implemented TBEGIN
+353891 Assert 'bad_scanned_addr < VG_ROUNDDN(start+len, sizeof(Addr))' failed
353917 unhandled amd64-solaris syscall fchdir(120)
353920 unhandled amd64-solaris syscall: 170
Modified: trunk/coregrind/m_signals.c
==============================================================================
--- trunk/coregrind/m_signals.c (original)
+++ trunk/coregrind/m_signals.c Thu Oct 22 20:14:30 2015
@@ -2538,16 +2538,13 @@
return True;
}
-static void (*fault_catcher)(Int sig, Addr addr) = NULL;
+static fault_catcher_t fault_catcher = NULL;
-void VG_(set_fault_catcher)(void (*catcher)(Int, Addr))
+fault_catcher_t VG_(set_fault_catcher)(fault_catcher_t catcher)
{
- if (0)
- VG_(debugLog)(0, "signals", "set fault catcher to %p\n", catcher);
- vg_assert2(NULL == catcher || NULL == fault_catcher,
- "Fault catcher is already registered");
-
+ fault_catcher_t prev_catcher = fault_catcher;
fault_catcher = catcher;
+ return prev_catcher;
}
static
Modified: trunk/coregrind/pub_core_libcsignal.h
==============================================================================
--- trunk/coregrind/pub_core_libcsignal.h (original)
+++ trunk/coregrind/pub_core_libcsignal.h Thu Oct 22 20:14:30 2015
@@ -52,7 +52,7 @@
const vki_sigset_t* set2 );
extern Int VG_(sigaddset) ( vki_sigset_t* set, Int signum );
-extern Int VG_(sigdelset) ( vki_sigset_t* set, Int signum );
+/* VG_(sigdelset) is in pub_tool_libcsignal.h */
extern Int VG_(sigismember) ( const vki_sigset_t* set, Int signum );
extern void VG_(sigaddset_from_set) ( vki_sigset_t* dst, const vki_sigset_t* src );
Modified: trunk/include/pub_tool_libcsignal.h
==============================================================================
--- trunk/include/pub_tool_libcsignal.h (original)
+++ trunk/include/pub_tool_libcsignal.h Thu Oct 22 20:14:30 2015
@@ -39,6 +39,11 @@
defines. Since we're operating right at the kernel interface, glibc's view
of the world is entirely irrelevant. */
+/* --- Signal set ops (only the ops used by tools) --- */
+extern Int VG_(sigdelset) ( vki_sigset_t* set, Int signum );
+/* Other Signal set ops are in pub_core_libcsignal.h and must be moved
+ here if needed by tools. */
+
/* --- Mess with the kernel's sig state --- */
extern Int VG_(sigprocmask) ( Int how, const vki_sigset_t* set,
vki_sigset_t* oldset );
Modified: trunk/include/pub_tool_signals.h
==============================================================================
--- trunk/include/pub_tool_signals.h (original)
+++ trunk/include/pub_tool_signals.h Thu Oct 22 20:14:30 2015
@@ -36,11 +36,14 @@
// Register an interest in apparently internal faults; used code which
// wanders around dangerous memory (ie, leakcheck). The catcher is
// not expected to return.
+// Returns the previously set fault_catcher (NULL if there was no fault
+// catcher set)
//
// It's frustrating that we need this header for a single function used
// only by Memcheck during leak checking. We should find a way to remove
// the need for this file.
-extern void VG_(set_fault_catcher)(void (*catcher)(Int sig, Addr addr));
+typedef void (*fault_catcher_t)(Int sig, Addr addr);
+extern fault_catcher_t VG_(set_fault_catcher)(fault_catcher_t catcher);
#endif // __PUB_TOOL_SIGNALS_H
Modified: trunk/memcheck/mc_leakcheck.c
==============================================================================
--- trunk/memcheck/mc_leakcheck.c (original)
+++ trunk/memcheck/mc_leakcheck.c Thu Oct 22 20:14:30 2015
@@ -683,6 +683,65 @@
&& MC_(is_valid_aligned_word)(a + 4);
}
+/* The below leak_search_fault_catcher is used to catch memory access
+ errors happening during leak_search. During the scan, we check
+ with aspacemgr and/or VA bits that each page or dereferenced location is
+ readable and belongs to the client. However, we still protect
+ against SIGSEGV and SIGBUS e.g. in case aspacemgr is desynchronised
+ with the real page mappings. Such a desynchronisation could happen
+ due to an aspacemgr bug. Note that if the application is using
+ mprotect(NONE), then a page can be unreadable but have addressable
+ and defined VA bits (see mc_main.c function mc_new_mem_mprotect).
+ Currently, 2 functions are dereferencing client memory during leak search:
+ heuristic_reachedness and lc_scan_memory.
+ Each such function has its own fault catcher, that will call
+ leak_search_fault_catcher with the proper 'who' and jmpbuf parameters. */
+static volatile Addr bad_scanned_addr;
+static
+void leak_search_fault_catcher ( Int sigNo, Addr addr,
+ const HChar *who, VG_MINIMAL_JMP_BUF(jmpbuf) )
+{
+ vki_sigset_t sigmask;
+
+ if (0)
+ VG_(printf)("OUCH! sig=%d addr=%#lx who=%s\n", sigNo, addr, who);
+
+ /* Signal handler runs with the signal masked.
+ Unmask the handled signal before longjmp-ing or return-ing.
+ Note that during leak search, we expect only SIGSEGV or SIGBUS
+ and we do not expect another occurence until we longjmp-ed!return-ed
+ to resume the leak search. So, it is safe to unmask the signal
+ here. */
+ /* First get current mask (by passing NULL as first arg) */
+ VG_(sigprocmask)(VKI_SIG_SETMASK, NULL, &sigmask);
+ /* Then set a new sigmask, with this signal removed from the mask. */
+ VG_(sigdelset)(&sigmask, sigNo);
+ VG_(sigprocmask)(VKI_SIG_SETMASK, &sigmask, NULL);
+
+ if (sigNo == VKI_SIGSEGV || sigNo == VKI_SIGBUS) {
+ bad_scanned_addr = addr;
+ VG_MINIMAL_LONGJMP(jmpbuf);
+ } else {
+ /* ??? During leak search, we are not supposed to receive any
+ other sync signal that these 2.
+ In theory, we should not call VG_(umsg) in a signal handler,
+ but better (try to) report this unexpected behaviour. */
+ VG_(umsg)("leak_search_fault_catcher:"
+ " unexpected signal %d, catcher %s ???\n",
+ sigNo, who);
+ }
+}
+
+// jmpbuf and fault_catcher used during heuristic_reachedness
+static VG_MINIMAL_JMP_BUF(heuristic_reachedness_jmpbuf);
+static
+void heuristic_reachedness_fault_catcher ( Int sigNo, Addr addr )
+{
+ leak_search_fault_catcher (sigNo, addr,
+ "heuristic_reachedness_fault_catcher",
+ heuristic_reachedness_jmpbuf);
+}
+
// If ch is heuristically reachable via an heuristic member of heur_set,
// returns this heuristic.
// If ch cannot be considered reachable using one of these heuristics,
@@ -696,6 +755,17 @@
MC_Chunk *ch, LC_Extra *ex,
UInt heur_set)
{
+
+ fault_catcher_t prev_catcher;
+
+ prev_catcher = VG_(set_fault_catcher)(heuristic_reachedness_fault_catcher);
+
+ // See leak_search_fault_catcher
+ if (VG_MINIMAL_SETJMP(heuristic_reachedness_jmpbuf) != 0) {
+ VG_(set_fault_catcher) (prev_catcher);
+ return LchNone;
+ }
+
if (HiS(LchStdString, heur_set)) {
// Detects inner pointers to Std::String for layout being
// length capacity refcount char_array[] \0
@@ -717,6 +787,7 @@
// ??? probably not a good idea, as I guess stdstring
// ??? allocator can be done via custom allocator
// ??? or even a call to malloc ????
+ VG_(set_fault_catcher) (prev_catcher);
return LchStdString;
}
}
@@ -735,6 +806,7 @@
&& is_valid_aligned_ULong(ch->data)) {
const ULong size = *((ULong*)ch->data);
if (size > 0 && (ch->szB - sizeof(ULong)) == size) {
+ VG_(set_fault_catcher) (prev_catcher);
return LchLength64;
}
}
@@ -761,6 +833,7 @@
const SizeT nr_elts = *((SizeT*)ch->data);
if (nr_elts > 0 && (ch->szB - sizeof(SizeT)) % nr_elts == 0) {
// ??? could check that ch->allockind is MC_AllocNewVec ???
+ VG_(set_fault_catcher) (prev_catcher);
return LchNewArray;
}
}
@@ -792,12 +865,14 @@
&& aligned_ptr_above_page0_is_vtable_addr(inner_addr)
&& aligned_ptr_above_page0_is_vtable_addr(first_addr)) {
// ??? could check that ch->allockind is MC_AllocNew ???
+ VG_(set_fault_catcher) (prev_catcher);
return LchMultipleInheritance;
}
}
}
}
+ VG_(set_fault_catcher) (prev_catcher);
return LchNone;
}
@@ -923,18 +998,13 @@
}
-static VG_MINIMAL_JMP_BUF(memscan_jmpbuf);
-static volatile Addr bad_scanned_addr;
-
+static VG_MINIMAL_JMP_BUF(lc_scan_memory_jmpbuf);
static
-void scan_all_valid_memory_catcher ( Int sigNo, Addr addr )
+void lc_scan_memory_fault_catcher ( Int sigNo, Addr addr )
{
- if (0)
- VG_(printf)("OUCH! sig=%d addr=%#lx\n", sigNo, addr);
- if (sigNo == VKI_SIGSEGV || sigNo == VKI_SIGBUS) {
- bad_scanned_addr = addr;
- VG_MINIMAL_LONGJMP(memscan_jmpbuf);
- }
+ leak_search_fault_catcher (sigNo, addr,
+ "lc_scan_memory_fault_catcher",
+ lc_scan_memory_jmpbuf);
}
// lc_scan_memory has 2 modes:
@@ -983,13 +1053,12 @@
#endif
Addr ptr = VG_ROUNDUP(start, sizeof(Addr));
const Addr end = VG_ROUNDDN(start+len, sizeof(Addr));
- vki_sigset_t sigmask;
+ fault_catcher_t prev_catcher;
if (VG_DEBUG_LEAKCHECK)
VG_(printf)("scan %#lx-%#lx (%lu)\n", start, end, len);
- VG_(sigprocmask)(VKI_SIG_SETMASK, NULL, &sigmask);
- VG_(set_fault_catcher)(scan_all_valid_memory_catcher);
+ prev_catcher = VG_(set_fault_catcher)(lc_scan_memory_fault_catcher);
/* Optimisation: the loop below will check for each begin
of SM chunk if the chunk is fully unaddressable. The idea is to
@@ -1016,19 +1085,9 @@
between VKI_PAGE_SIZE, SM_SIZE and sizeof(Addr) which are asserted in
MC_(detect_memory_leaks). */
- // During scan, we check with aspacemgr that each page is readable and
- // belongs to client.
- // We still protect against SIGSEGV and SIGBUS e.g. in case aspacemgr is
- // desynchronised with the real page mappings.
- // Such a desynchronisation could happen due to an aspacemgr bug.
- // Note that if the application is using mprotect(NONE), then
- // a page can be unreadable but have addressable and defined
- // VA bits (see mc_main.c function mc_new_mem_mprotect).
- if (VG_MINIMAL_SETJMP(memscan_jmpbuf) != 0) {
+ // See leak_search_fault_catcher
+ if (VG_MINIMAL_SETJMP(lc_scan_memory_jmpbuf) != 0) {
// Catch read error ...
- // We need to restore the signal mask, because we were
- // longjmped out of a signal handler.
- VG_(sigprocmask)(VKI_SIG_SETMASK, &sigmask, NULL);
# if defined(VGA_s390x)
// For a SIGSEGV, s390 delivers the page address of the bad address.
// For a SIGBUS, old s390 kernels deliver a NULL address.
@@ -1109,8 +1168,7 @@
ptr += sizeof(Addr);
}
- VG_(sigprocmask)(VKI_SIG_SETMASK, &sigmask, NULL);
- VG_(set_fault_catcher)(NULL);
+ VG_(set_fault_catcher)(prev_catcher);
}
@@ -1892,7 +1950,7 @@
tl_assert(ex->state == Unreached);
}
}
-
+
print_results( tid, lcp);
VG_(free) ( lc_markstack );
Modified: trunk/memcheck/tests/leak-segv-jmp.c
==============================================================================
--- trunk/memcheck/tests/leak-segv-jmp.c (original)
+++ trunk/memcheck/tests/leak-segv-jmp.c Thu Oct 22 20:14:30 2015
@@ -251,6 +251,7 @@
char **b10;
+char *interior_ptrs[3];
int mprotect_result = 0;
static void non_simd_mprotect (long tid, void* addr, long len)
{
@@ -268,6 +269,19 @@
#endif
}
+// can this work without global variable for return value?
+static void my_mprotect_none(void* addr, long len)
+{
+ if (RUNNING_ON_VALGRIND)
+ (void) VALGRIND_NON_SIMD_CALL2(non_simd_mprotect,
+ addr,
+ len);
+ else
+ mprotect_result = mprotect(addr,
+ len,
+ PROT_NONE);
+}
+
void f(void)
{
long pagesize;
@@ -279,7 +293,7 @@
for (i = 0; i < nr_ptr; i++)
b10[i] = (char*)b10;
b10[4000] = malloc (1000);
-
+
fprintf(stderr, "expecting no leaks\n");
fflush(stderr);
VALGRIND_DO_LEAK_CHECK;
@@ -306,32 +320,44 @@
if (pagesize == -1)
perror ("sysconf failed");
- if (RUNNING_ON_VALGRIND)
- (void) VALGRIND_NON_SIMD_CALL2(non_simd_mprotect, RNDPAGEDOWN(&b10[4000]), 2 * pagesize);
- else
- mprotect_result = mprotect((void*) RNDPAGEDOWN(&b10[4000]), 2 * pagesize, PROT_NONE);
+ my_mprotect_none((void*) RNDPAGEDOWN(&b10[4000]), 2 * pagesize);
fprintf(stderr, "mprotect result %d\n", mprotect_result);
fprintf(stderr, "expecting a leak again\n");
fflush(stderr);
VALGRIND_DO_LEAK_CHECK;
- if (RUNNING_ON_VALGRIND)
- (void) VALGRIND_NON_SIMD_CALL2(non_simd_mprotect,
- RNDPAGEDOWN(&b10[0]),
- RNDPAGEDOWN(&(b10[nr_ptr-1]))
- - RNDPAGEDOWN(&(b10[0])));
- else
- mprotect_result = mprotect((void*) RNDPAGEDOWN(&b10[0]),
+ my_mprotect_none((void*) RNDPAGEDOWN(&b10[0]),
RNDPAGEDOWN(&(b10[nr_ptr-1]))
- - RNDPAGEDOWN(&(b10[0])),
- PROT_NONE);
+ - RNDPAGEDOWN(&(b10[0])));
fprintf(stderr, "full mprotect result %d\n", mprotect_result);
fprintf(stderr, "expecting a leak again after full mprotect\n");
fflush(stderr);
VALGRIND_DO_LEAK_CHECK;
+ // allocate memory but keep only interior pointers to trigger various
+ // heuristics
+ // Allocate some memory:
+ interior_ptrs[0] = calloc (nr_ptr * sizeof(char*), 1);
+
+ // Inner pointer after 3 sizeT: triggers the stdstring heuristic:
+ interior_ptrs[2] = interior_ptrs[0] + 3 * sizeof(size_t);
+
+ // Inner pointer after 1 ULong: triggers the length64 heuristic:
+ interior_ptrs[1] = interior_ptrs[0] + sizeof(unsigned long);
+
+ // Inner pointer after a size: triggers the newarray heuristics.
+ interior_ptrs[0] += sizeof(size_t);
+
+ my_mprotect_none( (void*) RNDPAGEDOWN((interior_ptrs[0] - sizeof(size_t))),
+ RNDPAGEDOWN(nr_ptr * sizeof(char*)));
+ fprintf(stderr, "mprotect result %d\n", mprotect_result);
+
+ fprintf(stderr, "expecting heuristic not to crash after full mprotect\n");
+ fflush(stderr);
+ VALGRIND_DO_LEAK_CHECK;
+
fprintf(stderr, "finished\n");
}
Modified: trunk/memcheck/tests/leak-segv-jmp.stderr.exp
==============================================================================
--- trunk/memcheck/tests/leak-segv-jmp.stderr.exp (original)
+++ trunk/memcheck/tests/leak-segv-jmp.stderr.exp Thu Oct 22 20:14:30 2015
@@ -14,8 +14,8 @@
expecting a leak
1,000 bytes in 1 blocks are definitely lost in loss record ... of ...
at 0x........: malloc (vg_replace_malloc.c:...)
- by 0x........: f (leak-segv-jmp.c:281)
- by 0x........: main (leak-segv-jmp.c:344)
+ by 0x........: f (leak-segv-jmp.c:295)
+ by 0x........: main (leak-segv-jmp.c:370)
LEAK SUMMARY:
definitely lost: 1,000 bytes in 1 blocks
@@ -30,8 +30,8 @@
expecting a leak again
1,000 bytes in 1 blocks are definitely lost in loss record ... of ...
at 0x........: malloc (vg_replace_malloc.c:...)
- by 0x........: f (leak-segv-jmp.c:281)
- by 0x........: main (leak-segv-jmp.c:344)
+ by 0x........: f (leak-segv-jmp.c:295)
+ by 0x........: main (leak-segv-jmp.c:370)
LEAK SUMMARY:
definitely lost: 1,000 bytes in 1 blocks
@@ -46,8 +46,8 @@
expecting a leak again after full mprotect
1,000 bytes in 1 blocks are definitely lost in loss record ... of ...
at 0x........: malloc (vg_replace_malloc.c:...)
- by 0x........: f (leak-segv-jmp.c:281)
- by 0x........: main (leak-segv-jmp.c:344)
+ by 0x........: f (leak-segv-jmp.c:295)
+ by 0x........: main (leak-segv-jmp.c:370)
LEAK SUMMARY:
definitely lost: 1,000 bytes in 1 blocks
@@ -58,25 +58,46 @@
Reachable blocks (those to which a pointer was found) are not shown.
To see them, rerun with: --leak-check=full --show-leak-kinds=all
+mprotect result 0
+expecting heuristic not to crash after full mprotect
+1,000 bytes in 1 blocks are definitely lost in loss record ... of ...
+ at 0x........: malloc (vg_replace_malloc.c:...)
+ by 0x........: f (leak-segv-jmp.c:295)
+ by 0x........: main (leak-segv-jmp.c:370)
+
+40,000 bytes in 1 blocks are possibly lost in loss record ... of ...
+ at 0x........: calloc (vg_replace_malloc.c:...)
+ by 0x........: f (leak-segv-jmp.c:342)
+ by 0x........: main (leak-segv-jmp.c:370)
+
+LEAK SUMMARY:
+ definitely lost: 1,000 bytes in 1 blocks
+ indirectly lost: 0 bytes in 0 blocks
+ possibly lost: 40,000 bytes in 1 blocks
+ still reachable: 40,000 bytes in 1 blocks
+ suppressed: 0 bytes in 0 blocks
+Reachable blocks (those to which a pointer was found) are not shown.
+To see them, rerun with: --leak-check=full --show-leak-kinds=all
+
finished
LEAK SUMMARY:
definitely lost: 1,000 bytes in 1 blocks
indirectly lost: 0 bytes in 0 blocks
- possibly lost: 0 bytes in 0 blocks
+ possibly lost: 40,000 bytes in 1 blocks
still reachable: 40,000 bytes in 1 blocks
suppressed: 0 bytes in 0 blocks
Rerun with --leak-check=full to see details of leaked memory
leaked: 1000 bytes in 1 blocks
-dubious: 0 bytes in 0 blocks
+dubious: 40000 bytes in 1 blocks
reachable: 40000 bytes in 1 blocks
suppressed: 0 bytes in 0 blocks
HEAP SUMMARY:
- in use at exit: 41,000 bytes in 2 blocks
- total heap usage: 2 allocs, 0 frees, 41,000 bytes allocated
+ in use at exit: 81,000 bytes in 3 blocks
+ total heap usage: 3 allocs, 0 frees, 81,000 bytes allocated
For a detailed leak analysis, rerun with: --leak-check=full
For counts of detected and suppressed errors, rerun with: -v
-ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)
+ERROR SUMMARY: 5 errors from 5 contexts (suppressed: 0 from 0)
|
