|
From: Jeremy F. <je...@go...> - 2005-01-15 02:07:49
|
CVS commit by fitzhardinge:
Fix up a nasty little race on thread exit. After the thread sets the
status to Empty and releases run_sema, it could get recycled. Since
there's a little bit of code which needs to run after unlocking,
there's a chance that it could get its stack stolen from under it.
The solution it to use a temporary Zombie state, which prevents the
thread structure from being reallocated. The thread exit code then
sets the state to Empty and exits without touching the stack in the
meantime.
M +2 -1 core.h 1.60
M +2 -0 vg_main.c 1.233
M +15 -4 vg_scheduler.c 1.208
M +0 -6 linux/core_os.c 1.4
M +15 -9 x86-linux/syscalls.c 1.11
--- valgrind/coregrind/core.h #1.59:1.60
@@ -604,5 +604,5 @@ typedef struct
Empty -> Init -> Runnable <=> WaitSys/Yielding
^ |
- \-----------------/
+ \---- Zombie -----/
*/
typedef
@@ -613,4 +613,5 @@ typedef
VgTs_WaitSys, /* waiting for a syscall to complete */
VgTs_Yielding, /* temporarily yielding the CPU */
+ VgTs_Zombie, /* transient state just before exiting */
}
ThreadStatus;
--- valgrind/coregrind/vg_scheduler.c #1.207:1.208
@@ -212,4 +212,5 @@ const Char* name_of_thread_state ( Threa
case VgTs_WaitSys: return "VgTs_WaitSys";
case VgTs_Yielding: return "VgTs_Yielding";
+ case VgTs_Zombie: return "VgTs_Zombie";
default: return "VgTs_???";
}
@@ -321,5 +322,6 @@ Int VG_(count_living_threads)(void)
for(tid = 1; tid < VG_N_THREADS; tid++)
- if (VG_(threads)[tid].status != VgTs_Empty)
+ if (VG_(threads)[tid].status != VgTs_Empty &&
+ VG_(threads)[tid].status != VgTs_Zombie)
count++;
@@ -367,5 +369,7 @@ inline Bool VG_(is_exiting)(ThreadId tid
}
-/* Mark the the ThreadState as unused and release the semaphore */
+/* Clear out the ThreadState and release the semaphore. Leaves the
+ ThreadState in VgTs_Zombie state, so that it doesn't get
+ reallocated until the caller is really ready. */
void VG_(exit_thread)(ThreadId tid)
{
@@ -404,5 +408,7 @@ void VG_(kill_thread)(ThreadId tid)
if (VG_(threads)[tid].status == VgTs_WaitSys) {
- //VG_(printf)("kill_thread zaps tid %d lwp %d\n", tid, VG_(threads)[tid].os_state.lwpid);
+ if (VG_(clo_trace_signals))
+ VG_(message)(Vg_DebugMsg, "kill_thread zaps tid %d lwp %d",
+ tid, VG_(threads)[tid].os_state.lwpid);
VG_(tkill)(VG_(threads)[tid].os_state.lwpid, VKI_SIGVGKILL);
}
@@ -546,5 +552,9 @@ void mostly_clear_thread_record ( Thread
VGA_(clear_thread)(&VG_(threads)[tid].arch);
VG_(threads)[tid].tid = tid;
- VG_(threads)[tid].status = VgTs_Empty;
+
+ /* Leave the thread in Zombie, so that it doesn't get reallocated
+ until the caller is finally done with the thread stack. */
+ VG_(threads)[tid].status = VgTs_Zombie;
+
VG_(sigemptyset)(&VG_(threads)[tid].sig_mask);
@@ -608,4 +618,5 @@ void VG_(scheduler_init) ( void )
mostly_clear_thread_record(i);
+ VG_(threads)[i].status = VgTs_Empty;
VG_(threads)[i].stack_size = 0;
VG_(threads)[i].stack_base = (Addr)NULL;
--- valgrind/coregrind/vg_main.c #1.232:1.233
@@ -2840,4 +2840,6 @@ void VG_(shutdown_actions)(ThreadId tid)
vg_assert(VG_(count_living_threads)() == 0);
+ VG_(threads)[tid].status = VgTs_Empty;
+
//--------------------------------------------------------------
// Finalisation: cleanup, messages, etc. Order no so important, only
--- valgrind/coregrind/x86-linux/syscalls.c #1.10:1.11
@@ -180,17 +180,23 @@ static Int start_thread(void *arg)
VG_(exit_thread)(tid);
- /* XXX There's a nasty little race here. Once we release the lock,
- in principle this thread structure could be reused for a new
- thread, recycling the stack we're actually using. The only way
- to fix this would be to release the lock, poke the reaper and
- exit without using any stack...
- Fortunately this is quite a small race.
- */
+ vg_assert(tst->status == VgTs_Zombie);
/* Poke the reaper */
VG_(tkill)(VG_(threads)[VG_(master_tid)].os_state.lwpid, VKI_SIGVGCHLD);
- /* bye-bye */
- return tst->os_state.exitcode;
+ /* We have to use this sequence to terminate the thread to prevent
+ a subtle race. If VG_(exit_thread)() had left the ThreadState
+ as Empty, then it could have been reallocated, reusing the stack
+ while we're doing these last cleanups. Instead,
+ VG_(exit_thread) leaves it as Zombie to prevent reallocation.
+ We need to make sure we don't touch the stack between marking it
+ Empty and exiting. Hence the assembler. */
+ asm volatile (
+ "movl %1, %0\n" /* set tst->status = VgTs_Empty */
+ "int $0x80\n" /* exit(tst->os_state.exitcode) */
+ : "=m" (tst->status)
+ : "n" (VgTs_Empty), "a" (__NR_exit), "b" (tst->os_state.exitcode));
+
+ VG_(core_panic)("Thread exit failed?\n");
}
--- valgrind/coregrind/linux/core_os.c #1.3:1.4
@@ -56,10 +56,4 @@ void VGA_(thread_wrapper)(ThreadId tid)
VG_(shutdown_actions)(tid);
VGA_(terminate)(tid, ret);
- } else {
- /* OK, disappear, but tell the reaper */
- vg_assert(tst->status == VgTs_Runnable);
- VG_(exit_thread)(tid);
- VG_(tkill)(VG_(threads)[VG_(master_tid)].os_state.lwpid, VKI_SIGVGCHLD);
- VG_(exit_single)(tst->os_state.exitcode);
}
}
|