From eab7000df62b7b8b2b114fb2a9b75b744506242c Mon Sep 17 00:00:00 2001
From: Maged Michael <magedmichael@fb.com>
Date: Thu, 16 Feb 2017 12:19:29 -0800
Subject: [PATCH] Fix data race in Futex<EmulatedFutexAtomic>

Summary: Fixed a data race between an atomic store by the waker and a nonatomic memcpy by the waiter.

Reviewed By: nbronson

Differential Revision: D4572410

fbshipit-source-id: 3982ca433e0f628636916516e35aeb7738ae030f
---
 folly/detail/Futex.cpp   | 20 ++++++++++++--------
 folly/test/FutexTest.cpp | 23 +++++++++++++++++++++++
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/folly/detail/Futex.cpp b/folly/detail/Futex.cpp
index 118eb466..fa628602 100644
--- a/folly/detail/Futex.cpp
+++ b/folly/detail/Futex.cpp
@@ -226,21 +226,25 @@ int emulatedFutexWake(void* addr, int count, uint32_t waitMask) {
   return numAwoken;
 }
 
+template <typename F>
 FutexResult emulatedFutexWaitImpl(
-        void* addr,
-        uint32_t expected,
-        time_point<system_clock>* absSystemTime,
-        time_point<steady_clock>* absSteadyTime,
-        uint32_t waitMask) {
+    F* futex,
+    uint32_t expected,
+    time_point<system_clock>* absSystemTime,
+    time_point<steady_clock>* absSteadyTime,
+    uint32_t waitMask) {
+  static_assert(
+      std::is_same<F, Futex<std::atomic>>::value ||
+          std::is_same<F, Futex<EmulatedFutexAtomic>>::value,
+      "Type F must be either Futex<std::atomic> or Futex<EmulatedFutexAtomic>");
+  void* addr = static_cast<void*>(futex);
   auto& bucket = EmulatedFutexBucket::bucketFor(addr);
   EmulatedFutexWaitNode node(addr, waitMask);
 
   {
     std::unique_lock<std::mutex> bucketLock(bucket.mutex_);
 
-    uint32_t actual;
-    memcpy(&actual, addr, sizeof(uint32_t));
-    if (actual != expected) {
+    if (futex->load(std::memory_order_relaxed) != expected) {
       return FutexResult::VALUE_CHANGED;
     }
 
diff --git a/folly/test/FutexTest.cpp b/folly/test/FutexTest.cpp
index 689c51a4..24cd1756 100644
--- a/folly/test/FutexTest.cpp
+++ b/folly/test/FutexTest.cpp
@@ -182,6 +182,21 @@ void run_steady_clock_test() {
   EXPECT_TRUE(A <= B && B <= C);
 }
 
+template <template <typename> class Atom>
+void run_wake_blocked_test() {
+  Futex<Atom> f(0);
+
+  auto thr = DSched::thread([&] { EXPECT_TRUE(f.futexWait(0)); });
+
+  // A sleep here guarantees to a large extent that 'thr' will execute
+  // futexWait before we wake it up, thus testing late futexWake.
+  std::this_thread::sleep_for(std::chrono::milliseconds(2));
+
+  f.store(1);
+  f.futexWake(1);
+  DSched::join(thr);
+}
+
 TEST(Futex, clock_source) {
   run_system_clock_test();
 
@@ -207,3 +222,11 @@ TEST(Futex, basic_deterministic) {
   run_basic_tests<DeterministicAtomic>();
   run_wait_until_tests<DeterministicAtomic>();
 }
+
+TEST(Futex, wake_blocked_live) {
+  run_wake_blocked_test<std::atomic>();
+}
+
+TEST(Futex, wake_blocked_emulated) {
+  run_wake_blocked_test<EmulatedFutexAtomic>();
+}
-- 
2.34.1