also use new sync obj metadata scheme for INIT_ONCE and os_unfair_lock

Author: RalfJung

src/tools/miri/src/concurrency/sync.rs

@@ -372,6 +372,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     ///
     /// `new_meta_obj` gets invoked when there is not yet an initialization object.
     /// It has to ensure that the in-memory representation indeed matches `uninit_val`.
+    ///
+    /// The point of storing an `init_val` is so that if this memory gets copied somewhere else,
+    /// it does not look like the static initializer (i.e., `uninit_val`) any more. For some
+    /// objects we could just entirely forbid reading their bytes to ensure they don't get copied,
+    /// but that does not work for objects without a destructor (Windows `InitOnce`, macOS
+    /// `os_unfair_lock`).
     fn get_immovable_sync_with_static_init<'a, T: SyncObj>(
         &'a mut self,
         obj: &MPlaceTy<'tcx>,
@@ -383,6 +389,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     where
         'tcx: 'a,
     {
+        assert!(init_val != uninit_val);
         let this = self.eval_context_mut();
         this.check_ptr_access(obj.ptr(), obj.layout.size, CheckInAllocMsg::Dereferenceable)?;
         assert!(init_offset < obj.layout.size); // ensure our 1-byte flag fits
@@ -398,7 +405,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         // There's no sync object there yet. Create one, and try a CAS for uninit_val to init_val.
         let meta_obj = new_meta_obj(this)?;
-        let (_init, success) = this
+        let (old_init, success) = this
             .atomic_compare_exchange_scalar(
                 &init_field,
                 &ImmTy::from_scalar(Scalar::from_u8(uninit_val), this.machine.layouts.u8),
@@ -408,7 +415,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 /* can_fail_spuriously */ false,
             )?
             .to_scalar_pair();
-        assert!(success.to_bool()?, "`new_meta_obj` should have ensured that this CAS succeeds.");
+        if !success.to_bool()? {
+            // This can happen for the macOS lock if it is already marked as initialized.
+            assert_eq!(
+                old_init.to_u8()?,
+                init_val,
+                "`new_meta_obj` should have ensured that this CAS succeeds"
+            );
+        }
 
         let (alloc_extra, _machine) = this.get_alloc_extra_mut(alloc).unwrap();
         assert!(meta_obj.delete_on_write());

src/tools/miri/src/shims/unix/macos/sync.rs

@@ -13,18 +13,22 @@
 use std::cell::Cell;
 use std::time::Duration;
 
-use rustc_abi::Size;
+use rustc_abi::{Endian, FieldIdx, Size};
 
 use crate::concurrency::sync::{FutexRef, SyncObj};
 use crate::*;
 
 #[derive(Clone)]
 enum MacOsUnfairLock {
-    Poisoned,
     Active { mutex_ref: MutexRef },
+    PermanentlyLocked,
 }
 
-impl SyncObj for MacOsUnfairLock {}
+impl SyncObj for MacOsUnfairLock {
+    fn delete_on_write(&self) -> bool {
+        true
+    }
+}
 
 pub enum MacOsFutexTimeout<'a, 'tcx> {
     None,
@@ -57,22 +61,35 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
     where
         'tcx: 'a,
     {
+        // `os_unfair_lock_s` wraps a single `u32` field. We use the first byte to store the "init"
+        // flag. Due to macOS always being little endian, that's the least significant byte.
         let this = self.eval_context_mut();
+        assert!(this.tcx.data_layout.endian == Endian::Little);
+
         let lock = this.deref_pointer_as(lock_ptr, this.libc_ty_layout("os_unfair_lock_s"))?;
-        this.lazy_sync_get_data(
+        this.get_immovable_sync_with_static_init(
             &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)
+            /* uninit_val */ 0,
+            /* init_val */ 1,
+            |this| {
+                let field = this.project_field(&lock, FieldIdx::from_u32(0))?;
+                let val = this.read_scalar(&field)?.to_u32()?;
+                if val == 0 {
+                    interp_ok(MacOsUnfairLock::Active { mutex_ref: MutexRef::new() })
+                } else if val == 1 {
+                    // 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.
+                    interp_ok(MacOsUnfairLock::PermanentlyLocked)
+                } else {
+                    throw_ub_format!("`os_unfair_lock` was not properly initialized at this location, or it got overwritten");
+                }
             },
-            |_| interp_ok(MacOsUnfairLock::Active { mutex_ref: MutexRef::new() }),
         )
     }
 }
@@ -336,7 +353,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let this = self.eval_context_mut();
 
         let MacOsUnfairLock::Active { mutex_ref } = this.os_unfair_lock_get_data(lock_op)? else {
-            // The lock is poisoned, who knows who owns it... we'll pretend: someone else.
+            // A perma-locked lock is definitely not held by us.
             throw_machine_stop!(TerminationInfo::Abort(
                 "attempted to unlock an os_unfair_lock not owned by the current thread".to_owned()
             ));
@@ -365,7 +382,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let this = self.eval_context_mut();
 
         let MacOsUnfairLock::Active { mutex_ref } = this.os_unfair_lock_get_data(lock_op)? else {
-            // The lock is poisoned, who knows who owns it... we'll pretend: someone else.
+            // A perma-locked lock is definitely not held by us.
             throw_machine_stop!(TerminationInfo::Abort(
                 "called os_unfair_lock_assert_owner on an os_unfair_lock not owned by the current thread".to_owned()
             ));
@@ -387,7 +404,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let this = self.eval_context_mut();
 
         let MacOsUnfairLock::Active { mutex_ref } = this.os_unfair_lock_get_data(lock_op)? else {
-            // The lock is poisoned, who knows who owns it... we'll pretend: someone else.
+            // A perma-locked lock is definitely not held by us.
             return interp_ok(());
         };
         let mutex_ref = mutex_ref.clone();

src/tools/miri/src/shims/windows/sync.rs

@@ -1,6 +1,6 @@
 use std::time::Duration;
 
-use rustc_abi::Size;
+use rustc_abi::{FieldIdx, Size};
 
 use crate::concurrency::init_once::{EvalContextExt as _, InitOnceStatus};
 use crate::concurrency::sync::{FutexRef, SyncObj};
@@ -11,7 +11,11 @@ struct WindowsInitOnce {
     init_once: InitOnceRef,
 }
 
-impl SyncObj for WindowsInitOnce {}
+impl SyncObj for WindowsInitOnce {
+    fn delete_on_write(&self) -> bool {
+        true
+    }
+}
 
 struct WindowsFutex {
     futex: FutexRef,
@@ -22,7 +26,7 @@ impl SyncObj for WindowsFutex {}
 impl<'tcx> EvalContextExtPriv<'tcx> for crate::MiriInterpCx<'tcx> {}
 trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
     // Windows sync primitives are pointer sized.
-    // We only use the first 4 bytes for the id.
+    // We only use the first byte for the "init" flag.
 
     fn init_once_get_data<'a>(
         &'a mut self,
@@ -37,13 +41,19 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
             this.deref_pointer_as(init_once_ptr, this.windows_ty_layout("INIT_ONCE"))?;
         let init_offset = Size::ZERO;
 
-        this.lazy_sync_get_data(
+        this.get_immovable_sync_with_static_init(
             &init_once,
             init_offset,
-            || throw_ub_format!("`INIT_ONCE` can't be moved after first use"),
-            |_| {
-                // TODO: check that this is still all-zero.
-                interp_ok(WindowsInitOnce { init_once: InitOnceRef::new() })
+            /* uninit_val */ 0,
+            /* init_val */ 1,
+            |this| {
+                let ptr_field = this.project_field(&init_once, FieldIdx::from_u32(0))?;
+                let val = this.read_target_usize(&ptr_field)?;
+                if val == 0 {
+                    interp_ok(WindowsInitOnce { init_once: InitOnceRef::new() })
+                } else {
+                    throw_ub_format!("`INIT_ONCE` was not properly initialized at this location, or it got overwritten");
+                }
             },
         )
     }

src/tools/miri/tests/pass-dep/concurrency/apple-os-unfair-lock.rs

@@ -14,12 +14,10 @@ fn main() {
         libc::os_unfair_lock_assert_not_owner(lock.get());
     }
 
-    // `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).
-    // We only test the somewhat sane case of moving while unlocked that `std` plans to rely on.
+    // `os_unfair_lock`s can be moved, and even acquired again then.
     let lock = lock;
-    let locked = unsafe { libc::os_unfair_lock_trylock(lock.get()) };
-    assert!(locked);
-    let _lock = lock;
+    assert!(unsafe { libc::os_unfair_lock_trylock(lock.get()) });
+    // We can even move it while locked, but then we cannot acquire it any more.
+    let lock = lock;
+    assert!(!unsafe { libc::os_unfair_lock_trylock(lock.get()) });
 }