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

RalfJung · RalfJung · commit a802fd9c11f9 · 2024-10-14T22:43:41.000+02:00
diff --git a/src/tools/miri/src/concurrency/sync.rs b/src/tools/miri/src/concurrency/sync.rs
@@ -223,13 +223,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     }
 
     /// 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.
+    /// 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,
-        name: &str,
+        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();
@@ -254,11 +256,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             // 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 = this.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)
+            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)?;
diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/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!(
diff --git a/src/tools/miri/src/shims/unix/macos/sync.rs b/src/tools/miri/src/shims/unix/macos/sync.rs
@@ -10,24 +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 data = this.get_sync_or_init(lock.ptr(), |machine| {
-            let id = machine.sync.mutex_create();
-            interp_ok(MacOsUnfairLock { id })
-        })?;
-        interp_ok(data.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 })
+            },
+        )
     }
 }
 
@@ -36,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.
@@ -60,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.
@@ -76,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(())
     }
 }
diff --git a/src/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs
@@ -189,11 +189,16 @@ fn mutex_get_data<'tcx, 'a>(
     mutex_ptr: &OpTy<'tcx>,
 ) -> InterpResult<'tcx, PthreadMutex> {
     let mutex = ecx.deref_pointer(mutex_ptr)?;
-    ecx.lazy_sync_get_data(&mutex, mutex_init_offset(ecx)?, "pthread_mutex_t", |ecx| {
-        let kind = mutex_kind_from_static_initializer(ecx, &mutex)?;
-        let id = ecx.machine.sync.mutex_create();
-        interp_ok(PthreadMutex { id, kind })
-    })
+    ecx.lazy_sync_get_data(
+        &mutex,
+        mutex_init_offset(ecx)?,
+        || throw_ub_format!("`pthread_mutex_t` can't be moved after first use"),
+        |ecx| {
+            let kind = mutex_kind_from_static_initializer(ecx, &mutex)?;
+            let id = ecx.machine.sync.mutex_create();
+            interp_ok(PthreadMutex { id, kind })
+        },
+    )
 }
 
 /// Returns the kind of a static initializer.
@@ -261,17 +266,22 @@ fn rwlock_get_data<'tcx>(
     rwlock_ptr: &OpTy<'tcx>,
 ) -> InterpResult<'tcx, PthreadRwLock> {
     let rwlock = ecx.deref_pointer(rwlock_ptr)?;
-    ecx.lazy_sync_get_data(&rwlock, rwlock_init_offset(ecx)?, "pthread_rwlock_t", |ecx| {
-        if !bytewise_equal_atomic_relaxed(
-            ecx,
-            &rwlock,
-            &ecx.eval_path(&["libc", "PTHREAD_RWLOCK_INITIALIZER"]),
-        )? {
-            throw_unsup_format!("unsupported static initializer used for `pthread_rwlock_t`");
-        }
-        let id = ecx.machine.sync.rwlock_create();
-        interp_ok(PthreadRwLock { id })
-    })
+    ecx.lazy_sync_get_data(
+        &rwlock,
+        rwlock_init_offset(ecx)?,
+        || throw_ub_format!("`pthread_rwlock_t` can't be moved after first use"),
+        |ecx| {
+            if !bytewise_equal_atomic_relaxed(
+                ecx,
+                &rwlock,
+                &ecx.eval_path(&["libc", "PTHREAD_RWLOCK_INITIALIZER"]),
+            )? {
+                throw_unsup_format!("unsupported static initializer used for `pthread_rwlock_t`");
+            }
+            let id = ecx.machine.sync.rwlock_create();
+            interp_ok(PthreadRwLock { id })
+        },
+    )
 }
 
 // # pthread_condattr_t
@@ -386,18 +396,23 @@ fn cond_get_data<'tcx>(
     cond_ptr: &OpTy<'tcx>,
 ) -> InterpResult<'tcx, PthreadCondvar> {
     let cond = ecx.deref_pointer(cond_ptr)?;
