Auto merge of #3784 - Mandragorian:detect_moved_mutexes, r=RalfJung

Detect when pthread_mutex_t is moved

What I am not sure about this PR is how to support storing the additional mutex data like its address and kind. If I understand correctly the `concurrency::sync::Mutex` struct is to be used by any mutex implementation. This possibly means that different implementation might want to store different data in the mutex. So any additional data should be implementation defined somehow. Solutions that come to mind:

- Store the additional data as `Box<dyn Any>` and the implementations can downcast their data when they fetch them.
- Have each shim implementation define a `static mut` map between `MutexID`s and the additional data.

Let me know

Fixes #3749

Author: bors

src/concurrency/init_once.rs

@@ -35,8 +35,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         offset: u64,
     ) -> InterpResult<'tcx, InitOnceId> {
         let this = self.eval_context_mut();
-        this.get_or_create_id(lock_op, lock_layout, offset, |ecx| &mut ecx.machine.sync.init_onces)?
-            .ok_or_else(|| err_ub_format!("init_once has invalid ID").into())
+        this.get_or_create_id(
+            lock_op,
+            lock_layout,
+            offset,
+            |ecx| &mut ecx.machine.sync.init_onces,
+            |_| Ok(Default::default()),
+        )?
+        .ok_or_else(|| err_ub_format!("init_once has invalid ID").into())
     }
 
     #[inline]

src/concurrency/sync.rs

@@ -66,6 +66,27 @@ pub(super) use declare_id;
 
 declare_id!(MutexId);
 
+/// The mutex kind.
+#[derive(Debug, Clone, Copy)]
+#[non_exhaustive]
+pub enum MutexKind {
+    Invalid,
+    Normal,
+    Default,
+    Recursive,
+    ErrorCheck,
+}
+
+#[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: u64,
+}
+
 /// The mutex state.
 #[derive(Default, Debug)]
 struct Mutex {
@@ -77,6 +98,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);
@@ -168,13 +192,15 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
     /// Returns `None` if memory stores a non-zero invalid ID.
     ///
     /// `get_objs` must return the `IndexVec` that stores all the objects of this type.
+    /// `create_obj` must create the new object if initialization is needed.
     #[inline]
-    fn get_or_create_id<Id: SyncId + Idx, T: Default>(
+    fn get_or_create_id<Id: SyncId + Idx, T>(
         &mut self,
         lock_op: &OpTy<'tcx>,
         lock_layout: TyAndLayout<'tcx>,
         offset: u64,
         get_objs: impl for<'a> Fn(&'a mut MiriInterpCx<'tcx>) -> &'a mut IndexVec<Id, T>,
+        create_obj: impl for<'a> FnOnce(&'a mut MiriInterpCx<'tcx>) -> InterpResult<'tcx, T>,
     ) -> InterpResult<'tcx, Option<Id>> {
         let this = self.eval_context_mut();
         let value_place =
@@ -196,7 +222,8 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
         Ok(if success.to_bool().expect("compare_exchange's second return value is a bool") {
             // We set the in-memory ID to `next_index`, now also create this object in the machine
             // state.
-            let new_index = get_objs(this).push(T::default());
+            let obj = create_obj(this)?;
+            let new_index = get_objs(this).push(obj);
             assert_eq!(next_index, new_index);
             Some(new_index)
         } else {
@@ -210,6 +237,32 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
         })
     }
 
