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