throw an error if a synchronization object is read/written while threads are queued

Author: RalfJung

src/borrow_tracker/mod.rs

@@ -115,15 +115,6 @@ impl VisitProvenance for GlobalStateInner {
 /// We need interior mutable access to the global state.
 pub type GlobalState = RefCell<GlobalStateInner>;
 
-impl fmt::Display for AccessKind {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            AccessKind::Read => write!(f, "read access"),
-            AccessKind::Write => write!(f, "write access"),
-        }
-    }
-}
-
 /// Policy on whether to recurse into fields to retag
 #[derive(Copy, Clone, Debug)]
 pub enum RetagFields {

src/concurrency/init_once.rs

@@ -57,6 +57,10 @@ impl InitOnceRef {
     pub fn begin(&self) {
         self.0.borrow_mut().begin();
     }
+
+    pub fn queue_is_empty(&self) -> bool {
+        self.0.borrow().waiters.is_empty()
+    }
 }
 
 impl VisitProvenance for InitOnceRef {

src/concurrency/sync.rs

@@ -16,6 +16,11 @@ use crate::*;
 
 /// A trait for the synchronization metadata that can be attached to a memory location.
 pub trait SyncObj: Any {
+    /// Determines whether reads/writes to this object's location are currently permitted.
+    fn on_access<'tcx>(&self, _access_kind: AccessKind) -> InterpResult<'tcx> {
+        interp_ok(())
+    }
+
     /// Determines whether this object's metadata shall be deleted when a write to its
     /// location occurs.
     fn delete_on_write(&self) -> bool {
@@ -62,6 +67,10 @@ impl MutexRef {
     pub fn owner(&self) -> Option<ThreadId> {
         self.0.borrow().owner
     }
+
+    pub fn queue_is_empty(&self) -> bool {
+        self.0.borrow().queue.is_empty()
+    }
 }
 
 impl VisitProvenance for MutexRef {
@@ -138,6 +147,11 @@ impl RwLockRef {
     pub fn is_write_locked(&self) -> bool {
         self.0.borrow().is_write_locked()
     }
+
+    pub fn queue_is_empty(&self) -> bool {
+        let inner = self.0.borrow();
+        inner.reader_queue.is_empty() && inner.writer_queue.is_empty()
+    }
 }
 
 impl VisitProvenance for RwLockRef {
@@ -165,8 +179,8 @@ impl CondvarRef {
         Self(Default::default())
     }
 
-    pub fn is_awaited(&self) -> bool {
-        !self.0.borrow().waiters.is_empty()
+    pub fn queue_is_empty(&self) -> bool {
+        self.0.borrow().waiters.is_empty()
     }
 }
 

src/helpers.rs

@@ -1,7 +1,7 @@
 use std::num::NonZero;
 use std::sync::Mutex;
 use std::time::Duration;
-use std::{cmp, iter};
+use std::{cmp, fmt, iter};
 
 use rand::RngCore;
 use rustc_abi::{Align, ExternAbi, FieldIdx, FieldsShape, Size, Variants};
@@ -29,6 +29,15 @@ pub enum AccessKind {
     Write,
 }
 
+impl fmt::Display for AccessKind {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            AccessKind::Read => write!(f, "read access"),
+            AccessKind::Write => write!(f, "write access"),
+        }
+    }
+}
+
 /// Gets an instance for a path.
 ///
 /// A `None` namespace indicates we are looking for a module.

src/machine.rs

@@ -1547,6 +1547,11 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
         if let Some(borrow_tracker) = &alloc_extra.borrow_tracker {
             borrow_tracker.before_memory_read(alloc_id, prov_extra, range, machine)?;
         }
+        // Check if there are any sync objects that would like to prevent reading this memory.
+        for (_offset, obj) in alloc_extra.sync_objs.range(range.start..range.end()) {
+            obj.on_access(AccessKind::Read)?;
+        }
+
         interp_ok(())
     }
 
@@ -1590,11 +1595,13 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
         // Delete sync objects that don't like writes.
         // Most of the time, we can just skip this.
         if !alloc_extra.sync_objs.is_empty() {
-            let to_delete = alloc_extra
-                .sync_objs
-                .range(range.start..range.end())
-                .filter_map(|(offset, obj)| obj.delete_on_write().then_some(*offset))
-                .collect::<Vec<_>>();
+            let mut to_delete = vec![];
+            for (offset, obj) in alloc_extra.sync_objs.range(range.start..range.end()) {
+                obj.on_access(AccessKind::Write)?;
+                if obj.delete_on_write() {
+                    to_delete.push(*offset);
+                }
+            }
             for offset in to_delete {
                 alloc_extra.sync_objs.remove(&offset);
             }

src/shims/unix/macos/sync.rs

@@ -25,6 +25,17 @@ enum MacOsUnfairLock {
 }
 
 impl SyncObj for MacOsUnfairLock {
+    fn on_access<'tcx>(&self, access_kind: AccessKind) -> InterpResult<'tcx> {
+        if let MacOsUnfairLock::Active { mutex_ref } = self
+            && !mutex_ref.queue_is_empty()
+        {
+            throw_ub_format!(
+                "{access_kind} to `os_unfair_lock` is forbidden while the queue is non-empty"
+            );
+        }
+        interp_ok(())
+    }
+
     fn delete_on_write(&self) -> bool {
         true
     }
@@ -81,10 +92,10 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     // This is a lock that got copied while it is initialized. We de-initialize
                     // locks when they get released, so it got copied while locked. Unfortunately
                     // that is something `std` needs to support (the guard could have been leaked).
-                    // So we behave like a futex-based lock whose wait queue got pruned: any attempt
-                    // to acquire the lock will just wait forever.
-                    // In practice there actually could be a wait queue there, if someone moves a
-                    // lock *while threads are queued*; this is UB we will not detect.
+                    // On the plus side, we know nobody was queued for the lock while it got copied;
+                    // that would have been rejected by our `on_access`. So we behave like a
+                    // futex-based lock would in this case: any attempt to acquire the lock will
+                    // just wait forever, since there's nobody to wake us up.
                     interp_ok(MacOsUnfairLock::PermanentlyLocked)
                 } else {
                     throw_ub_format!("`os_unfair_lock` was not properly initialized at this location, or it got overwritten");

src/shims/unix/sync.rs

@@ -110,6 +110,15 @@ struct PthreadMutex {
 }
 
 impl SyncObj for PthreadMutex {
+    fn on_access<'tcx>(&self, access_kind: AccessKind) -> InterpResult<'tcx> {
+        if !self.mutex_ref.queue_is_empty() {
+            throw_ub_format!(
+                "{access_kind} to `pthread_mutex_t` is forbidden while the queue is non-empty"
+            );
+        }
+        interp_ok(())
+    }
+
     fn delete_on_write(&self) -> bool {
         true
     }
@@ -230,6 +239,15 @@ struct PthreadRwLock {
 }
 
 impl SyncObj for PthreadRwLock {
+    fn on_access<'tcx>(&self, access_kind: AccessKind) -> InterpResult<'tcx> {
+        if !self.rwlock_ref.queue_is_empty() {
+            throw_ub_format!(
+                "{access_kind} to `pthread_rwlock_t` is forbidden while the queue is non-empty"
+            );
+        }
+        interp_ok(())
+    }
+
     fn delete_on_write(&self) -> bool {
         true
     }
@@ -361,6 +379,15 @@ struct PthreadCondvar {
 }
 
 impl SyncObj for PthreadCondvar {
+    fn on_access<'tcx>(&self, access_kind: AccessKind) -> InterpResult<'tcx> {
+        if !self.condvar_ref.queue_is_empty() {
+            throw_ub_format!(
+                "{access_kind} to `pthread_cond_t` is forbidden while the queue is non-empty"
+            );
+        }
+        interp_ok(())
+    }
+
     fn delete_on_write(&self) -> bool {
         true
     }
@@ -900,7 +927,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         // Reading the field also has the side-effect that we detect double-`destroy`
         // since we make the field uninit below.
         let condvar = &cond_get_data(this, cond_op)?.condvar_ref;
-        if condvar.is_awaited() {
+        if !condvar.queue_is_empty() {
             throw_ub_format!("destroying an awaited conditional variable");
         }
 

src/shims/windows/sync.rs

@@ -12,6 +12,15 @@ struct WindowsInitOnce {
 }
 
 impl SyncObj for WindowsInitOnce {
+    fn on_access<'tcx>(&self, access_kind: AccessKind) -> InterpResult<'tcx> {
+        if !self.init_once.queue_is_empty() {
+            throw_ub_format!(
+                "{access_kind} to `INIT_ONCE` is forbidden while the queue is non-empty"
+            );
+        }
+        interp_ok(())
+    }
+
     fn delete_on_write(&self) -> bool {
         true
     }

tests/fail-dep/concurrency/apple_os_unfair_lock_move_with_queue.rs

@@ -0,0 +1,29 @@
+//@only-target: darwin
+#![feature(sync_unsafe_cell)]
+
+use std::cell::SyncUnsafeCell;
+use std::sync::atomic::*;
+use std::thread;
+
+fn main() {
+    let lock = SyncUnsafeCell::new(libc::OS_UNFAIR_LOCK_INIT);
+
+    thread::scope(|s| {
+        // First thread: grabs the lock.
+        s.spawn(|| {
+            unsafe { libc::os_unfair_lock_lock(lock.get()) };
+            thread::yield_now();
+            unreachable!();
+        });
+        // Second thread: queues for the lock.
+        s.spawn(|| {
+            unsafe { libc::os_unfair_lock_lock(lock.get()) };
+            unreachable!();
+        });
+        // Third thread: tries to read the lock while second thread is queued.
+        s.spawn(|| {
+            let atomic_ref = unsafe { &*lock.get().cast::<AtomicU32>() };
+            let _val = atomic_ref.load(Ordering::Relaxed); //~ERROR: read access to `os_unfair_lock` is forbidden while the queue is non-empty
+        });
+    });
+}

tests/fail-dep/concurrency/apple_os_unfair_lock_move_with_queue.stderr

@@ -0,0 +1,13 @@
+error: Undefined Behavior: read access to `os_unfair_lock` is forbidden while the queue is non-empty
+  --> tests/fail-dep/concurrency/apple_os_unfair_lock_move_with_queue.rs:LL:CC
+   |
+LL | ...   let _val = atomic_ref.load(Ordering::Relaxed);
+   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+