Auto merge of #3973 - RalfJung:os-unfair-lock, r=RalfJung

ensure that a macOS os_unfair_lock that is moved while being held is not implicitly unlocked

Fixes #3859

We mark an os_unfair_lock that is moved while being held as "poisoned", which means it is not considered forever locked. That's not quite what the real implementation does, but allowing arbitrary moves-while-locked would likely expose a ton of implementation details, so hopefully this is good enough.

Author: bors

src/concurrency/sync.rs

@@ -193,75 +193,109 @@ impl<'tcx> AllocExtra<'tcx> {
 /// If `init` is set to this, we consider the primitive initialized.
 pub const LAZY_INIT_COOKIE: u32 = 0xcafe_affe;
 
-/// Helper for lazily initialized `alloc_extra.sync` data:
-/// this forces an immediate init.
-pub fn lazy_sync_init<'tcx, T: 'static + Copy>(
-    ecx: &mut MiriInterpCx<'tcx>,
-    primitive: &MPlaceTy<'tcx>,
-    init_offset: Size,
-    data: T,
-) -> InterpResult<'tcx> {
-    let (alloc, offset, _) = ecx.ptr_get_alloc_id(primitive.ptr(), 0)?;
-    let (alloc_extra, _machine) = ecx.get_alloc_extra_mut(alloc)?;
-    alloc_extra.sync.insert(offset, Box::new(data));
-    // Mark this as "initialized".
-    let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?;
-    ecx.write_scalar_atomic(
-        Scalar::from_u32(LAZY_INIT_COOKIE),
-        &init_field,
-        AtomicWriteOrd::Relaxed,
-    )?;
-    interp_ok(())
-}
-
-/// Helper for lazily initialized `alloc_extra.sync` data:
-/// Checks if the primitive is initialized, and return its associated data if so.
-/// Otherwise, calls `new_data` to initialize the primitive.
-pub fn lazy_sync_get_data<'tcx, T: 'static + Copy>(
-    ecx: &mut MiriInterpCx<'tcx>,
-    primitive: &MPlaceTy<'tcx>,
-    init_offset: Size,
-    name: &str,
-    new_data: impl FnOnce(&mut MiriInterpCx<'tcx>) -> InterpResult<'tcx, T>,
-) -> InterpResult<'tcx, T> {
-    // Check if this is already initialized. Needs to be atomic because we can race with another
-    // thread initializing. Needs to be an RMW operation to ensure we read the *latest* value.
-    // So we just try to replace MUTEX_INIT_COOKIE with itself.
-    let init_cookie = Scalar::from_u32(LAZY_INIT_COOKIE);
-    let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?;
-    let (_init, success) = ecx
-        .atomic_compare_exchange_scalar(
-            &init_field,
-            &ImmTy::from_scalar(init_cookie, ecx.machine.layouts.u32),
-            init_cookie,
-            AtomicRwOrd::Relaxed,
-            AtomicReadOrd::Relaxed,
-            /* can_fail_spuriously */ false,
-        )?
-        .to_scalar_pair();
-
-    if success.to_bool()? {
-        // If it is initialized, it must be found in the "sync primitive" table,
-        // or else it has been moved illegally.
-        let (alloc, offset, _) = ecx.ptr_get_alloc_id(primitive.ptr(), 0)?;
-        let alloc_extra = ecx.get_alloc_extra(alloc)?;
-        let data = alloc_extra
-            .get_sync::<T>(offset)
-            .ok_or_else(|| err_ub_format!("`{name}` can't be moved after first use"))?;
-        interp_ok(*data)
-    } else {
-        let data = new_data(ecx)?;
-        lazy_sync_init(ecx, primitive, init_offset, data)?;
-        interp_ok(data)
-    }
-}
-
 // Public interface to synchronization primitives. Please note that in most
 // cases, the function calls are infallible and it is the client's (shim
 // implementation's) responsibility to detect and deal with erroneous
 // situations.
 impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
 pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
