From: Philippe Waroquiers Date: Mon, 21 Oct 2013 19:57:08 +0000 (+0000) Subject: Fix 324227 memcheck false positive leak when a thread calls exit+block X-Git-Tag: svn/VALGRIND_3_9_0~21 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=81d7bfdddec1c32a702aff4a461368aa9844098f;p=thirdparty%2Fvalgrind.git 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 --- diff --git a/NEWS b/NEWS index 06e4db4e9b..dab8fb3805 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 6c8aa8ffb6..28a180e227 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 e50665e9bb..e91b2a90f5 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 46bf289bf7..c20000288d 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 64a08a7b73..7a9611b99d 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 d5d9ab0cb1..410d3473f6 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 7614e89857..6753dcb84f 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 0000000000..2128130d51 --- /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 0000000000..fbd8a42b54 --- /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 0000000000..1d04b03a17 --- /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