Fix rare corruption in StaticMeta::head_ list around fork

Summary:
In a rare case, the current thread's would be inserted in the StaticMeta linked
list twice, causing the list to be corrupted, leading to code spinning forever.

After a fork, in the child, only the current thread survives, so all other threads
must be forcefully removed from StaticMeta. We do that by clearing the list
and re-adding the current thread, but we didn't check whether the current thread
was already there. It is possible for the current thread to not be in the list
if it never used any ThreadLocalPtr objects with the same tag.

Now, when the thread in the child tries to use a ThreadLocalPtr with the same
tag, it adds itself to the list (##if (prevCapacity == 0)
meta.push_back(threadEntry)##), but it had already been added by the post-fork
handler, boom.

Fix by adding the necessary check in onForkChild.

Test Plan: @durham's test case, also added new test for this

Reviewed By: delong.j@fb.com

FB internal diff: D1158672

@override-unit-failures

diff --git a/folly/detail/ThreadLocalDetail.h b/folly/detail/ThreadLocalDetail.h
index 9efd8cab127a0e54151f21fe24f1ad84b18df74e..86910f0df8e754f0688d64294ebb305dad7a6199 100644 (file)
--- a/folly/detail/ThreadLocalDetail.h
+++ b/folly/detail/ThreadLocalDetail.h
@@ -213,7 +213,11 @@ struct StaticMeta {
   static void onForkChild(void) {
     // only the current thread survives
     inst_->head_.next = inst_->head_.prev = &inst_->head_;
-    inst_->push_back(getThreadEntry());
+    ThreadEntry* threadEntry = getThreadEntry();
+    // If this thread was in the list before the fork, add it back.
+    if (threadEntry->elementsCapacity != 0) {
+      inst_->push_back(threadEntry);
+    }
     inst_->lock_.unlock();
   }
 

diff --git a/folly/test/ThreadLocalTest.cpp b/folly/test/ThreadLocalTest.cpp
index 7c72c51cb1e7e8003361ccf73dd32f7a04c3d9e0..fdf60ebd66db1d8c62334f473d85db17087668fc 100644 (file)
--- a/folly/test/ThreadLocalTest.cpp
+++ b/folly/test/ThreadLocalTest.cpp
@@ -476,6 +476,35 @@ TEST(ThreadLocal, Fork) {
   EXPECT_EQ(1, totalValue());
 }
 
+struct HoldsOneTag2 {};
+
+TEST(ThreadLocal, Fork2) {
+  // A thread-local tag that was used in the parent from a *different* thread
+  // (but not the forking thread) would cause the child to hang in a
+  // ThreadLocalPtr's object destructor. Yeah.
+  ThreadLocal<HoldsOne, HoldsOneTag2> p;
+  {
+    // use tag in different thread
+    std::thread t([&p] { p.get(); });
+    t.join();
+  }
+  pid_t pid = fork();
+  if (pid == 0) {
+    {
+      ThreadLocal<HoldsOne, HoldsOneTag2> q;
+      q.get();
+    }
+    _exit(0);
+  } else if (pid > 0) {
+    int status;
+    EXPECT_EQ(pid, waitpid(pid, &status, 0));
+    EXPECT_TRUE(WIFEXITED(status));
+    EXPECT_EQ(0, WEXITSTATUS(status));
+  } else {
+    EXPECT_TRUE(false) << "fork failed";
+  }
+}
+
 // Simple reference implementation using pthread_get_specific
 template<typename T>
 class PThreadGetSpecific {