+    /// Helper for lazily initialized `alloc_extra.sync` data:
+    /// this forces an immediate init.
+    fn lazy_sync_init<T: 'static + Copy>(
+        &mut self,
+        primitive: &MPlaceTy<'tcx>,
+        init_offset: Size,
+        data: T,
+    ) -> InterpResult<'tcx> {
+        let this = self.eval_context_mut();
+
+        let (alloc, offset, _) = this.ptr_get_alloc_id(primitive.ptr(), 0)?;
+        let (alloc_extra, _machine) = this.get_alloc_extra_mut(alloc)?;
+        alloc_extra.sync.insert(offset, Box::new(data));
+        // Mark this as "initialized".
+        let init_field = primitive.offset(init_offset, this.machine.layouts.u32, this)?;
+        this.write_scalar_atomic(
+            Scalar::from_u32(LAZY_INIT_COOKIE),
+            &init_field,
+            AtomicWriteOrd::Relaxed,
+        )?;
+        interp_ok(())
+    }
+
+    /// Helper for lazily initialized `alloc_extra.sync` data:
+    /// Checks if the primitive is initialized:
+    /// - If yes, fetches the data from `alloc_extra.sync`, or calls `missing_data` if that fails
+    ///   and stores that in `alloc_extra.sync`.
+    /// - Otherwise, calls `new_data` to initialize the primitive.
+    fn lazy_sync_get_data<T: 'static + Copy>(
+        &mut self,
+        primitive: &MPlaceTy<'tcx>,
+        init_offset: Size,
+        missing_data: impl FnOnce() -> InterpResult<'tcx, T>,
+        new_data: impl FnOnce(&mut MiriInterpCx<'tcx>) -> InterpResult<'tcx, T>,
+    ) -> InterpResult<'tcx, T> {
+        let this = self.eval_context_mut();
+
+        // Check if this is already initialized. Needs to be atomic because we can race with another
+        // thread initializing. Needs to be an RMW operation to ensure we read the *latest* value.
+        // So we just try to replace MUTEX_INIT_COOKIE with itself.
+        let init_cookie = Scalar::from_u32(LAZY_INIT_COOKIE);
+        let init_field = primitive.offset(init_offset, this.machine.layouts.u32, this)?;
+        let (_init, success) = this
+            .atomic_compare_exchange_scalar(
+                &init_field,
+                &ImmTy::from_scalar(init_cookie, this.machine.layouts.u32),
+                init_cookie,
+                AtomicRwOrd::Relaxed,
+                AtomicReadOrd::Relaxed,
+                /* can_fail_spuriously */ false,
+            )?
+            .to_scalar_pair();
+
+        if success.to_bool()? {
+            // If it is initialized, it must be found in the "sync primitive" table,
+            // or else it has been moved illegally.
+            let (alloc, offset, _) = this.ptr_get_alloc_id(primitive.ptr(), 0)?;
+            let (alloc_extra, _machine) = this.get_alloc_extra_mut(alloc)?;
+            if let Some(data) = alloc_extra.get_sync::<T>(offset) {
+                interp_ok(*data)
+            } else {
+                let data = missing_data()?;
+                alloc_extra.sync.insert(offset, Box::new(data));
+                interp_ok(data)
+            }
+        } else {
+            let data = new_data(this)?;
+            this.lazy_sync_init(primitive, init_offset, data)?;
+            interp_ok(data)
+        }
+    }
+
+    /// Get the synchronization primitive associated with the given pointer,
+    /// or initialize a new one.
+    fn get_sync_or_init<'a, T: 'static>(
+        &'a mut self,
+        ptr: Pointer,
+        new: impl FnOnce(&'a mut MiriMachine<'tcx>) -> InterpResult<'tcx, T>,
+    ) -> InterpResult<'tcx, &'a T>
+    where
+        'tcx: 'a,
+    {
+        let this = self.eval_context_mut();
+        // Ensure there is memory behind this pointer, so that this allocation
+        // is truly the only place where the data could be stored.
+        this.check_ptr_access(ptr, Size::from_bytes(1), CheckInAllocMsg::InboundsTest)?;
+
+        let (alloc, offset, _) = this.ptr_get_alloc_id(ptr, 0)?;
+        let (alloc_extra, machine) = this.get_alloc_extra_mut(alloc)?;
+        // Due to borrow checker reasons, we have to do the lookup twice.
+        if alloc_extra.get_sync::<T>(offset).is_none() {
+            let new = new(machine)?;
+            alloc_extra.sync.insert(offset, Box::new(new));
+        }
+        interp_ok(alloc_extra.get_sync::<T>(offset).unwrap())
+    }
+
     #[inline]
     /// Get the id of the thread that currently owns this lock.
     fn mutex_get_owner(&mut self, id: MutexId) -> ThreadId {

src/concurrency/thread.rs

@@ -59,7 +59,7 @@ macro_rules! callback {
         @unblock = |$this:ident| $unblock:block
     ) => {
         callback!(
-            @capture<$tcx, $($lft),*> { $($name: $type),+ }
+            @capture<$tcx, $($lft),*> { $($name: $type),* }
             @unblock = |$this| $unblock
             @timeout = |_this| {
                 unreachable!(

src/shims/unix/macos/sync.rs

@@ -10,28 +10,42 @@
 //! and we do not detect copying of the lock, but macOS doesn't guarantee anything
 //! in that case either.
 
+use rustc_target::abi::Size;
+
 use crate::*;
 
-struct MacOsUnfairLock {
-    id: MutexId,
+#[derive(Copy, Clone)]
+enum MacOsUnfairLock {
+    Poisoned,
+    Active { id: MutexId },
 }
 
 impl<'tcx> EvalContextExtPriv<'tcx> for crate::MiriInterpCx<'tcx> {}
 trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
-    fn os_unfair_lock_getid(&mut self, lock_ptr: &OpTy<'tcx>) -> InterpResult<'tcx, MutexId> {
+    fn os_unfair_lock_get_data(
+        &mut self,
+        lock_ptr: &OpTy<'tcx>,
+    ) -> InterpResult<'tcx, MacOsUnfairLock> {
         let this = self.eval_context_mut();
         let lock = this.deref_pointer(lock_ptr)?;
-        // We store the mutex ID in the `sync` metadata. This means that when the lock is moved,
-        // that's just implicitly creating a new lock at the new location.
-        let (alloc, offset, _) = this.ptr_get_alloc_id(lock.ptr(), 0)?;
-        let (alloc_extra, machine) = this.get_alloc_extra_mut(alloc)?;
-        if let Some(data) = alloc_extra.get_sync::<MacOsUnfairLock>(offset) {
-            interp_ok(data.id)
-        } else {
-            let id = machine.sync.mutex_create();
-            alloc_extra.sync.insert(offset, Box::new(MacOsUnfairLock { id }));
-            interp_ok(id)
-        }
+        this.lazy_sync_get_data(
+            &lock,
+            Size::ZERO, // offset for init tracking
+            || {
+                // If we get here, due to how we reset things to zero in `os_unfair_lock_unlock`,
+                // this means the lock was moved while locked. This can happen with a `std` lock,
+                // but then any future attempt to unlock will just deadlock. In practice, terrible
+                // things can probably happen if you swap two locked locks, since they'd wake up
+                // from the wrong queue... we just won't catch all UB of this library API then (we
+                // would need to store some unique identifer in-memory for this, instead of a static
+                // LAZY_INIT_COOKIE). This can't be hit via `std::sync::Mutex`.
+                interp_ok(MacOsUnfairLock::Poisoned)
+            },
+            |ecx| {
+                let id = ecx.machine.sync.mutex_create();
+                interp_ok(MacOsUnfairLock::Active { id })
+            },
+        )
     }
 }
 
@@ -40,7 +54,21 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     fn os_unfair_lock_lock(&mut self, lock_op: &OpTy<'tcx>) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
 
-        let id = this.os_unfair_lock_getid(lock_op)?;
+        let MacOsUnfairLock::Active { id } = this.os_unfair_lock_get_data(lock_op)? else {
+            // Trying to get a poisoned lock. Just block forever...
+            this.block_thread(
+                BlockReason::Sleep,
+                None,
+                callback!(
+                    @capture<'tcx> {}
+                    @unblock = |_this| {
+                        panic!("we shouldn't wake up ever")
+                    }
+                ),
+            );
+            return interp_ok(());
+        };
+
         if this.mutex_is_locked(id) {
             if this.mutex_get_owner(id) == this.active_thread() {
                 // Matching the current macOS implementation: abort on reentrant locking.
@@ -64,7 +92,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
 
-        let id = this.os_unfair_lock_getid(lock_op)?;
+        let MacOsUnfairLock::Active { id } = this.os_unfair_lock_get_data(lock_op)? else {
+            // Trying to get a poisoned lock. That never works.
+            this.write_scalar(Scalar::from_bool(false), dest)?;
+            return interp_ok(());
+        };
+
         if this.mutex_is_locked(id) {
             // Contrary to the blocking lock function, this does not check for
             // reentrancy.
@@ -80,40 +113,71 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     fn os_unfair_lock_unlock(&mut self, lock_op: &OpTy<'tcx>) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
 
-        let id = this.os_unfair_lock_getid(lock_op)?;
+        let MacOsUnfairLock::Active { id } = this.os_unfair_lock_get_data(lock_op)? else {
+            // The lock is poisoned, who knows who owns it... we'll pretend: someone else.
+            throw_machine_stop!(TerminationInfo::Abort(
+                "attempted to unlock an os_unfair_lock not owned by the current thread".to_owned()
+            ));
+        };
+
+        // Now, unlock.
         if this.mutex_unlock(id)?.is_none() {
             // Matching the current macOS implementation: abort.
             throw_machine_stop!(TerminationInfo::Abort(
                 "attempted to unlock an os_unfair_lock not owned by the current thread".to_owned()
             ));
         }
 
+        // If the lock is not locked by anyone now, it went quer.
+        // Reset to zero so that it can be moved and initialized again for the next phase.
+        if !this.mutex_is_locked(id) {
+            let lock_place = this.deref_pointer_as(lock_op, this.machine.layouts.u32)?;
+            this.write_scalar_atomic(Scalar::from_u32(0), &lock_place, AtomicWriteOrd::Relaxed)?;
+        }
+
         interp_ok(())
     }
 
     fn os_unfair_lock_assert_owner(&mut self, lock_op: &OpTy<'tcx>) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
 
-        let id = this.os_unfair_lock_getid(lock_op)?;
+        let MacOsUnfairLock::Active { id } = this.os_unfair_lock_get_data(lock_op)? else {
+            // The lock is poisoned, who knows who owns it... we'll pretend: someone else.
+            throw_machine_stop!(TerminationInfo::Abort(
+                "called os_unfair_lock_assert_owner on an os_unfair_lock not owned by the current thread".to_owned()
+            ));
+        };
         if !this.mutex_is_locked(id) || this.mutex_get_owner(id) != this.active_thread() {
             throw_machine_stop!(TerminationInfo::Abort(
                 "called os_unfair_lock_assert_owner on an os_unfair_lock not owned by the current thread".to_owned()
             ));
         }
 
+        // The lock is definitely not quiet since we are the owner.
+
         interp_ok(())
     }
 
     fn os_unfair_lock_assert_not_owner(&mut self, lock_op: &OpTy<'tcx>) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
 
-        let id = this.os_unfair_lock_getid(lock_op)?;
+        let MacOsUnfairLock::Active { id } = this.os_unfair_lock_get_data(lock_op)? else {
+            // The lock is poisoned, who knows who owns it... we'll pretend: someone else.
+            return interp_ok(());
+        };
         if this.mutex_is_locked(id) && this.mutex_get_owner(id) == this.active_thread() {
             throw_machine_stop!(TerminationInfo::Abort(
                 "called os_unfair_lock_assert_not_owner on an os_unfair_lock owned by the current thread".to_owned()
             ));
         }
 
+        // If the lock is not locked by anyone now, it went quer.
+        // Reset to zero so that it can be moved and initialized again for the next phase.
+        if !this.mutex_is_locked(id) {
+            let lock_place = this.deref_pointer_as(lock_op, this.machine.layouts.u32)?;
+            this.write_scalar_atomic(Scalar::from_u32(0), &lock_place, AtomicWriteOrd::Relaxed)?;
+        }
+
         interp_ok(())
     }
 }