Detect pthread_mutex_t is moved

Mandragorian · Mandragorian · commit 1df484c5effb · 2024-08-02T21:56:44.000+02:00
See: #3749
diff --git a/src/concurrency/sync.rs b/src/concurrency/sync.rs
@@ -66,6 +66,30 @@ pub(super) use declare_id;
 
 declare_id!(MutexId);
 
+/// The mutex kind.
+#[derive(Debug, Clone, Copy)]
+pub struct MutexKind(i32);
+
+impl MutexKind {
+    pub fn new(k: i32) -> Self {
+        Self(k)
+    }
+
+    pub fn to_i32(&self) -> i32 {
+        self.0
+    }
+}
+
+#[derive(Debug)]
+/// Additional data that may be used by shim implementations.
+pub struct AdditionalMutexData {
+    /// The mutex kind, used by some mutex implementations like pthreads mutexes.
+    pub kind: MutexKind,
+
+    /// The address of the mutex.
+    pub address: Pointer,
+}
+
 /// The mutex state.
 #[derive(Default, Debug)]
 struct Mutex {
@@ -77,6 +101,9 @@ struct Mutex {
     queue: VecDeque<ThreadId>,
     /// Mutex clock. This tracks the moment of the last unlock.
     clock: VClock,
+
+    /// Additional data that can be set by shim implementations.
+    data: Option<AdditionalMutexData>,
 }
 
 declare_id!(RwLockId);
@@ -200,9 +227,14 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
     /// Provides the closure with the next MutexId. Creates that mutex if the closure returns None,
     /// otherwise returns the value from the closure.
     #[inline]
-    fn mutex_get_or_create<F>(&mut self, existing: F) -> InterpResult<'tcx, MutexId>
+    fn mutex_get_or_create<F, I>(
+        &mut self,
+        existing: F,
+        initialize_data: I,
+    ) -> InterpResult<'tcx, MutexId>
     where
         F: FnOnce(&mut MiriInterpCx<'tcx>, MutexId) -> InterpResult<'tcx, Option<MutexId>>,
+        I: FnOnce() -> InterpResult<'tcx, Option<AdditionalMutexData>>,
     {
         let this = self.eval_context_mut();
         let next_index = this.machine.sync.mutexes.next_index();
@@ -214,6 +246,8 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
         } else {
             let new_index = this.machine.sync.mutexes.push(Default::default());
             assert_eq!(next_index, new_index);
+            let data = initialize_data()?;
+            this.machine.sync.mutexes[new_index].data = data;
             Ok(new_index)
         }
     }
@@ -291,11 +325,22 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         lock_op: &OpTy<'tcx>,
         lock_layout: TyAndLayout<'tcx>,
         offset: u64,
+        initialize_data: impl FnOnce() -> InterpResult<'tcx, Option<AdditionalMutexData>>,
     ) -> InterpResult<'tcx, MutexId> {
         let this = self.eval_context_mut();
-        this.mutex_get_or_create(|ecx, next_id| {
-            ecx.get_or_create_id(next_id, lock_op, lock_layout, offset)
-        })
+        this.mutex_get_or_create(
+            |ecx, next_id| ecx.get_or_create_id(next_id, lock_op, lock_layout, offset),
+            initialize_data,
+        )
+    }
+
+    /// Retrieve the additional data stored for a mutex.
+    fn mutex_get_data<'a>(&'a mut self, id: MutexId) -> Option<&'a AdditionalMutexData>
+    where
+        'tcx: 'a,
+    {
+        let this = self.eval_context_ref();
+        this.machine.sync.mutexes[id].data.as_ref()
     }
 
     fn rwlock_get_or_create_id(
diff --git a/src/lib.rs b/src/lib.rs
@@ -131,7 +131,10 @@ pub use crate::concurrency::{
     cpu_affinity::MAX_CPUS,
     data_race::{AtomicFenceOrd, AtomicReadOrd, AtomicRwOrd, AtomicWriteOrd, EvalContextExt as _},
     init_once::{EvalContextExt as _, InitOnceId},
-    sync::{CondvarId, EvalContextExt as _, MutexId, RwLockId, SynchronizationObjects},
+    sync::{
+        AdditionalMutexData, CondvarId, EvalContextExt as _, MutexId, MutexKind, RwLockId,
+        SynchronizationObjects,
+    },
     thread::{
         BlockReason, EvalContextExt as _, StackEmptyCallback, ThreadId, ThreadManager,
         TimeoutAnchor, TimeoutClock, UnblockCallback,
diff --git a/src/shims/unix/macos/sync.rs b/src/shims/unix/macos/sync.rs
@@ -19,7 +19,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
         // os_unfair_lock holds a 32-bit value, is initialized with zero and
         // must be assumed to be opaque. Therefore, we can just store our
         // internal mutex ID in the structure without anyone noticing.
-        this.mutex_get_or_create_id(lock_op, this.libc_ty_layout("os_unfair_lock"), 0)
+        this.mutex_get_or_create_id(lock_op, this.libc_ty_layout("os_unfair_lock"), 0, || Ok(None))
     }
 }
 
diff --git a/src/shims/unix/sync.rs b/src/shims/unix/sync.rs
@@ -118,13 +118,29 @@ fn mutex_get_id<'tcx>(
     ecx: &mut MiriInterpCx<'tcx>,
     mutex_op: &OpTy<'tcx>,
 ) -> InterpResult<'tcx, MutexId> {
