Skip to content

Commit

Permalink
Auto merge of #3973 - RalfJung:os-unfair-lock, r=RalfJung
Browse files Browse the repository at this point in the history 
ensure that a macOS os_unfair_lock that is moved while being held is not implicitly unlocked

Fixes rust-lang/miri#3859

We mark an os_unfair_lock that is moved while being held as "poisoned", which means it is not considered forever locked. That's not quite what the real implementation does, but allowing arbitrary moves-while-locked would likely expose a ton of implementation details, so hopefully this is good enough.
  • Loading branch information
@bors
bors committed Oct 14, 2024
2 parents 2b020bf + a802fd9 commit b7c06b4
Show file tree
Hide file tree
Showing 10 changed files with 297 additions and 122 deletions.
160 changes: 97 additions & 63 deletions src/tools/miri/src/concurrency/sync.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,75 +193,109 @@ impl<'tcx> AllocExtra<'tcx> {
/// If `init` is set to this, we consider the primitive initialized.
pub const LAZY_INIT_COOKIE: u32 = 0xcafe_affe;

/// Helper for lazily initialized `alloc_extra.sync` data:
/// this forces an immediate init.
pub fn lazy_sync_init<'tcx, T: 'static + Copy>(
ecx: &mut MiriInterpCx<'tcx>,
primitive: &MPlaceTy<'tcx>,
init_offset: Size,
data: T,
) -> InterpResult<'tcx> {
let (alloc, offset, _) = ecx.ptr_get_alloc_id(primitive.ptr(), 0)?;
let (alloc_extra, _machine) = ecx.get_alloc_extra_mut(alloc)?;
alloc_extra.sync.insert(offset, Box::new(data));
// Mark this as "initialized".
let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?;
ecx.write_scalar_atomic(
Scalar::from_u32(LAZY_INIT_COOKIE),
&init_field,
AtomicWriteOrd::Relaxed,
)?;
interp_ok(())
}

/// 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.
pub fn lazy_sync_get_data<'tcx, T: 'static + Copy>(
ecx: &mut MiriInterpCx<'tcx>,
primitive: &MPlaceTy<'tcx>,
init_offset: Size,
name: &str,
new_data: impl FnOnce(&mut MiriInterpCx<'tcx>) -> InterpResult<'tcx, T>,
) -> InterpResult<'tcx, T> {
// Check if this is already initialized. Needs to be atomic because we can race with another
// thread initializing. Needs to be an RMW operation to ensure we read the *latest* value.
// So we just try to replace MUTEX_INIT_COOKIE with itself.
let init_cookie = Scalar::from_u32(LAZY_INIT_COOKIE);
let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?;
let (_init, success) = ecx
.atomic_compare_exchange_scalar(
&init_field,
&ImmTy::from_scalar(init_cookie, ecx.machine.layouts.u32),
init_cookie,
AtomicRwOrd::Relaxed,
AtomicReadOrd::Relaxed,
/* can_fail_spuriously */ false,
)?
.to_scalar_pair();

if success.to_bool()? {
// If it is initialized, it must be found in the "sync primitive" table,
// or else it has been moved illegally.
let (alloc, offset, _) = ecx.ptr_get_alloc_id(primitive.ptr(), 0)?;
let alloc_extra = ecx.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)
} else {
let data = new_data(ecx)?;
lazy_sync_init(ecx, primitive, init_offset, data)?;
interp_ok(data)
}
}

// Public interface to synchronization primitives. Please note that in most
// cases, the function calls are infallible and it is the client's (shim
// implementation's) responsibility to detect and deal with erroneous
// situations.
impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
/// Helper for lazily initialized `alloc_extra.sync` data:
/// this forces an immediate init.
fn lazy_sync_init<T: 'static + Copy>(
&mut self,
primitive: &MPlaceTy<'tcx>,
init_offset: Size,
data: T,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();

let (alloc, offset, _) = this.ptr_get_alloc_id(primitive.ptr(), 0)?;
let (alloc_extra, _machine) = this.get_alloc_extra_mut(alloc)?;
alloc_extra.sync.insert(offset, Box::new(data));
// Mark this as "initialized".
let init_field = primitive.offset(init_offset, this.machine.layouts.u32, this)?;
this.write_scalar_atomic(
Scalar::from_u32(LAZY_INIT_COOKIE),
&init_field,
AtomicWriteOrd::Relaxed,
)?;
interp_ok(())
}

/// Helper for lazily initialized `alloc_extra.sync` data:
/// 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,
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();

// Check if this is already initialized. Needs to be atomic because we can race with another
// thread initializing. Needs to be an RMW operation to ensure we read the *latest* value.
// So we just try to replace MUTEX_INIT_COOKIE with itself.
let init_cookie = Scalar::from_u32(LAZY_INIT_COOKIE);
let init_field = primitive.offset(init_offset, this.machine.layouts.u32, this)?;
let (_init, success) = this
.atomic_compare_exchange_scalar(
&init_field,
&ImmTy::from_scalar(init_cookie, this.machine.layouts.u32),
init_cookie,
AtomicRwOrd::Relaxed,
AtomicReadOrd::Relaxed,
/* can_fail_spuriously */ false,
)?
.to_scalar_pair();

if success.to_bool()? {
// 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, _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)?;
interp_ok(data)
}
}

/// Get the synchronization primitive associated with the given pointer,
/// or initialize a new one.
fn get_sync_or_init<'a, T: 'static>(
&'a mut self,
ptr: Pointer,
new: impl FnOnce(&'a mut MiriMachine<'tcx>) -> InterpResult<'tcx, T>,
) -> InterpResult<'tcx, &'a T>
where
'tcx: 'a,
{
let this = self.eval_context_mut();
// Ensure there is memory behind this pointer, so that this allocation
// is truly the only place where the data could be stored.
this.check_ptr_access(ptr, Size::from_bytes(1), CheckInAllocMsg::InboundsTest)?;

let (alloc, offset, _) = this.ptr_get_alloc_id(ptr, 0)?;
let (alloc_extra, machine) = this.get_alloc_extra_mut(alloc)?;
// Due to borrow checker reasons, we have to do the lookup twice.
if alloc_extra.get_sync::<T>(offset).is_none() {
let new = new(machine)?;
alloc_extra.sync.insert(offset, Box::new(new));
}
interp_ok(alloc_extra.get_sync::<T>(offset).unwrap())
}

#[inline]
/// Get the id of the thread that currently owns this lock.
fn mutex_get_owner(&mut self, id: MutexId) -> ThreadId {
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/src/concurrency/thread.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down
102 changes: 83 additions & 19 deletions src/tools/miri/src/shims/unix/macos/sync.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +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 (alloc, offset, _) = this.ptr_get_alloc_id(lock.ptr(), 0)?;
let (alloc_extra, machine) = this.get_alloc_extra_mut(alloc)?;
if let Some(data) = alloc_extra.get_sync::<MacOsUnfairLock>(offset) {
interp_ok(data.id)
} else {
let id = machine.sync.mutex_create();
alloc_extra.sync.insert(offset, Box::new(MacOsUnfairLock { id }));
interp_ok(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 })
},
)
}
}

Expand All @@ -40,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.
Expand All @@ -64,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.
Expand All @@ -80,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(())
}
}
Loading

0 comments on commit b7c06b4

Please sign in to comment.