commit 966a48f3e78e75c3a4f3a2f23b98e970f06bf983
author Glenn Kasten <gkasten@google.com> Tue Feb 01 11:32:29 2011 -0800
committer Alex Ray <aray@google.com> Tue Jul 30 13:56:55 2013 -0700
tree 991c30600b750da2568dc4255bb2692e79d2748d
parent 84a23fa4a9a2dca0f53fae2283b57fff988d1c74

Bug 3362814 Fix SMP race in access to mRequestExit

Also fix an unlikely SMP race in access to mHoldSelf on entry to _threadLoop.

Change-Id: I6cbc0b94739c7dd5e77e8a5ba0da22cdc0b1a4db

libs/utils/Threads.cpp

diff --git a/libs/utils/Threads.cpp b/libs/utils/Threads.cpp
index 35dcbcb..8b5da0e 100644
--- a/libs/utils/Threads.cpp
+++ b/libs/utils/Threads.cpp
@@ -675,6 +675,9 @@
         mLock("Thread::mLock"),
         mStatus(NO_ERROR),
         mExitPending(false), mRunning(false)
+#ifdef HAVE_ANDROID_OS
+        , mTid(-1)
+#endif
 {
 }
 
@@ -715,6 +718,7 @@
         res = androidCreateRawThreadEtc(_threadLoop,
                 this, name, priority, stack, &mThread);
     }
+    // The new thread wakes up at _threadLoop, but immediately blocks on mLock
     
     if (res == false) {
         mStatus = UNKNOWN_ERROR;   // something happened!
@@ -730,11 +734,19 @@
     // here merely indicates successfully starting the thread and does not
     // imply successful termination/execution.
     return NO_ERROR;
+
+    // Exiting scope of mLock is a memory barrier and allows new thread to run
 }
 
 int Thread::_threadLoop(void* user)
 {
     Thread* const self = static_cast<Thread*>(user);
+
+    // force a memory barrier before reading any fields, in particular mHoldSelf
+    {
+    Mutex::Autolock _l(self->mLock);
+    }
+
     sp<Thread> strong(self->mHoldSelf);
     wp<Thread> weak(strong);
     self->mHoldSelf.clear();
@@ -753,7 +765,7 @@
             self->mStatus = self->readyToRun();
             result = (self->mStatus == NO_ERROR);
 
-            if (result && !self->mExitPending) {
+            if (result && !self->exitPending()) {
                 // Binder threads (and maybe others) rely on threadLoop
                 // running at least once after a successful ::readyToRun()
                 // (unless, of course, the thread has already been asked to exit
@@ -770,18 +782,21 @@
             result = self->threadLoop();
         }
 
+        // establish a scope for mLock
+        {
+        Mutex::Autolock _l(self->mLock);
         if (result == false || self->mExitPending) {
             self->mExitPending = true;
-            self->mLock.lock();
             self->mRunning = false;
             // clear thread ID so that requestExitAndWait() does not exit if
             // called by a new thread using the same thread ID as this one.
             self->mThread = thread_id_t(-1);
+            // note that interested observers blocked in requestExitAndWait are
+            // awoken by broadcast, but blocked on mLock until break exits scope
             self->mThreadExitedCondition.broadcast();
-            self->mThread = thread_id_t(-1); // thread id could be reused
-            self->mLock.unlock();
             break;
         }
+        }
         
         // Release our strong reference, to let a chance to the thread
         // to die a peaceful death.
@@ -795,6 +810,7 @@
 
 void Thread::requestExit()
 {
+    Mutex::Autolock _l(mLock);
     mExitPending = true;
 }
 
@@ -815,6 +831,8 @@
     while (mRunning == true) {
         mThreadExitedCondition.wait(mLock);
     }
+    // This next line is probably not needed any more, but is being left for
+    // historical reference. Note that each interested party will clear flag.
     mExitPending = false;
 
     return mStatus;
@@ -822,6 +840,7 @@
 
 bool Thread::exitPending() const
 {
+    Mutex::Autolock _l(mLock);
     return mExitPending;
 }