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

@@ -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

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);
       }
    }

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);
 }

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);

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 );
 

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,

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
 

memcheck/tests/reach_thread_register.c

@@ -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;
+}

memcheck/tests/reach_thread_register.stderr.exp

@@ -0,0 +1 @@
+Abandoning the helper.

memcheck/tests/reach_thread_register.vgtest

@@ -0,0 +1,2 @@
+prog: reach_thread_register
+vgopts: -q --leak-check=full --show-leak-kinds=definite