From: Philippe Waroquiers Date: Thu, 22 Oct 2015 19:14:30 +0000 (+0000) Subject: Fix 353891 Assert 'bad_scanned_addr < VG_ROUNDDN(start+len, sizeof(Addr))' failed X-Git-Tag: svn/VALGRIND_3_12_0~307 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6b5a479bda4f033c88882b79e4a0c6bbf44c9750;p=thirdparty%2Fvalgrind.git 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. git-svn-id: svn://svn.valgrind.org/valgrind/trunk@15716 --- diff --git a/NEWS b/NEWS index 2a0d810003..b4827452e4 100644 --- a/NEWS +++ b/NEWS @@ -36,6 +36,7 @@ where XXXXXX is the bug number as listed below. 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 diff --git a/coregrind/m_signals.c b/coregrind/m_signals.c index b255ce5378..db756cb36c 100644 --- a/coregrind/m_signals.c +++ b/coregrind/m_signals.c @@ -2538,16 +2538,13 @@ Bool VG_(extend_stack)(ThreadId tid, Addr addr) 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 diff --git a/coregrind/pub_core_libcsignal.h b/coregrind/pub_core_libcsignal.h index 8da6fed76e..8f45a46804 100644 --- a/coregrind/pub_core_libcsignal.h +++ b/coregrind/pub_core_libcsignal.h @@ -52,7 +52,7 @@ extern Bool VG_(iseqsigset) ( const vki_sigset_t* set1, 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 ); diff --git a/include/pub_tool_libcsignal.h b/include/pub_tool_libcsignal.h index 5985af6b0c..88770d830f 100644 --- a/include/pub_tool_libcsignal.h +++ b/include/pub_tool_libcsignal.h @@ -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 ); diff --git a/include/pub_tool_signals.h b/include/pub_tool_signals.h index c346504b05..1bab4787db 100644 --- a/include/pub_tool_signals.h +++ b/include/pub_tool_signals.h @@ -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 diff --git a/memcheck/mc_leakcheck.c b/memcheck/mc_leakcheck.c index e39302291f..d1d1f2398f 100644 --- a/memcheck/mc_leakcheck.c +++ b/memcheck/mc_leakcheck.c @@ -683,6 +683,65 @@ static Bool is_valid_aligned_ULong ( Addr a ) && 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 @@ static LeakCheckHeuristic heuristic_reachedness (Addr ptr, 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 @@ static LeakCheckHeuristic heuristic_reachedness (Addr ptr, // ??? 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 @@ static LeakCheckHeuristic heuristic_reachedness (Addr ptr, && 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 @@ static LeakCheckHeuristic heuristic_reachedness (Addr ptr, 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 @@ static LeakCheckHeuristic heuristic_reachedness (Addr ptr, && 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 @@ lc_push_if_a_chunk_ptr(Addr ptr, } -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 @@ lc_scan_memory(Addr start, SizeT len, Bool is_prior_definite, #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 @@ lc_scan_memory(Addr start, SizeT len, Bool is_prior_definite, 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 @@ lc_scan_memory(Addr start, SizeT len, Bool is_prior_definite, 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 @@ void MC_(detect_memory_leaks) ( ThreadId tid, LeakCheckParams* lcp) tl_assert(ex->state == Unreached); } } - + print_results( tid, lcp); VG_(free) ( lc_markstack ); diff --git a/memcheck/tests/leak-segv-jmp.c b/memcheck/tests/leak-segv-jmp.c index f3380e910c..1d1f84ca74 100644 --- a/memcheck/tests/leak-segv-jmp.c +++ b/memcheck/tests/leak-segv-jmp.c @@ -251,6 +251,7 @@ UWord do_syscall_WRK (UWord syscall_no, 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 @@ static void non_simd_mprotect (long tid, void* addr, long len) #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 @@ void f(void) 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 @@ void f(void) 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"); } diff --git a/memcheck/tests/leak-segv-jmp.stderr.exp b/memcheck/tests/leak-segv-jmp.stderr.exp index ba51e4d698..1ee37cdf80 100644 --- a/memcheck/tests/leak-segv-jmp.stderr.exp +++ b/memcheck/tests/leak-segv-jmp.stderr.exp @@ -14,8 +14,8 @@ To see them, rerun with: --leak-check=full --show-leak-kinds=all 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 @@ mprotect result 0 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 @@ full mprotect result 0 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 @@ LEAK SUMMARY: 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)