From b6bcbf76fdd4da771977c1591eaec3faad05a9f3 Mon Sep 17 00:00:00 2001
From: Andy Wang <cbeuw.andy@gmail.com>
Date: Sat, 11 Jun 2022 20:45:45 +0100
Subject: [PATCH] Prevent futex_wait from reading outdated value

---
 src/shims/unix/linux/sync.rs          | 26 ++++++++++------
 tests/pass/concurrency/linux-futex.rs | 44 +++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 10 deletions(-)

diff --git a/src/shims/unix/linux/sync.rs b/src/shims/unix/linux/sync.rs
index 72898baa4b0a..42494da37b79 100644
--- a/src/shims/unix/linux/sync.rs
+++ b/src/shims/unix/linux/sync.rs
@@ -126,24 +126,26 @@ pub fn futex<'tcx>(
                 Align::from_bytes(4).unwrap(),
                 CheckInAllocMsg::MemoryAccessTest,
             )?;
+            // This SeqCst fence is paired with the SeqCst fence in futex_wake.
+            // Together, they make sure that our read on addr observes the latest
+            // value in modification order.
+            //
+            // If there is another thread which has changed the value of
+            // addr (to something other than expected) and called futex_wake
+            // before we get to run, then we must not block our thread
+            // because there'll be no one to wake us. We must see
+            // the value changed by the other thread and return without
+            // actually waiting.
+            this.atomic_fence(&[], AtomicFenceOp::SeqCst)?;
             // Read an `i32` through the pointer, regardless of any wrapper types.
             // It's not uncommon for `addr` to be passed as another type than `*mut i32`, such as `*const AtomicI32`.
             // FIXME: this fails if `addr` is not a pointer type.
-            // The atomic ordering for futex(https://man7.org/linux/man-pages/man2/futex.2.html):
-            //  "The load of the value of the futex word is an
-            //   atomic memory access (i.e., using atomic machine instructions
-            //   of the respective architecture).  This load, the comparison
-            //   with the expected value, and starting to sleep are performed
-            //   atomically and totally ordered with respect to other futex
-            //   operations on the same futex word."
-            // SeqCst is total order over all operations.
-            // FIXME: check if this should be changed when weak memory orders are added.
             let futex_val = this
                 .read_scalar_at_offset_atomic(
                     &addr.into(),
                     0,
                     this.machine.layouts.i32,
-                    AtomicReadOp::SeqCst,
+                    AtomicReadOp::Relaxed,
                 )?
                 .to_i32()?;
             if val == futex_val {
@@ -203,6 +205,10 @@ pub fn futex<'tcx>(
                 this.write_scalar(Scalar::from_machine_isize(-1, this), dest)?;
                 return Ok(());
             }
+            // Together with the SeqCst fence in futex_wait, this makes sure that futex_wait
+            // will see the latest value on addr which could be changed by our caller
+            // before doing the syscall.
+            this.atomic_fence(&[], AtomicFenceOp::SeqCst)?;
             let mut n = 0;
             for _ in 0..val {
                 if let Some(thread) = this.futex_wake(addr_usize, bitset) {
diff --git a/tests/pass/concurrency/linux-futex.rs b/tests/pass/concurrency/linux-futex.rs
index b2791a428566..56aba60d534a 100644
--- a/tests/pass/concurrency/linux-futex.rs
+++ b/tests/pass/concurrency/linux-futex.rs
@@ -6,6 +6,8 @@ extern crate libc;
 
 use std::mem::MaybeUninit;
 use std::ptr;
+use std::sync::atomic::AtomicI32;
+use std::sync::atomic::Ordering;
 use std::thread;
 use std::time::{Duration, Instant};
 
@@ -206,6 +208,47 @@ fn wait_wake_bitset() {
     t.join().unwrap();
 }
 
+const FREE: i32 = 0;
+const HELD: i32 = 1;
+fn concurrent_wait_wake() {
+    static FUTEX: AtomicI32 = AtomicI32::new(0);
+    for _ in 0..100 {
+        // Suppose the main thread is holding a lock implemented using futex...
+        FUTEX.store(HELD, Ordering::Relaxed);
+
+        let t = thread::spawn(move || {
+            // If this syscall runs first, then we'll be woken up by
+            // the main thread's FUTEX_WAKE, and all is fine.
+            //
+            // If this sycall runs after the main thread's store
+            // and FUTEX_WAKE, the syscall must observe that
+            // the FUTEX is FREE != HELD and return without waiting
+            // or we'll deadlock.
+            unsafe {
+                libc::syscall(
+                    libc::SYS_futex,
+                    &FUTEX as *const AtomicI32,
+                    libc::FUTEX_WAIT,
+                    HELD,
+                    ptr::null::<libc::timespec>(),
+                );
+            }
+        });
+
+        FUTEX.store(FREE, Ordering::Relaxed);
+        unsafe {
+            libc::syscall(
+                libc::SYS_futex,
+                &FUTEX as *const AtomicI32,
+                libc::FUTEX_WAKE,
+                1,
+            );
+        }
+
+        t.join().unwrap();
+    }
+}
+
 fn main() {
     wake_nobody();
     wake_dangling();
@@ -214,4 +257,5 @@ fn main() {
     wait_absolute_timeout();
     wait_wake();
     wait_wake_bitset();
+    concurrent_wait_wake();
 }