From 81d7bfdddec1c32a702aff4a461368aa9844098f Mon Sep 17 00:00:00 2001 From: Philippe Waroquiers Date: Mon, 21 Oct 2013 19:57:08 +0000 Subject: [PATCH] Fix 324227 memcheck false positive leak when a thread calls exit+block only reachable via other thread live register The exiting thread will have its registers considered as not reachable anymore, registers of other threads will be considered reachable. This is ensured by using a different exit reason for the exiting thread and for the other threads. git-svn-id: svn://svn.valgrind.org/valgrind/trunk@13670 --- NEWS | 4 ++ coregrind/m_machine.c | 6 +- coregrind/m_syswrap/syswrap-linux.c | 55 ++++++++++++++++--- coregrind/m_threadstate.c | 11 ++++ coregrind/pub_core_threadstate.h | 6 +- include/pub_tool_machine.h | 3 +- memcheck/tests/Makefile.am | 4 ++ memcheck/tests/reach_thread_register.c | 51 +++++++++++++++++ .../tests/reach_thread_register.stderr.exp | 1 + memcheck/tests/reach_thread_register.vgtest | 2 + 10 files changed, 131 insertions(+), 12 deletions(-) create mode 100644 memcheck/tests/reach_thread_register.c create mode 100644 memcheck/tests/reach_thread_register.stderr.exp create mode 100644 memcheck/tests/reach_thread_register.vgtest diff --git a/NEWS b/NEWS index 06e4db4e9..dab8fb380 100644 --- a/NEWS +++ b/NEWS @@ -690,6 +690,10 @@ DONE 322563 vex mips->IR: 0x70 0x83 0xF0 0x3A FIXED 13558 2765 +324227 memcheck false positive leak when a thread calls exit+block + only reachable via other thread live register + FIXED + 326091 drd: Avoid that optimized strlen() implementations trigger false positive race reports. FIXED 13664 diff --git a/coregrind/m_machine.c b/coregrind/m_machine.c index 6c8aa8ffb..28a180e22 100644 --- a/coregrind/m_machine.c +++ b/coregrind/m_machine.c @@ -212,6 +212,7 @@ static void apply_to_GPs_of_tid(ThreadId tid, void (*f)(ThreadId, const HChar*, Addr)) { VexGuestArchState* vex = &(VG_(get_ThreadState)(tid)->arch.vex); + VG_(debugLog)(2, "machine", "apply_to_GPs_of_tid %d\n", tid); #if defined(VGA_x86) (*f)(tid, "EAX", vex->guest_EAX); (*f)(tid, "ECX", vex->guest_ECX); @@ -349,7 +350,10 @@ void VG_(apply_to_GP_regs)(void (*f)(ThreadId, const HChar*, UWord)) ThreadId tid; for (tid = 1; tid < VG_N_THREADS; tid++) { - if (VG_(is_valid_tid)(tid)) { + if (VG_(is_valid_tid)(tid) + || VG_(threads)[tid].exitreason == VgSrc_ExitProcess) { + // live thread or thread instructed to die by another thread that + // called exit. apply_to_GPs_of_tid(tid, f); } } diff --git a/coregrind/m_syswrap/syswrap-linux.c b/coregrind/m_syswrap/syswrap-linux.c index e50665e9b..e91b2a90f 100644 --- a/coregrind/m_syswrap/syswrap-linux.c +++ b/coregrind/m_syswrap/syswrap-linux.c @@ -108,8 +108,8 @@ static VgSchedReturnCode thread_wrapper(Word /*ThreadId*/ tidW) vg_assert(VG_(is_running_thread)(tid)); VG_(debugLog)(1, "syswrap-linux", - "thread_wrapper(tid=%lld): exit\n", - (ULong)tidW); + "thread_wrapper(tid=%lld): exit, schedreturncode %s\n", + (ULong)tidW, VG_(name_of_VgSchedReturnCode)(ret)); /* Return to caller, still holding the lock. */ return ret; @@ -197,7 +197,6 @@ static void run_a_thread_NORETURN ( Word tidW ) carry on to show final tool results, then exit the entire system. Use the continuation pointer set at startup in m_main. */ ( * VG_(address_of_m_main_shutdown_actions_NORETURN) ) (tid, src); - } else { VG_(debugLog)(1, "syswrap-linux", @@ -689,9 +688,20 @@ PRE(sys_exit_group) PRE_REG_READ1(void, "exit_group", int, status); tst = VG_(get_ThreadState)(tid); - /* A little complex; find all the threads with the same threadgroup as this one (including this one), and mark them to exit */ + /* It is unclear how one can get a threadgroup in this process which + is not the threadgroup of the calling thread: + The assignments to threadgroups are: + = 0; /// scheduler.c os_state_clear + = getpid(); /// scheduler.c in child after fork + = getpid(); /// this file, in thread_wrapper + = ptst->os_state.threadgroup; /// syswrap-*-linux.c, + copying the thread group of the thread doing clone + So, the only case where the threadgroup might be different to the getpid + value is in the child, just after fork. But then the fork syscall is + still going on, the forked thread has had no chance yet to make this + syscall. */ for (t = 1; t < VG_N_THREADS; t++) { if ( /* not alive */ VG_(threads)[t].status == VgTs_Empty @@ -700,14 +710,41 @@ PRE(sys_exit_group) VG_(threads)[t].os_state.threadgroup != tst->os_state.threadgroup ) continue; - - VG_(threads)[t].exitreason = VgSrc_ExitThread; + /* Assign the exit code, VG_(nuke_all_threads_except) will assign + the exitreason. */ VG_(threads)[t].os_state.exitcode = ARG1; - - if (t != tid) - VG_(get_thread_out_of_syscall)(t); /* unblock it, if blocked */ } + /* Indicate in all other threads that the process is exiting. + Then wait using VG_(reap_threads) for these threads to disappear. + + Can this give a deadlock if another thread is calling exit in parallel + and would then wait for this thread to disappear ? + The answer is no: + Other threads are either blocked in a syscall or have yielded the CPU. + + A thread that has yielded the CPU is trying to get the big lock in + VG_(scheduler). This thread will get the CPU thanks to the call + to VG_(reap_threads). The scheduler will then check for signals, + kill the process if this is a fatal signal, and otherwise prepare + the thread for handling this signal. After this preparation, if + the thread status is VG_(is_exiting), the scheduler exits the thread. + So, a thread that has yielded the CPU does not have a chance to + call exit => no deadlock for this thread. + + VG_(nuke_all_threads_except) will send the VG_SIGVGKILL signal + to all threads blocked in a syscall. + The syscall will be interrupted, and the control will go to the + scheduler. The scheduler will then return, as the thread is in + exiting state. */ + + VG_(nuke_all_threads_except)( tid, VgSrc_ExitProcess ); + VG_(reap_threads)(tid); + VG_(threads)[tid].exitreason = VgSrc_ExitThread; + /* we do assign VgSrc_ExitThread and not VgSrc_ExitProcess, as this thread + is the thread calling exit_group and so its registers must be considered + as not reachable. See pub_tool_machine.h VG_(apply_to_GP_regs). */ + /* We have to claim the syscall already succeeded. */ SET_STATUS_Success(0); } diff --git a/coregrind/m_threadstate.c b/coregrind/m_threadstate.c index 46bf289bf..c20000288 100644 --- a/coregrind/m_threadstate.c +++ b/coregrind/m_threadstate.c @@ -78,6 +78,17 @@ const HChar* VG_(name_of_ThreadStatus) ( ThreadStatus status ) } } +const HChar* VG_(name_of_VgSchedReturnCode) ( VgSchedReturnCode retcode ) +{ + switch (retcode) { + case VgSrc_None: return "VgSrc_None"; + case VgSrc_ExitThread: return "VgSrc_ExitThread"; + case VgSrc_ExitProcess: return "VgSrc_ExitProcess"; + case VgSrc_FatalSig: return "VgSrc_FatalSig"; + default: return "VgSrc_???"; + } +} + ThreadState *VG_(get_ThreadState)(ThreadId tid) { vg_assert(tid >= 0 && tid < VG_N_THREADS); diff --git a/coregrind/pub_core_threadstate.h b/coregrind/pub_core_threadstate.h index 64a08a7b7..7a9611b99 100644 --- a/coregrind/pub_core_threadstate.h +++ b/coregrind/pub_core_threadstate.h @@ -70,7 +70,8 @@ typedef enum { VgSrc_None, /* not exiting yet */ VgSrc_ExitThread, /* just this thread is exiting */ - VgSrc_ExitProcess, /* entire process is exiting */ + VgSrc_ExitProcess, /* this thread is exiting due to another thread + calling exit() */ VgSrc_FatalSig /* Killed by the default action of a fatal signal */ } @@ -388,6 +389,9 @@ void VG_(init_Threads)(void); // Convert a ThreadStatus to a string. const HChar* VG_(name_of_ThreadStatus) ( ThreadStatus status ); +// Convert a VgSchedReturnCode to a string. +const HChar* VG_(name_of_VgSchedReturnCode) ( VgSchedReturnCode retcode ); + /* Get the ThreadState for a particular thread */ extern ThreadState *VG_(get_ThreadState) ( ThreadId tid ); diff --git a/include/pub_tool_machine.h b/include/pub_tool_machine.h index d5d9ab0cb..410d3473f 100644 --- a/include/pub_tool_machine.h +++ b/include/pub_tool_machine.h @@ -133,7 +133,8 @@ void VG_(set_syscall_return_shadows) ( ThreadId tid, UWord s1err, UWord s2err ); // Apply a function 'f' to all the general purpose registers in all the -// current threads. +// current threads. This is all live threads, or (when the process is exiting) +// all threads that were instructed to die by the thread calling exit. // This is very Memcheck-specific -- it's used to find the roots when // doing leak checking. extern void VG_(apply_to_GP_regs)(void (*f)(ThreadId tid, diff --git a/memcheck/tests/Makefile.am b/memcheck/tests/Makefile.am index 7614e8985..6753dcb84 100644 --- a/memcheck/tests/Makefile.am +++ b/memcheck/tests/Makefile.am @@ -197,6 +197,7 @@ EXTRA_DIST = \ pointer-trace.vgtest \ pointer-trace.stderr.exp \ post-syscall.stderr.exp post-syscall.vgtest \ + reach_thread_register.stderr.exp reach_thread_register.vgtest \ realloc1.stderr.exp realloc1.vgtest \ realloc2.stderr.exp realloc2.vgtest \ realloc3.stderr.exp realloc3.vgtest \ @@ -318,6 +319,7 @@ check_PROGRAMS = \ partial_load pdb-realloc pdb-realloc2 \ pipe pointer-trace \ post-syscall \ + reach_thread_register \ realloc1 realloc2 realloc3 \ recursive-merge \ sbfragment \ @@ -379,6 +381,8 @@ dw4_CFLAGS = $(AM_CFLAGS) -gdwarf-4 -fdebug-types-section err_disable3_LDADD = -lpthread err_disable4_LDADD = -lpthread +reach_thread_register_CFLAGS = $(AM_CFLAGS) -O2 +reach_thread_register_LDADD = -lpthread thread_alloca_LDADD = -lpthread threadname_LDADD = -lpthread diff --git a/memcheck/tests/reach_thread_register.c b/memcheck/tests/reach_thread_register.c new file mode 100644 index 000000000..2128130d5 --- /dev/null +++ b/memcheck/tests/reach_thread_register.c @@ -0,0 +1,51 @@ +#include +#include +#include +#include +#include +#include + +/* test based on code from Alexander Potapenko, slightly modified. */ +/* Reproduces a false positive leak when a pointer is (only) in a live + thread register, and another thread calls exit */ + +pthread_mutex_t mu = PTHREAD_MUTEX_INITIALIZER; +int cont = 1; + +void* helper(void* v_bar) { + pthread_barrier_t* bar = (pthread_barrier_t*)v_bar; + register int* i = malloc(sizeof(*i)); + // Try hard to have i allocated in a register. + *i = 3; + pthread_barrier_wait(bar); + pthread_mutex_lock(&mu); + while (cont) { +#if defined(VGA_x86) || defined(VGA_amd64) + // Below helps to have i really in a register. + asm volatile("test %0, %0" : : "S"(i)); +#else + // Not clear that for other archs, i is in a register. + if (*i) // should do better for other archs. + // "then" part after the #endif +#endif + pthread_mutex_unlock(&mu); + sched_yield(); + pthread_mutex_lock(&mu); + } + pthread_mutex_unlock(&mu); + free((void *)i); + fprintf(stderr, "Quitting the helper.\n"); + return NULL; +} + +int main() { + pthread_barrier_t bar; + pthread_barrier_init(&bar, NULL, 2); + pthread_t thr; + pthread_create(&thr, NULL, &helper, &bar); + pthread_barrier_wait(&bar); + pthread_barrier_destroy(&bar); + fprintf(stderr, "Abandoning the helper.\n"); + pthread_detach(thr); + return 0; +} diff --git a/memcheck/tests/reach_thread_register.stderr.exp b/memcheck/tests/reach_thread_register.stderr.exp new file mode 100644 index 000000000..fbd8a42b5 --- /dev/null +++ b/memcheck/tests/reach_thread_register.stderr.exp @@ -0,0 +1 @@ +Abandoning the helper. diff --git a/memcheck/tests/reach_thread_register.vgtest b/memcheck/tests/reach_thread_register.vgtest new file mode 100644 index 000000000..1d04b03a1 --- /dev/null +++ b/memcheck/tests/reach_thread_register.vgtest @@ -0,0 +1,2 @@ +prog: reach_thread_register +vgopts: -q --leak-check=full --show-leak-kinds=definite