Change lock order expecation between StaticMeta.lock_ and ThreadEntry…

…Set lock.

Summary:
The dispose() of a thread local copy of an object is called when a
thread does a reset() on it. It can invoke arbitrary code including calling
destroy() or reset() on other ThreadLocals. Such a call will need to acquire
the StaticMeta's lock_. To enforce that lock order, we need to fix paths that
today acquire a ThreadEntrySet lock after acquiring StaticMeta's lock_.

Reviewed By: yfeldblum

Differential Revision: D59076052

fbshipit-source-id: 0109d66b3a5f74d7460b8a2d27103f2e919677e9

folly/ThreadLocal.h

@@ -196,7 +196,6 @@ class ThreadLocalPtr {
       typename = typename std::enable_if<
           std::is_convertible<SourceT*, T*>::value>::type>
   void reset(std::unique_ptr<SourceT, Deleter> source) {
-    auto rlocked = getForkGuard();
     auto deleter = [delegate = source.get_deleter()](
                        T* ptr, TLPDestructionMode) { delegate(ptr); };
     reset(source.release(), deleter);
@@ -230,6 +229,7 @@ class ThreadLocalPtr {
       }
     });
 
+    auto rlocked = getForkGuard();
     threadlocal_detail::ThreadEntry* te = StaticMeta::getThreadEntry(&id_);
     uint32_t id = id_.getOrInvalid();
     // Only valid index into the the elements array