+    let address = ecx.deref_pointer_as(mutex_op, ecx.libc_ty_layout("pthread_mutex_t"))?.ptr();
+    let kind = MutexKind::new(mutex_get_kind(ecx, mutex_op)?);
     ecx.mutex_get_or_create_id(
         mutex_op,
         ecx.libc_ty_layout("pthread_mutex_t"),
         mutex_id_offset(ecx)?,
+        || Ok(Some(AdditionalMutexData { kind, address })),
     )
 }
 
+fn mutex_check_moved<'tcx>(
+    ecx: &mut MiriInterpCx<'tcx>,
+    mutex_op: &OpTy<'tcx>,
+    id: MutexId,
+) -> InterpResult<'tcx, ()> {
+    let addr = ecx.deref_pointer_as(mutex_op, ecx.libc_ty_layout("pthread_mutex_t"))?.ptr().addr();
+    let data = ecx.mutex_get_data(id).expect("data should be always exist for pthreads");
+    if data.address.addr() != addr {
+        throw_ub_format!("pthread_mutex_t can't be moved after first use")
+    }
+    Ok(())
+}
+
 fn mutex_reset_id<'tcx>(
     ecx: &mut MiriInterpCx<'tcx>,
     mutex_op: &OpTy<'tcx>,
@@ -457,6 +473,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         mutex_set_kind(this, mutex_op, kind)?;
 
+        // Fetch (and ignore) the mutex's id to go through the initialization
+        // code, and thus register the mutex's address.
+        mutex_get_id(this, mutex_op)?;
+
         Ok(())
     }
 
@@ -467,8 +487,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
 
-        let kind = mutex_get_kind(this, mutex_op)?;
         let id = mutex_get_id(this, mutex_op)?;
