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
diff --git a/include/utils/threads.h b/include/utils/threads.h
index 1bcfaed..41e5766 100644
--- a/include/utils/threads.h
+++ b/include/utils/threads.h
@@ -527,9 +527,10 @@
static int _threadLoop(void* user);
const bool mCanCallJava;
thread_id_t mThread;
- Mutex mLock;
+ mutable Mutex mLock;
Condition mThreadExitedCondition;
status_t mStatus;
+ // note that all accesses of mExitPending and mRunning need to hold mLock
volatile bool mExitPending;
volatile bool mRunning;
sp<Thread> mHoldSelf;
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;
}