@@ -427,9 +427,9 @@ class ThreadLocalPtr {
           lock_(&meta_.lock_) {
       forkHandlerLock_->lock_shared();
       accessAllThreadsLock_->lock();
-      lock_->lock();
       id_ = id;
       wlockedThreadEntrySet_ = meta_.allId2ThreadEntrySets_[id_].wlock();
+      lock_->lock();
     }
 
     void release() {

folly/detail/ThreadLocalDetail.cpp

@@ -114,16 +114,19 @@ void StaticMetaBase::onThreadExit(void* ptr) {
     // Make sure this ThreadEntry is available if ThreadLocal A is accessed in
     // ThreadLocal B destructor.
     pthread_setspecific(meta.pthreadKey_, threadEntry);
+
+    std::shared_lock forkRlock(meta.forkHandlerLock_);
     std::shared_lock rlock(meta.accessAllThreadsLock_, std::defer_lock);
     if (meta.strict_) {
       rlock.lock();
     }
+    meta.removeThreadEntryFromAllInMap(threadEntry);
+    forkRlock.unlock();
     {
       std::lock_guard<std::mutex> g(meta.lock_);
       // mark it as removed
       threadEntry->removed_ = true;
       auto elementsCapacity = threadEntry->getElementsCapacity();
-      meta.removeThreadEntryFromAllInMap(threadEntry);
       auto beforeCount = meta.totalElementWrappers_.fetch_sub(elementsCapacity);
       DCHECK_GE(beforeCount, elementsCapacity);
       // No need to hold the lock any longer; the ThreadEntry is private to this
@@ -142,7 +145,7 @@ void StaticMetaBase::onThreadExit(void* ptr) {
           shouldRun = true;
         }
       }
-      DCHECK(threadEntry->meta->isThreadEntryRemovedFromAllInMap(threadEntry));
+      DCHECK(meta.isThreadEntryRemovedFromAllInMap(threadEntry, !meta.strict_));
     }
     pthread_setspecific(meta.pthreadKey_, nullptr);
   }
@@ -203,7 +206,7 @@ void StaticMetaBase::cleanupThreadEntriesAndList(
 
     // Fail safe check to make sure that the ThreadEntry is not present
     // before issuing a delete.
-    DCHECK(tmp->meta->isThreadEntryRemovedFromAllInMap(tmp));
+    DCHECK(tmp->meta->isThreadEntryRemovedFromAllInMap(tmp, true));
 
     delete tmp;
   }
@@ -246,6 +249,7 @@ void StaticMetaBase::destroy(EntryID* ent) {
     ThreadEntrySet tmpEntrySet;
 
     {
+      std::shared_lock forkRlock(meta.forkHandlerLock_);
       std::unique_lock wlock(meta.accessAllThreadsLock_, std::defer_lock);
       if (meta.strict_) {
         /*
@@ -258,15 +262,16 @@ void StaticMetaBase::destroy(EntryID* ent) {
         wlock.lock();
       }
 
+      uint32_t id =
+          ent->value.exchange(kEntryIDInvalid, std::memory_order_acquire);
+      if (id == kEntryIDInvalid) {
+        return;
+      }
+      meta.allId2ThreadEntrySets_[id].swap(tmpEntrySet);
+      forkRlock.unlock();
+
       {
         std::lock_guard<std::mutex> g(meta.lock_);
-        uint32_t id =
-            ent->value.exchange(kEntryIDInvalid, std::memory_order_relaxed);
-        if (id == kEntryIDInvalid) {
-          return;
-        }
-
-        meta.allId2ThreadEntrySets_[id].swap(tmpEntrySet);
         for (auto& e : tmpEntrySet.threadEntries) {
           auto elementsCapacity = e->getElementsCapacity();
           if (id < elementsCapacity) {

folly/detail/ThreadLocalDetail.h

@@ -64,7 +64,7 @@ constexpr uint32_t kEntryIDInvalid = std::numeric_limits<uint32_t>::max();
 struct ElementWrapper {
   using DeleterFunType = void(void*, TLPDestructionMode);
 
-  bool dispose(TLPDestructionMode mode) {
+  bool dispose(TLPDestructionMode mode) noexcept {
     if (ptr == nullptr) {
       return false;
     }
@@ -118,7 +118,7 @@ struct ElementWrapper {
     ptr = p;
   }
 
-  void cleanup() {
+  void cleanup() noexcept {
     if (ownsDeleter) {
       delete deleter2;
     }
@@ -454,7 +454,12 @@ struct StaticMetaBase {
   /*
    * Check if ThreadEntry* is present in the map for all slots of @ids.
    */
-  FOLLY_ALWAYS_INLINE bool isThreadEntryRemovedFromAllInMap(ThreadEntry* te) {
+  FOLLY_ALWAYS_INLINE bool isThreadEntryRemovedFromAllInMap(
+      ThreadEntry* te, bool needForkLock) {
+    std::shared_lock rlocked(forkHandlerLock_, std::defer_lock);
+    if (needForkLock) {
+      rlocked.lock();
+    }
     uint32_t maxId = nextId_.load();
     for (uint32_t i = 0; i < maxId; ++i) {
       if (allId2ThreadEntrySets_[i].rlock()->contains(te)) {
@@ -497,6 +502,17 @@ struct StaticMetaBase {
   // This is a map of all thread entries mapped to index i with active
   // elements[i];
   folly::atomic_grow_array<SynchronizedThreadEntrySet> allId2ThreadEntrySets_;
+
+  // Note on locking rules. There are 4 locks involved in managing StaticMeta:
+  // fork handler lock (getStaticMetaGlobalForkMutex(),
+  // access all threads lock (accessAllThreadsLock_),
+  // per thread entry set lock implicit in SynchronizedThreadEntrySet and
+  // meta lock (lock_)
+  //
+  // If multiple locks need to be acquired in a call path, the above is also
+  // the order in which they should be acquired. Additionally, if per
+  // ThreadEntrySet locks are the only ones that are acquired in a path, it
+  // must also acquire shared lock on the fork handler lock.
 };
 
 struct FakeUniqueInstance {
@@ -687,8 +703,10 @@ struct FOLLY_EXPORT StaticMeta final : StaticMetaBase {
   }
 
   static void onForkChild() {
-    // only the current thread survives
     auto& meta = instance();
+    // only the current thread survives
+    meta.lock_.unlock();
+    meta.accessAllThreadsLock_.unlock();
     auto threadEntry = meta.threadEntry_();
     // Loop through allId2ThreadEntrySets_; Only keep ThreadEntry* in the map
     // for ThreadEntry::elements that are still in use by the current thread.
@@ -703,8 +721,6 @@ struct FOLLY_EXPORT StaticMeta final : StaticMetaBase {
         wlockedSet->clear();
       }
     }
-    meta.lock_.unlock();
-    meta.accessAllThreadsLock_.unlock();
     meta.forkHandlerLock_.unlock();
   }
 };