Bug 1038854 - Synchronize mutexes in pthread_cond_wait after recreating threads to avoid race conditions. Parts by Ting-Yu, r=khuey. Parts by Kyle Huey, r=thinker

mozglue/build/Nuwa.cpp

@@ -140,7 +140,21 @@ struct thread_info : public mozilla::LinkedListElement<thread_info> {
 
   TLSInfoList tlsInfo;
 
-  pthread_mutex_t *reacquireMutex;
+  /**
+   * We must ensure that the recreated thread has entered pthread_cond_wait() or
+   * similar functions before proceeding to recreate the next one. Otherwise, if
+   * the next thread depends on the same mutex, it may be used in an incorrect
+   * state.  To do this, the main thread must unconditionally acquire the mutex.
+   * The mutex is unconditionally released when the recreated thread enters
+   * pthread_cond_wait().  The recreated thread may have locked the mutex itself
+   * (if the pthread_mutex_trylock succeeded) or another thread may have already
+   * held the lock.  If the recreated thread did lock the mutex we must balance
+   * that with another unlock on the main thread, which is signaled by
+   * condMutexNeedsBalancing.
+   */
+  pthread_mutex_t *condMutex;
+  bool condMutexNeedsBalancing;
+
   void *stk;
 
   pid_t origNativeThreadID;
@@ -501,7 +515,8 @@ thread_info_new(void) {
   tinfo->recrArg = nullptr;
   tinfo->recreatedThreadID = 0;
   tinfo->recreatedNativeThreadID = 0;
-  tinfo->reacquireMutex = nullptr;
+  tinfo->condMutex = nullptr;
+  tinfo->condMutexNeedsBalancing = false;
   tinfo->stk = MozTaggedAnonymousMmap(nullptr,
                                       NUWA_STACK_SIZE + getPageSize(),
                                       PROT_READ | PROT_WRITE,
@@ -1016,19 +1031,32 @@ __wrap_pthread_cond_wait(pthread_cond_t *cond,
     return rv;
   }
   if (recreated && mtx) {
-    if (!freezePoint1 && pthread_mutex_trylock(mtx)) {
+    if (!freezePoint1) {
+      tinfo->condMutex = mtx;
       // The thread was frozen in pthread_cond_wait() after releasing mtx in the
       // Nuwa process. In recreating this thread, We failed to reacquire mtx
       // with the pthread_mutex_trylock() call, that is, mtx was acquired by
       // another thread. Because of this, we need the main thread's help to
       // reacquire mtx so that it will be in a valid state.
-      tinfo->reacquireMutex = mtx;
+      if (!pthread_mutex_trylock(mtx)) {
+        tinfo->condMutexNeedsBalancing = true;
+      }
     }
     RECREATE_CONTINUE();
     RECREATE_PASS_VIP();
   }
   rv = REAL(pthread_cond_wait)(cond, mtx);
   if (recreated && mtx) {
+    // We have reacquired mtx. The main thread also wants to acquire mtx to
+    // synchronize with us. If the main thread didn't get a chance to acquire
+    // mtx let it do that now. The main thread clears condMutex after acquiring
+    // it to signal us.
+    if (tinfo->condMutex) {
+      // We need mtx to end up locked, so tell the main thread not to unlock
+      // mtx after it locks it.
+      tinfo->condMutexNeedsBalancing = false;
+      pthread_mutex_unlock(mtx);
+    }
     // We still need to be gated as not to acquire another mutex associated with
     // another VIP thread and interfere with it.
     RECREATE_GATE_VIP();
@@ -1052,14 +1080,21 @@ __wrap_pthread_cond_timedwait(pthread_cond_t *cond,
     return rv;
   }
   if (recreated && mtx) {
-    if (!freezePoint1 && pthread_mutex_trylock(mtx)) {
-      tinfo->reacquireMutex = mtx;
+    if (!freezePoint1) {
+      tinfo->condMutex = mtx;
+      if (!pthread_mutex_trylock(mtx)) {
+        tinfo->condMutexNeedsBalancing = true;
+      }
     }
     RECREATE_CONTINUE();
     RECREATE_PASS_VIP();
   }
   rv = REAL(pthread_cond_timedwait)(cond, mtx, abstime);
   if (recreated && mtx) {
+    if (tinfo->condMutex) {
+      tinfo->condMutexNeedsBalancing = false;
+      pthread_mutex_unlock(mtx);
+    }
     RECREATE_GATE_VIP();
   }
   THREAD_FREEZE_POINT2_VIP();
@@ -1087,14 +1122,21 @@ __wrap___pthread_cond_timedwait(pthread_cond_t *cond,
     return rv;
   }
   if (recreated && mtx) {
-    if (!freezePoint1 && pthread_mutex_trylock(mtx)) {
-      tinfo->reacquireMutex = mtx;
+    if (!freezePoint1) {
+      tinfo->condMutex = mtx;
+      if (!pthread_mutex_trylock(mtx)) {
+        tinfo->condMutexNeedsBalancing = true;
+      }
     }
     RECREATE_CONTINUE();
     RECREATE_PASS_VIP();
   }
   rv = REAL(__pthread_cond_timedwait)(cond, mtx, abstime, clock);
   if (recreated && mtx) {
+    if (tinfo->condMutex) {
+      tinfo->condMutexNeedsBalancing = false;
+      pthread_mutex_unlock(mtx);
+    }
     RECREATE_GATE_VIP();
   }
   THREAD_FREEZE_POINT2_VIP();
@@ -1403,8 +1445,17 @@ RecreateThreads() {
       RECREATE_BEFORE(tinfo);
       thread_recreate(tinfo);
       RECREATE_WAIT();
-      if (tinfo->reacquireMutex) {
-        REAL(pthread_mutex_lock)(tinfo->reacquireMutex);
+      if (tinfo->condMutex) {
+        // Synchronize with the recreated thread in pthread_cond_wait().
+        REAL(pthread_mutex_lock)(tinfo->condMutex);
+        // Tell the other thread that we have successfully locked the mutex.
+        // NB: condMutex can only be touched while it is held, so we must clear
+        // it here and store the mutex locally.
+        pthread_mutex_t *mtx = tinfo->condMutex;
+        tinfo->condMutex = nullptr;
+        if (tinfo->condMutexNeedsBalancing) {
+          pthread_mutex_unlock(mtx);
+        }
       }
     } else if(!(tinfo->flags & TINFO_FLAG_NUWA_SKIP)) {
       // An unmarked thread is found other than the main thread.