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

diff --git a/folly/detail/Futex.cpp b/folly/detail/Futex.cpp
index 118eb466f72dbaf6a252b6765f7b375b385b5ef2..fa628602aaa92bf32e1405e3c371cb2110d06f26 100644 (file)
--- 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 689c51a4e728aaf36f32d244e9830ff003c92a20..24cd175625758281d893dae811202c4872c0ab22 100644 (file)
--- 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>();
+}