|
From: <sv...@va...> - 2013-10-21 19:57:21
|
Author: philippe
Date: Mon Oct 21 19:57:08 2013
New Revision: 13670
Log:
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.
Added:
trunk/memcheck/tests/reach_thread_register.c
trunk/memcheck/tests/reach_thread_register.stderr.exp
trunk/memcheck/tests/reach_thread_register.vgtest
Modified:
trunk/NEWS
trunk/coregrind/m_machine.c
trunk/coregrind/m_syswrap/syswrap-linux.c
trunk/coregrind/m_threadstate.c
trunk/coregrind/pub_core_threadstate.h
trunk/include/pub_tool_machine.h
trunk/memcheck/tests/Makefile.am
Modified: trunk/NEWS
==============================================================================
--- trunk/NEWS (original)
+++ trunk/NEWS Mon Oct 21 19:57:08 2013
@@ -690,6 +690,10 @@
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
Modified: trunk/coregrind/m_machine.c
==============================================================================
--- trunk/coregrind/m_machine.c (original)
+++ trunk/coregrind/m_machine.c Mon Oct 21 19:57:08 2013
@@ -212,6 +212,7 @@
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 @@
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);
}
}
Modified: trunk/coregrind/m_syswrap/syswrap-linux.c
==============================================================================
--- trunk/coregrind/m_syswrap/syswrap-linux.c (original)
+++ trunk/coregrind/m_syswrap/syswrap-linux.c Mon Oct 21 19:57:08 2013
@@ -108,8 +108,8 @@
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 @@
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_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 @@
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);
}
Modified: trunk/coregrind/m_threadstate.c
==============================================================================
--- trunk/coregrind/m_threadstate.c (original)
+++ trunk/coregrind/m_threadstate.c Mon Oct 21 19:57:08 2013
@@ -78,6 +78,17 @@
}
}
+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);
Modified: trunk/coregrind/pub_core_threadstate.h
==============================================================================
--- trunk/coregrind/pub_core_threadstate.h (original)
+++ trunk/coregrind/pub_core_threadstate.h Mon Oct 21 19:57:08 2013
@@ -70,7 +70,8 @@
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 @@
// 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 );
Modified: trunk/include/pub_tool_machine.h
==============================================================================
--- trunk/include/pub_tool_machine.h (original)
+++ trunk/include/pub_tool_machine.h Mon Oct 21 19:57:08 2013
@@ -133,7 +133,8 @@
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,
Modified: trunk/memcheck/tests/Makefile.am
==============================================================================
--- trunk/memcheck/tests/Makefile.am (original)
+++ trunk/memcheck/tests/Makefile.am Mon Oct 21 19:57:08 2013
@@ -197,6 +197,7 @@
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 @@
partial_load pdb-realloc pdb-realloc2 \
pipe pointer-trace \
post-syscall \
+ reach_thread_register \
realloc1 realloc2 realloc3 \
recursive-merge \
sbfragment \
@@ -379,6 +381,8 @@
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
Added: trunk/memcheck/tests/reach_thread_register.c
==============================================================================
--- trunk/memcheck/tests/reach_thread_register.c (added)
+++ trunk/memcheck/tests/reach_thread_register.c Mon Oct 21 19:57:08 2013
@@ -0,0 +1,51 @@
+#include <pthread.h>
+#include <sched.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <config.h>
+
+/* 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;
+}
Added: trunk/memcheck/tests/reach_thread_register.stderr.exp
==============================================================================
--- trunk/memcheck/tests/reach_thread_register.stderr.exp (added)
+++ trunk/memcheck/tests/reach_thread_register.stderr.exp Mon Oct 21 19:57:08 2013
@@ -0,0 +1 @@
+Abandoning the helper.
Added: trunk/memcheck/tests/reach_thread_register.vgtest
==============================================================================
--- trunk/memcheck/tests/reach_thread_register.vgtest (added)
+++ trunk/memcheck/tests/reach_thread_register.vgtest Mon Oct 21 19:57:08 2013
@@ -0,0 +1,2 @@
+prog: reach_thread_register
+vgopts: -q --leak-check=full --show-leak-kinds=definite
|