-    ecx.lazy_sync_get_data(&cond, cond_init_offset(ecx)?, "pthread_cond_t", |ecx| {
-        if !bytewise_equal_atomic_relaxed(
-            ecx,
-            &cond,
-            &ecx.eval_path(&["libc", "PTHREAD_COND_INITIALIZER"]),
-        )? {
-            throw_unsup_format!("unsupported static initializer used for `pthread_cond_t`");
-        }
-        // This used the static initializer. The clock there is always CLOCK_REALTIME.
-        let id = ecx.machine.sync.condvar_create();
-        interp_ok(PthreadCondvar { id, clock: ClockId::Realtime })
-    })
+    ecx.lazy_sync_get_data(
+        &cond,
+        cond_init_offset(ecx)?,
+        || throw_ub_format!("`pthread_cond_t` can't be moved after first use"),
+        |ecx| {
+            if !bytewise_equal_atomic_relaxed(
+                ecx,
+                &cond,
+                &ecx.eval_path(&["libc", "PTHREAD_COND_INITIALIZER"]),
+            )? {
+                throw_unsup_format!("unsupported static initializer used for `pthread_cond_t`");
+            }
+            // This used the static initializer. The clock there is always CLOCK_REALTIME.
+            let id = ecx.machine.sync.condvar_create();
+            interp_ok(PthreadCondvar { id, clock: ClockId::Realtime })
+        },
+    )
 }
 
 impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
diff --git a/src/tools/miri/src/shims/windows/sync.rs b/src/tools/miri/src/shims/windows/sync.rs
@@ -24,11 +24,16 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let init_once = this.deref_pointer(init_once_ptr)?;
         let init_offset = Size::ZERO;
 
-        this.lazy_sync_get_data(&init_once, init_offset, "INIT_ONCE", |this| {
-            // TODO: check that this is still all-zero.
-            let id = this.machine.sync.init_once_create();
-            interp_ok(WindowsInitOnce { id })
-        })
+        this.lazy_sync_get_data(
+            &init_once,
+            init_offset,
+            || throw_ub_format!("`INIT_ONCE` can't be moved after first use"),
+            |this| {
+                // TODO: check that this is still all-zero.
+                let id = this.machine.sync.init_once_create();
+                interp_ok(WindowsInitOnce { id })
+            },
+        )
     }
 
     /// Returns `true` if we were succssful, `false` if we would block.
diff --git a/src/tools/miri/tests/fail-dep/concurrency/apple_os_unfair_lock_move_deadlock.rs b/src/tools/miri/tests/fail-dep/concurrency/apple_os_unfair_lock_move_deadlock.rs
@@ -0,0 +1,13 @@
+//@only-target: darwin
+
+use std::cell::UnsafeCell;
+
+fn main() {
+    let lock = UnsafeCell::new(libc::OS_UNFAIR_LOCK_INIT);
+
+    unsafe { libc::os_unfair_lock_lock(lock.get()) };
+    let lock = lock;
+    // This needs to either error or deadlock.
+    unsafe { libc::os_unfair_lock_lock(lock.get()) };
+    //~^ error: deadlock
+}
diff --git a/src/tools/miri/tests/fail-dep/concurrency/apple_os_unfair_lock_move_deadlock.stderr b/src/tools/miri/tests/fail-dep/concurrency/apple_os_unfair_lock_move_deadlock.stderr
@@ -0,0 +1,13 @@
+error: deadlock: the evaluated program deadlocked
+  --> tests/fail-dep/concurrency/apple_os_unfair_lock_move_deadlock.rs:LL:CC
+   |
+LL |     unsafe { libc::os_unfair_lock_lock(lock.get()) };
+   |                                                  ^ the evaluated program deadlocked
+   |
+   = note: BACKTRACE:
+   = note: inside `main` at tests/fail-dep/concurrency/apple_os_unfair_lock_move_deadlock.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+
diff --git a/src/tools/miri/tests/pass-dep/concurrency/apple-os-unfair-lock.rs b/src/tools/miri/tests/pass-dep/concurrency/apple-os-unfair-lock.rs
@@ -16,8 +16,8 @@ fn main() {
 
     // `os_unfair_lock`s can be moved and leaked.
     // In the real implementation, even moving it while locked is possible
-    // (and "forks" the lock, i.e. old and new location have independent wait queues);
-    // Miri behavior differs here and anyway none of this is documented.
+    // (and "forks" the lock, i.e. old and new location have independent wait queues).
+    // We only test the somewhat sane case of moving while unlocked that `std` plans to rely on.
     let lock = lock;
     let locked = unsafe { libc::os_unfair_lock_trylock(lock.get()) };
     assert!(locked);