+        let kind = this
+            .mutex_get_data(id)
+            .expect("data should always exist for pthread mutexes")
+            .kind
+            .to_i32();
+
+        mutex_check_moved(this, mutex_op, id)?;
 
         let ret = if this.mutex_is_locked(id) {
             let owner_thread = this.mutex_get_owner(id);
@@ -504,8 +530,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     fn pthread_mutex_trylock(&mut self, mutex_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
-        let kind = mutex_get_kind(this, mutex_op)?;
         let id = mutex_get_id(this, mutex_op)?;
+        let kind = this
+            .mutex_get_data(id)
+            .expect("data should always exist for pthread mutexes")
+            .kind
+            .to_i32();
+
+        mutex_check_moved(this, mutex_op, id)?;
 
         Ok(Scalar::from_i32(if this.mutex_is_locked(id) {
             let owner_thread = this.mutex_get_owner(id);
@@ -536,8 +568,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     fn pthread_mutex_unlock(&mut self, mutex_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
-        let kind = mutex_get_kind(this, mutex_op)?;
         let id = mutex_get_id(this, mutex_op)?;
+        let kind = this
+            .mutex_get_data(id)
+            .expect("data should always exist for pthread mutexes")
+            .kind
+            .to_i32();
+
+        mutex_check_moved(this, mutex_op, id)?;
 
         if let Some(_old_locked_count) = this.mutex_unlock(id)? {
             // The mutex was locked by the current thread.
diff --git a/tests/fail-dep/concurrency/libc_pthread_mutex_move.init.stderr b/tests/fail-dep/concurrency/libc_pthread_mutex_move.init.stderr
@@ -0,0 +1,20 @@
+error: Undefined Behavior: pthread_mutex_t can't be moved after first use
+  --> $DIR/libc_pthread_mutex_move.rs:LL:CC
+   |
+LL |         libc::pthread_mutex_lock(&mut m2 as *mut _);
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pthread_mutex_t can't be moved after first use
+   |
+   = 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: BACKTRACE:
+   = note: inside `check` at $DIR/libc_pthread_mutex_move.rs:LL:CC
+note: inside `main`
+  --> $DIR/libc_pthread_mutex_move.rs:LL:CC
+   |
+LL |         check();
+   |         ^^^^^^^
+
+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/tests/fail-dep/concurrency/libc_pthread_mutex_move.lock.stderr b/tests/fail-dep/concurrency/libc_pthread_mutex_move.lock.stderr
@@ -0,0 +1,20 @@
+error: Undefined Behavior: pthread_mutex_t can't be moved after first use
+  --> $DIR/libc_pthread_mutex_move.rs:LL:CC
+   |
+LL |         libc::pthread_mutex_lock(&mut m2 as *mut _);
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pthread_mutex_t can't be moved after first use
+   |
+   = 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: BACKTRACE:
+   = note: inside `check` at $DIR/libc_pthread_mutex_move.rs:LL:CC
+note: inside `main`
+  --> $DIR/libc_pthread_mutex_move.rs:LL:CC
+   |
+LL |         check();
+   |         ^^^^^^^
+
+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/tests/fail-dep/concurrency/libc_pthread_mutex_move.rs b/tests/fail-dep/concurrency/libc_pthread_mutex_move.rs
@@ -0,0 +1,52 @@
+//@ignore-target-windows: No pthreads on Windows
+//@revisions: lock trylock unlock init
+
+fn main() {
+    check();
+}
+
+#[cfg(init)]
+fn check() {
+    unsafe {
+        let mut m: libc::pthread_mutex_t = std::mem::zeroed();
+        assert_eq!(libc::pthread_mutex_init(&mut m as *mut _, std::ptr::null()), 0);
+
+        let mut m2 = m;
+        libc::pthread_mutex_lock(&mut m2 as *mut _); //~[init] ERROR: pthread_mutex_t can't be moved after first use
+    }
+}
+
+#[cfg(lock)]
+fn check() {
+    unsafe {
+        let mut m: libc::pthread_mutex_t = libc::PTHREAD_MUTEX_INITIALIZER;
+        libc::pthread_mutex_lock(&mut m as *mut _);
+        // libc::pthread_mutex_unlock(&mut m as *mut _);
+
+        let mut m2 = m;
+        libc::pthread_mutex_lock(&mut m2 as *mut _); //~[lock] ERROR: pthread_mutex_t can't be moved after first use
+    }
+}
+
+#[cfg(trylock)]
+fn check() {
+    unsafe {
+        let mut m: libc::pthread_mutex_t = libc::PTHREAD_MUTEX_INITIALIZER;
+        libc::pthread_mutex_trylock(&mut m as *mut _);
+        // libc::pthread_mutex_unlock(&mut m as *mut _);
+
+        let mut m2 = m;
+        libc::pthread_mutex_trylock(&mut m2 as *mut _); //~[trylock] ERROR: pthread_mutex_t can't be moved after first use
+    }
+}
+
+#[cfg(unlock)]
+fn check() {
+    unsafe {
+        let mut m: libc::pthread_mutex_t = libc::PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
+        libc::pthread_mutex_unlock(&mut m as *mut _);
+
+        let mut m2 = m;
+        libc::pthread_mutex_unlock(&mut m2 as *mut _); //~[unlock] ERROR: pthread_mutex_t can't be moved after first use
+    }
+}
diff --git a/tests/fail-dep/concurrency/libc_pthread_mutex_move.stderr b/tests/fail-dep/concurrency/libc_pthread_mutex_move.stderr
@@ -0,0 +1,15 @@
+error: Undefined Behavior: pthread_mutex_t can't be moved
+  --> $DIR/libc_pthread_mutex_move.rs:LL:CC
+   |
+LL |         libc::pthread_mutex_lock(&mut mutex2 as *mut _);
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pthread_mutex_t can't be moved
+   |
+   = 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: BACKTRACE:
+   = note: inside `main` at $DIR/libc_pthread_mutex_move.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/tests/fail-dep/concurrency/libc_pthread_mutex_move.trylock.stderr b/tests/fail-dep/concurrency/libc_pthread_mutex_move.trylock.stderr
@@ -0,0 +1,20 @@
+error: Undefined Behavior: pthread_mutex_t can't be moved after first use
+  --> $DIR/libc_pthread_mutex_move.rs:LL:CC
+   |
+LL |         libc::pthread_mutex_trylock(&mut m2 as *mut _);
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pthread_mutex_t can't be moved after first use
+   |
+   = 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: BACKTRACE:
+   = note: inside `check` at $DIR/libc_pthread_mutex_move.rs:LL:CC
+note: inside `main`
+  --> $DIR/libc_pthread_mutex_move.rs:LL:CC
+   |
+LL |         check();
+   |         ^^^^^^^
+
+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/tests/fail-dep/concurrency/libc_pthread_mutex_move.unlock.stderr b/tests/fail-dep/concurrency/libc_pthread_mutex_move.unlock.stderr
@@ -0,0 +1,20 @@
+error: Undefined Behavior: pthread_mutex_t can't be moved after first use
+  --> $DIR/libc_pthread_mutex_move.rs:LL:CC
+   |
+LL |         libc::pthread_mutex_unlock(&mut m2 as *mut _);
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pthread_mutex_t can't be moved after first use
+   |
+   = 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: BACKTRACE:
+   = note: inside `check` at $DIR/libc_pthread_mutex_move.rs:LL:CC
+note: inside `main`
+  --> $DIR/libc_pthread_mutex_move.rs:LL:CC
+   |
+LL |         check();
+   |         ^^^^^^^
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {`
`19`	`19`	`// os_unfair_lock holds a 32-bit value, is initialized with zero and`
`20`	`20`	`// must be assumed to be opaque. Therefore, we can just store our`
`21`	`21`	`// internal mutex ID in the structure without anyone noticing.`
`22`		`- this.mutex_get_or_create_id(lock_op, this.libc_ty_layout("os_unfair_lock"), 0)`
	`22`	`+ this.mutex_get_or_create_id(lock_op, this.libc_ty_layout("os_unfair_lock"), 0, \|\| Ok(None))`
`23`	`23`	`}`
`24`	`24`	`}`
`25`	`25`