+    /// Eagerly creates a Miri sync structure.
+    ///
+    /// `create_id` will store the index of the sync_structure in the memory pointed to by
+    /// `lock_op`, so that future calls to `get_or_create_id` will see it as initialized.
+    /// - `lock_op` must hold a pointer to the sync structure.
+    /// - `lock_layout` must be the memory layout of the sync structure.
+    /// - `offset` must be the offset inside the sync structure where its miri id will be stored.
+    /// - `get_objs` is described in `get_or_create_id`.
+    /// - `obj` must be the new sync object.
+    fn create_id<Id: SyncId + Idx, T>(
+        &mut self,
+        lock_op: &OpTy<'tcx>,
+        lock_layout: TyAndLayout<'tcx>,
+        offset: u64,
+        get_objs: impl for<'a> Fn(&'a mut MiriInterpCx<'tcx>) -> &'a mut IndexVec<Id, T>,
+        obj: T,
+    ) -> InterpResult<'tcx, Id> {
+        let this = self.eval_context_mut();
+        let value_place =
+            this.deref_pointer_and_offset(lock_op, offset, lock_layout, this.machine.layouts.u32)?;
+
+        let new_index = get_objs(this).push(obj);
+        this.write_scalar(Scalar::from_u32(new_index.to_u32()), &value_place)?;
+        Ok(new_index)
+    }
+
     fn condvar_reacquire_mutex(
         &mut self,
         mutex: MutexId,
@@ -236,15 +289,53 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
 // situations.
 impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
 pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
+    /// Eagerly create and initialize a new mutex.
+    fn mutex_create(
+        &mut self,
+        lock_op: &OpTy<'tcx>,
+        lock_layout: TyAndLayout<'tcx>,
+        offset: u64,
+        data: Option<AdditionalMutexData>,
+    ) -> InterpResult<'tcx, MutexId> {
+        let this = self.eval_context_mut();
+        this.create_id(
+            lock_op,
+            lock_layout,
+            offset,
+            |ecx| &mut ecx.machine.sync.mutexes,
+            Mutex { data, ..Default::default() },
+        )
+    }
+
+    /// Lazily create a new mutex.
+    /// `initialize_data` must return any additional data that a user wants to associate with the mutex.
     fn mutex_get_or_create_id(
         &mut self,
         lock_op: &OpTy<'tcx>,
         lock_layout: TyAndLayout<'tcx>,
         offset: u64,
+        initialize_data: impl for<'a> FnOnce(
+            &'a mut MiriInterpCx<'tcx>,
+        ) -> InterpResult<'tcx, Option<AdditionalMutexData>>,
     ) -> InterpResult<'tcx, MutexId> {
         let this = self.eval_context_mut();
-        this.get_or_create_id(lock_op, lock_layout, offset, |ecx| &mut ecx.machine.sync.mutexes)?
-            .ok_or_else(|| err_ub_format!("mutex has invalid ID").into())
+        this.get_or_create_id(
+            lock_op,
+            lock_layout,
+            offset,
+            |ecx| &mut ecx.machine.sync.mutexes,
+            |ecx| initialize_data(ecx).map(|data| Mutex { data, ..Default::default() }),
+        )?
+        .ok_or_else(|| err_ub_format!("mutex has invalid ID").into())
+    }
+
+    /// 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(
@@ -254,8 +345,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         offset: u64,
     ) -> InterpResult<'tcx, RwLockId> {
         let this = self.eval_context_mut();
-        this.get_or_create_id(lock_op, lock_layout, offset, |ecx| &mut ecx.machine.sync.rwlocks)?
-            .ok_or_else(|| err_ub_format!("rwlock has invalid ID").into())
+        this.get_or_create_id(
+            lock_op,
+            lock_layout,
+            offset,
+            |ecx| &mut ecx.machine.sync.rwlocks,
+            |_| Ok(Default::default()),
+        )?
+        .ok_or_else(|| err_ub_format!("rwlock has invalid ID").into())
     }
 
     fn condvar_get_or_create_id(
@@ -265,8 +362,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         offset: u64,
     ) -> InterpResult<'tcx, CondvarId> {
         let this = self.eval_context_mut();
-        this.get_or_create_id(lock_op, lock_layout, offset, |ecx| &mut ecx.machine.sync.condvars)?
-            .ok_or_else(|| err_ub_format!("condvar has invalid ID").into())
+        this.get_or_create_id(
+            lock_op,
+            lock_layout,
+            offset,
+            |ecx| &mut ecx.machine.sync.condvars,
+            |_| Ok(Default::default()),
+        )?
+        .ok_or_else(|| err_ub_format!("condvar has invalid ID").into())
     }
 
     #[inline]

src/lib.rs

@@ -133,7 +133,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,

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))
     }
 }