Skip to content

Commit

Permalink
Auto merge of rust-lang#3784 - Mandragorian:detect_moved_mutexes, r=R…
Browse files Browse the repository at this point in the history
…alfJung

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 rust-lang#3749
  • Loading branch information
bors committed Sep 5, 2024
2 parents 989f63b + 5f22103 commit 18d358c
Show file tree
Hide file tree
Showing 8 changed files with 310 additions and 116 deletions.
10 changes: 8 additions & 2 deletions src/tools/miri/src/concurrency/init_once.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
119 changes: 111 additions & 8 deletions src/tools/miri/src/concurrency/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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);
Expand Down Expand Up @@ -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 =
Expand All @@ -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 {
Expand All @@ -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,
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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]
Expand Down
5 changes: 4 additions & 1 deletion src/tools/miri/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/src/shims/unix/macos/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}

Expand Down
Loading

0 comments on commit 18d358c

Please sign in to comment.