From 0c297f7ea7a5b7355bdaa2e930262ec36b030f89 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 12 Oct 2024 13:38:23 +0200 Subject: [PATCH 1/7] pthread_mutex: store mutex ID outside adressable memory, so it can be trusted --- src/concurrency/sync.rs | 16 +- src/helpers.rs | 7 +- src/machine.rs | 26 +- src/shims/unix/sync.rs | 257 +++++++++++------- .../libc_pthread_mutex_move.init.stderr | 4 +- .../concurrency/libc_pthread_mutex_move.rs | 4 +- ...hread_mutex_move.static_initializer.stderr | 4 +- 7 files changed, 190 insertions(+), 128 deletions(-) diff --git a/src/concurrency/sync.rs b/src/concurrency/sync.rs index e1e748e794..364f368f15 100644 --- a/src/concurrency/sync.rs +++ b/src/concurrency/sync.rs @@ -167,8 +167,10 @@ pub struct SynchronizationObjects { mutexes: IndexVec, rwlocks: IndexVec, condvars: IndexVec, - futexes: FxHashMap, pub(super) init_onces: IndexVec, + + /// Futex info for the futex at the given address. + futexes: FxHashMap, } // Private extension trait for local helper methods @@ -277,17 +279,9 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { 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: &MPlaceTy<'tcx>, - offset: u64, - data: Option>, - ) -> InterpResult<'tcx, MutexId> { + fn mutex_create(&mut self) -> MutexId { let this = self.eval_context_mut(); - this.create_id(lock, offset, |ecx| &mut ecx.machine.sync.mutexes, Mutex { - data, - ..Default::default() - }) + this.machine.sync.mutexes.push(Default::default()) } /// Lazily create a new mutex. diff --git a/src/helpers.rs b/src/helpers.rs index 8bd429deef..9ec325863f 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -223,14 +223,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } /// Evaluates the scalar at the specified path. - fn eval_path(&self, path: &[&str]) -> OpTy<'tcx> { + fn eval_path(&self, path: &[&str]) -> MPlaceTy<'tcx> { let this = self.eval_context_ref(); let instance = resolve_path(*this.tcx, path, Namespace::ValueNS); // We don't give a span -- this isn't actually used directly by the program anyway. - let const_val = this.eval_global(instance).unwrap_or_else(|err| { + this.eval_global(instance).unwrap_or_else(|err| { panic!("failed to evaluate required Rust item: {path:?}\n{err:?}") - }); - const_val.into() + }) } fn eval_path_scalar(&self, path: &[&str]) -> Scalar { let this = self.eval_context_ref(); diff --git a/src/machine.rs b/src/machine.rs index 0d28a4ed3e..60d096b92f 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -1,6 +1,7 @@ //! Global machine state as well as implementation of the interpreter engine //! `Machine` trait. +use std::any::Any; use std::borrow::Cow; use std::cell::RefCell; use std::collections::hash_map::Entry; @@ -321,7 +322,7 @@ impl ProvenanceExtra { } /// Extra per-allocation data -#[derive(Debug, Clone)] +#[derive(Debug)] pub struct AllocExtra<'tcx> { /// Global state of the borrow tracker, if enabled. pub borrow_tracker: Option, @@ -336,11 +337,24 @@ pub struct AllocExtra<'tcx> { /// if this allocation is leakable. The backtrace is not /// pruned yet; that should be done before printing it. pub backtrace: Option>>, + /// Synchronization primitives like to attach extra data to particular addresses. We store that + /// inside the relevant allocation, to ensure that everything is removed when the allocation is + /// freed. + /// This maps offsets to synchronization-primitive-specific data. + pub sync: FxHashMap>, +} + +// We need a `Clone` impl because the machine passes `Allocation` through `Cow`... +// but that should never end up actually cloning our `AllocExtra`. +impl<'tcx> Clone for AllocExtra<'tcx> { + fn clone(&self) -> Self { + panic!("our allocations should never be cloned"); + } } impl VisitProvenance for AllocExtra<'_> { fn visit_provenance(&self, visit: &mut VisitWith<'_>) { - let AllocExtra { borrow_tracker, data_race, weak_memory, backtrace: _ } = self; + let AllocExtra { borrow_tracker, data_race, weak_memory, backtrace: _, sync: _ } = self; borrow_tracker.visit_provenance(visit); data_race.visit_provenance(visit); @@ -1179,7 +1193,13 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { .insert(id, (ecx.machine.current_span(), None)); } - interp_ok(AllocExtra { borrow_tracker, data_race, weak_memory, backtrace }) + interp_ok(AllocExtra { + borrow_tracker, + data_race, + weak_memory, + backtrace, + sync: FxHashMap::default(), + }) } fn adjust_alloc_root_pointer( diff --git a/src/shims/unix/sync.rs b/src/shims/unix/sync.rs index b05f340861..86c75575f3 100644 --- a/src/shims/unix/sync.rs +++ b/src/shims/unix/sync.rs @@ -4,8 +4,37 @@ use rustc_target::abi::Size; use crate::*; -// pthread_mutexattr_t is either 4 or 8 bytes, depending on the platform. -// We ignore the platform layout and store our own fields: +/// Do a bytewise comparison of the two places, using relaxed atomic reads. +/// This is used to check if a mutex matches its static initializer value. +fn bytewise_equal_atomic_relaxed<'tcx>( + ecx: &MiriInterpCx<'tcx>, + left: &MPlaceTy<'tcx>, + right: &MPlaceTy<'tcx>, +) -> InterpResult<'tcx, bool> { + let size = left.layout.size; + assert_eq!(size, right.layout.size); + + // We do this in chunks of 4, so that we are okay to race with (sufficiently aligned) + // 4-byte atomic accesses. + assert!(size.bytes() % 4 == 0); + for i in 0..(size.bytes() / 4) { + let offset = Size::from_bytes(i.strict_mul(4)); + let load = |place: &MPlaceTy<'tcx>| { + let byte = place.offset(offset, ecx.machine.layouts.u32, ecx)?; + ecx.read_scalar_atomic(&byte, AtomicReadOrd::Relaxed)?.to_u32() + }; + let left = load(left)?; + let right = load(right)?; + if left != right { + return interp_ok(false); + } + } + + interp_ok(true) +} + +// # pthread_mutexattr_t +// We store some data directly inside the type, ignoring the platform layout: // - kind: i32 #[inline] @@ -49,52 +78,54 @@ fn mutexattr_set_kind<'tcx>( /// field *not* PTHREAD_MUTEX_DEFAULT but this special flag. const PTHREAD_MUTEX_KIND_UNCHANGED: i32 = 0x8000000; +// # pthread_mutex_t +// We store some data directly inside the type, ignoring the platform layout: +// - init: u32 + /// The mutex kind. #[derive(Debug, Clone, Copy)] -pub enum MutexKind { +enum MutexKind { Normal, Default, Recursive, ErrorCheck, } -#[derive(Debug)] +#[derive(Debug, Clone, Copy)] /// Additional data that we attach with each mutex instance. -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, +pub struct MutexData { + id: MutexId, + kind: MutexKind, } -// pthread_mutex_t is between 4 and 48 bytes, depending on the platform. -// We ignore the platform layout and store our own fields: -// - id: u32 +/// If `init` is set to this, we consider the mutex initialized. +const MUTEX_INIT_COOKIE: u32 = 0xcafe_affe; -fn mutex_id_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, u64> { - // When adding a new OS, make sure we also support all its static initializers in - // `mutex_kind_from_static_initializer`! +/// To ensure an initialized mutex that was moved somewhere else can be distinguished from +/// a statically initialized mutex that is used the first time, we pick some offset within +/// `pthread_mutex_t` and use it as an "initialized" flag. +fn mutex_init_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, Size> { let offset = match &*ecx.tcx.sess.target.os { "linux" | "illumos" | "solaris" | "freebsd" | "android" => 0, // macOS stores a signature in the first bytes, so we have to move to offset 4. "macos" => 4, os => throw_unsup_format!("`pthread_mutex` is not supported on {os}"), }; + let offset = Size::from_bytes(offset); // Sanity-check this against PTHREAD_MUTEX_INITIALIZER (but only once): - // the id must start out as 0. - // FIXME on some platforms (e.g linux) there are more static initializers for - // recursive or error checking mutexes. We should also add thme in this sanity check. + // the `init` field start out as `false`. static SANITY: AtomicBool = AtomicBool::new(false); if !SANITY.swap(true, Ordering::Relaxed) { let check_static_initializer = |name| { let static_initializer = ecx.eval_path(&["libc", name]); - let id_field = static_initializer - .offset(Size::from_bytes(offset), ecx.machine.layouts.u32, ecx) - .unwrap(); - let id = ecx.read_scalar(&id_field).unwrap().to_u32().unwrap(); - assert_eq!(id, 0, "{name} is incompatible with our pthread_mutex layout: id is not 0"); + let init_field = + static_initializer.offset(offset, ecx.machine.layouts.u32, ecx).unwrap(); + let init = ecx.read_scalar(&init_field).unwrap().to_u32().unwrap(); + assert_ne!( + init, MUTEX_INIT_COOKIE, + "{name} is incompatible with our pthread_mutex layout: `init` is equal to our cookie" + ); }; check_static_initializer("PTHREAD_MUTEX_INITIALIZER"); @@ -120,42 +151,69 @@ fn mutex_create<'tcx>( ecx: &mut MiriInterpCx<'tcx>, mutex_ptr: &OpTy<'tcx>, kind: MutexKind, -) -> InterpResult<'tcx> { +) -> InterpResult<'tcx, MutexId> { let mutex = ecx.deref_pointer(mutex_ptr)?; - let address = mutex.ptr().addr().bytes(); - let data = Box::new(AdditionalMutexData { address, kind }); - ecx.mutex_create(&mutex, mutex_id_offset(ecx)?, Some(data))?; - interp_ok(()) + let init_field = mutex.offset(mutex_init_offset(ecx)?, ecx.machine.layouts.u32, ecx)?; + + let id = ecx.mutex_create(); + let (alloc, offset, _) = ecx.ptr_get_alloc_id(mutex.ptr(), 0)?; + let (alloc_extra, _machine) = ecx.get_alloc_extra_mut(alloc)?; + alloc_extra.sync.insert(offset, Box::new(MutexData { id, kind })); + // Mark this as "initialized". + ecx.write_scalar_atomic( + Scalar::from_u32(MUTEX_INIT_COOKIE), + &init_field, + AtomicWriteOrd::Relaxed, + )?; + interp_ok(id) } /// Returns the `MutexId` of the mutex stored at `mutex_op`. /// /// `mutex_get_id` will also check if the mutex has been moved since its first use and /// return an error if it has. -fn mutex_get_id<'tcx>( - ecx: &mut MiriInterpCx<'tcx>, +fn mutex_get_data<'tcx, 'a>( + ecx: &'a mut MiriInterpCx<'tcx>, mutex_ptr: &OpTy<'tcx>, -) -> InterpResult<'tcx, MutexId> { +) -> InterpResult<'tcx, MutexData> { let mutex = ecx.deref_pointer(mutex_ptr)?; - let address = mutex.ptr().addr().bytes(); - - let id = ecx.mutex_get_or_create_id(&mutex, mutex_id_offset(ecx)?, |ecx| { - // This is called if a static initializer was used and the lock has not been assigned - // an ID yet. We have to determine the mutex kind from the static initializer. + let init_field = mutex.offset(mutex_init_offset(ecx)?, ecx.machine.layouts.u32, ecx)?; + + // 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(MUTEX_INIT_COOKIE); + let (_init, success) = ecx + .atomic_compare_exchange_scalar( + &init_field, + &ImmTy::from_scalar(init_cookie, ecx.machine.layouts.u32), + init_cookie, + AtomicRwOrd::Acquire, + AtomicReadOrd::Acquire, + /* 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(mutex.ptr(), 0)?; + let (alloc_extra, _machine) = ecx.get_alloc_extra_mut(alloc)?; + alloc_extra + .sync + .get(&offset) + .and_then(|s| s.downcast_ref::()) + .copied() + .ok_or_else(|| err_ub_format!("`pthread_mutex_t` can't be moved after first use")) + .into() + } else { + // Not yet initialized. This must be a static initializer, figure out the kind + // from that. We don't need to worry about races since we are the interpreter + // and don't let any other tread take a step. let kind = mutex_kind_from_static_initializer(ecx, &mutex)?; - - interp_ok(Some(Box::new(AdditionalMutexData { kind, address }))) - })?; - - // Check that the mutex has not been moved since last use. - let data = ecx - .mutex_get_data::(id) - .expect("data should always exist for pthreads"); - if data.address != address { - throw_ub_format!("pthread_mutex_t can't be moved after first use") + // And then create the mutex like this. + let id = mutex_create(ecx, mutex_ptr, kind)?; + interp_ok(MutexData { id, kind }) } - - interp_ok(id) } /// Returns the kind of a static initializer. @@ -163,25 +221,28 @@ fn mutex_kind_from_static_initializer<'tcx>( ecx: &MiriInterpCx<'tcx>, mutex: &MPlaceTy<'tcx>, ) -> InterpResult<'tcx, MutexKind> { - interp_ok(match &*ecx.tcx.sess.target.os { - // Only linux has static initializers other than PTHREAD_MUTEX_DEFAULT. - "linux" => { - let offset = if ecx.pointer_size().bytes() == 8 { 16 } else { 12 }; - let kind_place = - mutex.offset(Size::from_bytes(offset), ecx.machine.layouts.i32, ecx)?; - let kind = ecx.read_scalar(&kind_place)?.to_i32()?; - // Here we give PTHREAD_MUTEX_DEFAULT priority so that - // PTHREAD_MUTEX_INITIALIZER behaves like `pthread_mutex_init` with a NULL argument. - if kind == ecx.eval_libc_i32("PTHREAD_MUTEX_DEFAULT") { - MutexKind::Default - } else { - mutex_translate_kind(ecx, kind)? - } - } - _ => MutexKind::Default, - }) + // All the static initializers recognized here *must* be checked in `mutex_init_offset`! + let is_initializer = + |name| bytewise_equal_atomic_relaxed(ecx, mutex, &ecx.eval_path(&["libc", name])); + + // PTHREAD_MUTEX_INITIALIZER is recognized on all targets. + if is_initializer("PTHREAD_MUTEX_INITIALIZER")? { + return interp_ok(MutexKind::Default); + } + // Support additional platform-specific initializers. + match &*ecx.tcx.sess.target.os { + "linux" => + if is_initializer("PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP")? { + return interp_ok(MutexKind::Recursive); + } else if is_initializer("PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP")? { + return interp_ok(MutexKind::ErrorCheck); + }, + _ => {} + } + throw_unsup_format!("unsupported static initializer used for `pthread_mutex_t"); } +/// Translates the mutex kind from what is stored in pthread_mutexattr_t to our enum. fn mutex_translate_kind<'tcx>( ecx: &MiriInterpCx<'tcx>, kind: i32, @@ -203,8 +264,8 @@ fn mutex_translate_kind<'tcx>( }) } -// pthread_rwlock_t is between 4 and 56 bytes, depending on the platform. -// We ignore the platform layout and store our own fields: +// # pthread_rwlock_t +// We store some data directly inside the type, ignoring the platform layout: // - id: u32 #[derive(Debug)] @@ -262,8 +323,8 @@ fn rwlock_get_id<'tcx>( interp_ok(id) } -// pthread_condattr_t. -// We ignore the platform layout and store our own fields: +// # pthread_condattr_t +// We store some data directly inside the type, ignoring the platform layout: // - clock: i32 #[inline] @@ -315,8 +376,8 @@ fn condattr_set_clock_id<'tcx>( ) } -// pthread_cond_t can be only 4 bytes in size, depending on the platform. -// We ignore the platform layout and store our own fields: +// # pthread_cond_t +// We store some data directly inside the type, ignoring the platform layout: // - id: u32 fn cond_id_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, u64> { @@ -468,20 +529,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let id = mutex_get_id(this, mutex_op)?; - let kind = this - .mutex_get_data::(id) - .expect("data should always exist for pthread mutexes") - .kind; + let mutex = mutex_get_data(this, mutex_op)?; - let ret = if this.mutex_is_locked(id) { - let owner_thread = this.mutex_get_owner(id); + let ret = if this.mutex_is_locked(mutex.id) { + let owner_thread = this.mutex_get_owner(mutex.id); if owner_thread != this.active_thread() { - this.mutex_enqueue_and_block(id, Some((Scalar::from_i32(0), dest.clone()))); + this.mutex_enqueue_and_block(mutex.id, Some((Scalar::from_i32(0), dest.clone()))); return interp_ok(()); } else { // Trying to acquire the same mutex again. - match kind { + match mutex.kind { MutexKind::Default => throw_ub_format!( "trying to acquire default mutex already locked by the current thread" @@ -489,14 +546,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { MutexKind::Normal => throw_machine_stop!(TerminationInfo::Deadlock), MutexKind::ErrorCheck => this.eval_libc_i32("EDEADLK"), MutexKind::Recursive => { - this.mutex_lock(id); + this.mutex_lock(mutex.id); 0 } } } } else { // The mutex is unlocked. Let's lock it. - this.mutex_lock(id); + this.mutex_lock(mutex.id); 0 }; this.write_scalar(Scalar::from_i32(ret), dest)?; @@ -506,29 +563,25 @@ 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 id = mutex_get_id(this, mutex_op)?; - let kind = this - .mutex_get_data::(id) - .expect("data should always exist for pthread mutexes") - .kind; + let mutex = mutex_get_data(this, mutex_op)?; - interp_ok(Scalar::from_i32(if this.mutex_is_locked(id) { - let owner_thread = this.mutex_get_owner(id); + interp_ok(Scalar::from_i32(if this.mutex_is_locked(mutex.id) { + let owner_thread = this.mutex_get_owner(mutex.id); if owner_thread != this.active_thread() { this.eval_libc_i32("EBUSY") } else { - match kind { + match mutex.kind { MutexKind::Default | MutexKind::Normal | MutexKind::ErrorCheck => this.eval_libc_i32("EBUSY"), MutexKind::Recursive => { - this.mutex_lock(id); + this.mutex_lock(mutex.id); 0 } } } } else { // The mutex is unlocked. Let's lock it. - this.mutex_lock(id); + this.mutex_lock(mutex.id); 0 })) } @@ -536,20 +589,16 @@ 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 id = mutex_get_id(this, mutex_op)?; - let kind = this - .mutex_get_data::(id) - .expect("data should always exist for pthread mutexes") - .kind; + let mutex = mutex_get_data(this, mutex_op)?; - if let Some(_old_locked_count) = this.mutex_unlock(id)? { + if let Some(_old_locked_count) = this.mutex_unlock(mutex.id)? { // The mutex was locked by the current thread. interp_ok(Scalar::from_i32(0)) } else { // The mutex was locked by another thread or not locked at all. See // the “Unlock When Not Owner” column in // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutex_unlock.html. - match kind { + match mutex.kind { MutexKind::Default => throw_ub_format!( "unlocked a default mutex that was not locked by the current thread" @@ -569,9 +618,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Reading the field also has the side-effect that we detect double-`destroy` // since we make the field unint below. - let id = mutex_get_id(this, mutex_op)?; + let mutex = mutex_get_data(this, mutex_op)?; - if this.mutex_is_locked(id) { + if this.mutex_is_locked(mutex.id) { throw_ub_format!("destroyed a locked mutex"); } @@ -809,7 +858,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let this = self.eval_context_mut(); let id = cond_get_id(this, cond_op)?; - let mutex_id = mutex_get_id(this, mutex_op)?; + let mutex_id = mutex_get_data(this, mutex_op)?.id; this.condvar_wait( id, @@ -833,7 +882,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let this = self.eval_context_mut(); let id = cond_get_id(this, cond_op)?; - let mutex_id = mutex_get_id(this, mutex_op)?; + let mutex_id = mutex_get_data(this, mutex_op)?.id; // Extract the timeout. let clock_id = this diff --git a/tests/fail-dep/concurrency/libc_pthread_mutex_move.init.stderr b/tests/fail-dep/concurrency/libc_pthread_mutex_move.init.stderr index 15f397d4ac..7df8e8be58 100644 --- a/tests/fail-dep/concurrency/libc_pthread_mutex_move.init.stderr +++ b/tests/fail-dep/concurrency/libc_pthread_mutex_move.init.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: pthread_mutex_t can't be moved after first use +error: Undefined Behavior: `pthread_mutex_t` can't be moved after first use --> tests/fail-dep/concurrency/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 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `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 diff --git a/tests/fail-dep/concurrency/libc_pthread_mutex_move.rs b/tests/fail-dep/concurrency/libc_pthread_mutex_move.rs index c12a97a9ca..6c1f967b2b 100644 --- a/tests/fail-dep/concurrency/libc_pthread_mutex_move.rs +++ b/tests/fail-dep/concurrency/libc_pthread_mutex_move.rs @@ -12,7 +12,7 @@ fn check() { assert_eq!(libc::pthread_mutex_init(&mut m as *mut _, std::ptr::null()), 0); let mut m2 = m; // move the mutex - libc::pthread_mutex_lock(&mut m2 as *mut _); //~[init] ERROR: pthread_mutex_t can't be moved after first use + libc::pthread_mutex_lock(&mut m2 as *mut _); //~[init] ERROR: can't be moved after first use } } @@ -23,6 +23,6 @@ fn check() { libc::pthread_mutex_lock(&mut m as *mut _); let mut m2 = m; // move the mutex - libc::pthread_mutex_unlock(&mut m2 as *mut _); //~[static_initializer] ERROR: pthread_mutex_t can't be moved after first use + libc::pthread_mutex_unlock(&mut m2 as *mut _); //~[static_initializer] ERROR: can't be moved after first use } } diff --git a/tests/fail-dep/concurrency/libc_pthread_mutex_move.static_initializer.stderr b/tests/fail-dep/concurrency/libc_pthread_mutex_move.static_initializer.stderr index ebc253bf7a..acc018cb4b 100644 --- a/tests/fail-dep/concurrency/libc_pthread_mutex_move.static_initializer.stderr +++ b/tests/fail-dep/concurrency/libc_pthread_mutex_move.static_initializer.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: pthread_mutex_t can't be moved after first use +error: Undefined Behavior: `pthread_mutex_t` can't be moved after first use --> tests/fail-dep/concurrency/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 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `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 From 9dcb0b33711599670e36cc26bdaa702e334e5843 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 12 Oct 2024 14:07:36 +0200 Subject: [PATCH 2/7] pthread_rwlock: also store ID outside addressable memory --- src/concurrency/sync.rs | 48 +---- src/shims/unix/macos/sync.rs | 2 +- src/shims/unix/sync.rs | 191 ++++++++++-------- .../concurrency/libx_pthread_rwlock_moved.rs | 2 +- .../libx_pthread_rwlock_moved.stderr | 4 +- 5 files changed, 112 insertions(+), 135 deletions(-) diff --git a/src/concurrency/sync.rs b/src/concurrency/sync.rs index 364f368f15..b22684faec 100644 --- a/src/concurrency/sync.rs +++ b/src/concurrency/sync.rs @@ -79,9 +79,6 @@ struct Mutex { queue: VecDeque, /// Mutex clock. This tracks the moment of the last unlock. clock: VClock, - - /// Additional data that can be set by shim implementations. - data: Option>, } declare_id!(RwLockId); @@ -118,9 +115,6 @@ struct RwLock { /// locks. /// This is only relevant when there is an active reader. clock_current_readers: VClock, - - /// Additional data that can be set by shim implementations. - data: Option>, } declare_id!(CondvarId); @@ -290,56 +284,22 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { &mut self, lock: &MPlaceTy<'tcx>, offset: u64, - initialize_data: impl for<'a> FnOnce( - &'a mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, Option>>, ) -> InterpResult<'tcx, MutexId> { let this = self.eval_context_mut(); this.get_or_create_id( lock, offset, |ecx| &mut ecx.machine.sync.mutexes, - |ecx| initialize_data(ecx).map(|data| Mutex { data, ..Default::default() }), + |_ecx| interp_ok(Mutex::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, T: 'static>(&'a mut self, id: MutexId) -> Option<&'a T> - where - 'tcx: 'a, - { - let this = self.eval_context_ref(); - this.machine.sync.mutexes[id].data.as_deref().and_then(|p| p.downcast_ref::()) - } - - fn rwlock_get_or_create_id( - &mut self, - lock: &MPlaceTy<'tcx>, - offset: u64, - initialize_data: impl for<'a> FnOnce( - &'a mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, Option>>, - ) -> InterpResult<'tcx, RwLockId> { + /// Eagerly create and initialize a new rwlock. + fn rwlock_create(&mut self) -> RwLockId { let this = self.eval_context_mut(); - this.get_or_create_id( - lock, - offset, - |ecx| &mut ecx.machine.sync.rwlocks, - |ecx| initialize_data(ecx).map(|data| RwLock { data, ..Default::default() }), - )? - .ok_or_else(|| err_ub_format!("rwlock has invalid ID")) - .into() - } - - /// Retrieve the additional data stored for a rwlock. - fn rwlock_get_data<'a, T: 'static>(&'a mut self, id: RwLockId) -> Option<&'a T> - where - 'tcx: 'a, - { - let this = self.eval_context_ref(); - this.machine.sync.rwlocks[id].data.as_deref().and_then(|p| p.downcast_ref::()) + this.machine.sync.rwlocks.push(Default::default()) } /// Eagerly create and initialize a new condvar. diff --git a/src/shims/unix/macos/sync.rs b/src/shims/unix/macos/sync.rs index 2f96849d0d..1efe393a0e 100644 --- a/src/shims/unix/macos/sync.rs +++ b/src/shims/unix/macos/sync.rs @@ -20,7 +20,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, 0, |_| interp_ok(None)) + this.mutex_get_or_create_id(&lock, /* offset */ 0) } } diff --git a/src/shims/unix/sync.rs b/src/shims/unix/sync.rs index 86c75575f3..1a4c4094c5 100644 --- a/src/shims/unix/sync.rs +++ b/src/shims/unix/sync.rs @@ -33,6 +33,65 @@ fn bytewise_equal_atomic_relaxed<'tcx>( interp_ok(true) } +/// We designate an `init`` field in all primitives. +/// If `init` is set to this, we consider the primitive initialized. +const INIT_COOKIE: u32 = 0xcafe_affe; + +fn sync_create<'tcx, T: 'static + Copy>( + ecx: &mut MiriInterpCx<'tcx>, + primitive: &MPlaceTy<'tcx>, + init_offset: Size, + data: T, +) -> InterpResult<'tcx> { + let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?; + + 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". + ecx.write_scalar_atomic(Scalar::from_u32(INIT_COOKIE), &init_field, AtomicWriteOrd::Relaxed)?; + interp_ok(()) +} + +/// Checks if the primitive is initialized, and return its associated data if so. +/// Otherwise, return None. +fn sync_get_data<'tcx, T: 'static + Copy>( + ecx: &mut MiriInterpCx<'tcx>, + primitive: &MPlaceTy<'tcx>, + init_offset: Size, + name: &str, +) -> InterpResult<'tcx, Option> { + let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?; + // 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(INIT_COOKIE); + let (_init, success) = ecx + .atomic_compare_exchange_scalar( + &init_field, + &ImmTy::from_scalar(init_cookie, ecx.machine.layouts.u32), + init_cookie, + AtomicRwOrd::Acquire, + AtomicReadOrd::Acquire, + /* 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, _machine) = ecx.get_alloc_extra_mut(alloc)?; + let data = alloc_extra + .sync + .get(&offset) + .and_then(|s| s.downcast_ref::()) + .ok_or_else(|| err_ub_format!("`{name}` can't be moved after first use"))?; + interp_ok(Some(*data)) + } else { + interp_ok(None) + } +} + // # pthread_mutexattr_t // We store some data directly inside the type, ignoring the platform layout: // - kind: i32 @@ -98,23 +157,20 @@ pub struct MutexData { kind: MutexKind, } -/// If `init` is set to this, we consider the mutex initialized. -const MUTEX_INIT_COOKIE: u32 = 0xcafe_affe; - /// To ensure an initialized mutex that was moved somewhere else can be distinguished from /// a statically initialized mutex that is used the first time, we pick some offset within /// `pthread_mutex_t` and use it as an "initialized" flag. fn mutex_init_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, Size> { let offset = match &*ecx.tcx.sess.target.os { "linux" | "illumos" | "solaris" | "freebsd" | "android" => 0, - // macOS stores a signature in the first bytes, so we have to move to offset 4. + // macOS stores a signature in the first bytes, so we move to offset 4. "macos" => 4, os => throw_unsup_format!("`pthread_mutex` is not supported on {os}"), }; let offset = Size::from_bytes(offset); // Sanity-check this against PTHREAD_MUTEX_INITIALIZER (but only once): - // the `init` field start out as `false`. + // the `init` field must start out not equal to MUTEX_INIT_COOKIE. static SANITY: AtomicBool = AtomicBool::new(false); if !SANITY.swap(true, Ordering::Relaxed) { let check_static_initializer = |name| { @@ -122,10 +178,7 @@ fn mutex_init_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, Size> let init_field = static_initializer.offset(offset, ecx.machine.layouts.u32, ecx).unwrap(); let init = ecx.read_scalar(&init_field).unwrap().to_u32().unwrap(); - assert_ne!( - init, MUTEX_INIT_COOKIE, - "{name} is incompatible with our pthread_mutex layout: `init` is equal to our cookie" - ); + assert_ne!(init, INIT_COOKIE, "{name} is incompatible with our initialization cookie"); }; check_static_initializer("PTHREAD_MUTEX_INITIALIZER"); @@ -151,21 +204,12 @@ fn mutex_create<'tcx>( ecx: &mut MiriInterpCx<'tcx>, mutex_ptr: &OpTy<'tcx>, kind: MutexKind, -) -> InterpResult<'tcx, MutexId> { +) -> InterpResult<'tcx, MutexData> { let mutex = ecx.deref_pointer(mutex_ptr)?; - let init_field = mutex.offset(mutex_init_offset(ecx)?, ecx.machine.layouts.u32, ecx)?; - let id = ecx.mutex_create(); - let (alloc, offset, _) = ecx.ptr_get_alloc_id(mutex.ptr(), 0)?; - let (alloc_extra, _machine) = ecx.get_alloc_extra_mut(alloc)?; - alloc_extra.sync.insert(offset, Box::new(MutexData { id, kind })); - // Mark this as "initialized". - ecx.write_scalar_atomic( - Scalar::from_u32(MUTEX_INIT_COOKIE), - &init_field, - AtomicWriteOrd::Relaxed, - )?; - interp_ok(id) + let data = MutexData { id, kind }; + sync_create(ecx, &mutex, mutex_init_offset(ecx)?, data)?; + interp_ok(data) } /// Returns the `MutexId` of the mutex stored at `mutex_op`. @@ -177,42 +221,18 @@ fn mutex_get_data<'tcx, 'a>( mutex_ptr: &OpTy<'tcx>, ) -> InterpResult<'tcx, MutexData> { let mutex = ecx.deref_pointer(mutex_ptr)?; - let init_field = mutex.offset(mutex_init_offset(ecx)?, ecx.machine.layouts.u32, ecx)?; - // 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(MUTEX_INIT_COOKIE); - let (_init, success) = ecx - .atomic_compare_exchange_scalar( - &init_field, - &ImmTy::from_scalar(init_cookie, ecx.machine.layouts.u32), - init_cookie, - AtomicRwOrd::Acquire, - AtomicReadOrd::Acquire, - /* 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(mutex.ptr(), 0)?; - let (alloc_extra, _machine) = ecx.get_alloc_extra_mut(alloc)?; - alloc_extra - .sync - .get(&offset) - .and_then(|s| s.downcast_ref::()) - .copied() - .ok_or_else(|| err_ub_format!("`pthread_mutex_t` can't be moved after first use")) - .into() + if let Some(data) = + sync_get_data::(ecx, &mutex, mutex_init_offset(ecx)?, "pthread_mutex_t")? + { + interp_ok(data) } else { // Not yet initialized. This must be a static initializer, figure out the kind // from that. We don't need to worry about races since we are the interpreter // and don't let any other tread take a step. let kind = mutex_kind_from_static_initializer(ecx, &mutex)?; // And then create the mutex like this. - let id = mutex_create(ecx, mutex_ptr, kind)?; - interp_ok(MutexData { id, kind }) + mutex_create(ecx, mutex_ptr, kind) } } @@ -266,61 +286,58 @@ fn mutex_translate_kind<'tcx>( // # pthread_rwlock_t // We store some data directly inside the type, ignoring the platform layout: -// - id: u32 +// - init: u32 -#[derive(Debug)] +/// If `init` is set to this, we consider the rwlock initialized. +const RWLOCK_INIT_COOKIE: u32 = 0xcafe_affe; + +#[derive(Debug, Copy, Clone)] /// Additional data that we attach with each rwlock instance. -pub struct AdditionalRwLockData { - /// The address of the rwlock. - pub address: u64, +pub struct RwLockData { + id: RwLockId, } -fn rwlock_id_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, u64> { +fn rwlock_init_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, Size> { let offset = match &*ecx.tcx.sess.target.os { "linux" | "illumos" | "solaris" | "freebsd" | "android" => 0, - // macOS stores a signature in the first bytes, so we have to move to offset 4. + // macOS stores a signature in the first bytes, so we move to offset 4. "macos" => 4, os => throw_unsup_format!("`pthread_rwlock` is not supported on {os}"), }; + let offset = Size::from_bytes(offset); // Sanity-check this against PTHREAD_RWLOCK_INITIALIZER (but only once): - // the id must start out as 0. + // the `init` field must start out not equal to RWLOCK_INIT_COOKIE. static SANITY: AtomicBool = AtomicBool::new(false); if !SANITY.swap(true, Ordering::Relaxed) { let static_initializer = ecx.eval_path(&["libc", "PTHREAD_RWLOCK_INITIALIZER"]); - let id_field = static_initializer - .offset(Size::from_bytes(offset), ecx.machine.layouts.u32, ecx) - .unwrap(); - let id = ecx.read_scalar(&id_field).unwrap().to_u32().unwrap(); - assert_eq!( - id, 0, - "PTHREAD_RWLOCK_INITIALIZER is incompatible with our pthread_rwlock layout: id is not 0" + let init_field = static_initializer.offset(offset, ecx.machine.layouts.u32, ecx).unwrap(); + let init = ecx.read_scalar(&init_field).unwrap().to_u32().unwrap(); + assert_ne!( + init, RWLOCK_INIT_COOKIE, + "PTHREAD_RWLOCK_INITIALIZER is incompatible with our initialization cookie" ); } interp_ok(offset) } -fn rwlock_get_id<'tcx>( +fn rwlock_get_data<'tcx>( ecx: &mut MiriInterpCx<'tcx>, rwlock_ptr: &OpTy<'tcx>, -) -> InterpResult<'tcx, RwLockId> { +) -> InterpResult<'tcx, RwLockData> { let rwlock = ecx.deref_pointer(rwlock_ptr)?; - let address = rwlock.ptr().addr().bytes(); - - let id = ecx.rwlock_get_or_create_id(&rwlock, rwlock_id_offset(ecx)?, |_| { - interp_ok(Some(Box::new(AdditionalRwLockData { address }))) - })?; + let init_offset = rwlock_init_offset(ecx)?; - // Check that the rwlock has not been moved since last use. - let data = ecx - .rwlock_get_data::(id) - .expect("data should always exist for pthreads"); - if data.address != address { - throw_ub_format!("pthread_rwlock_t can't be moved after first use") + if let Some(data) = sync_get_data::(ecx, &rwlock, init_offset, "pthread_rwlock_t")? + { + interp_ok(data) + } else { + let id = ecx.rwlock_create(); + let data = RwLockData { id }; + sync_create(ecx, &rwlock, init_offset, data)?; + interp_ok(data) } - - interp_ok(id) } // # pthread_condattr_t @@ -640,7 +657,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let id = rwlock_get_id(this, rwlock_op)?; + let id = rwlock_get_data(this, rwlock_op)?.id; if this.rwlock_is_write_locked(id) { this.rwlock_enqueue_and_block_reader(id, Scalar::from_i32(0), dest.clone()); @@ -655,7 +672,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn pthread_rwlock_tryrdlock(&mut self, rwlock_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let id = rwlock_get_id(this, rwlock_op)?; + let id = rwlock_get_data(this, rwlock_op)?.id; if this.rwlock_is_write_locked(id) { interp_ok(Scalar::from_i32(this.eval_libc_i32("EBUSY"))) @@ -672,7 +689,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let id = rwlock_get_id(this, rwlock_op)?; + let id = rwlock_get_data(this, rwlock_op)?.id; if this.rwlock_is_locked(id) { // Note: this will deadlock if the lock is already locked by this @@ -699,7 +716,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn pthread_rwlock_trywrlock(&mut self, rwlock_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let id = rwlock_get_id(this, rwlock_op)?; + let id = rwlock_get_data(this, rwlock_op)?.id; if this.rwlock_is_locked(id) { interp_ok(Scalar::from_i32(this.eval_libc_i32("EBUSY"))) @@ -712,7 +729,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn pthread_rwlock_unlock(&mut self, rwlock_op: &OpTy<'tcx>) -> InterpResult<'tcx, ()> { let this = self.eval_context_mut(); - let id = rwlock_get_id(this, rwlock_op)?; + let id = rwlock_get_data(this, rwlock_op)?.id; #[allow(clippy::if_same_then_else)] if this.rwlock_reader_unlock(id)? || this.rwlock_writer_unlock(id)? { @@ -727,7 +744,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Reading the field also has the side-effect that we detect double-`destroy` // since we make the field unint below. - let id = rwlock_get_id(this, rwlock_op)?; + let id = rwlock_get_data(this, rwlock_op)?.id; if this.rwlock_is_locked(id) { throw_ub_format!("destroyed a locked rwlock"); diff --git a/tests/fail-dep/concurrency/libx_pthread_rwlock_moved.rs b/tests/fail-dep/concurrency/libx_pthread_rwlock_moved.rs index 540729962a..6af19b7df9 100644 --- a/tests/fail-dep/concurrency/libx_pthread_rwlock_moved.rs +++ b/tests/fail-dep/concurrency/libx_pthread_rwlock_moved.rs @@ -9,6 +9,6 @@ fn main() { // Move rwlock let mut rw2 = rw; - libc::pthread_rwlock_unlock(&mut rw2 as *mut _); //~ ERROR: pthread_rwlock_t can't be moved after first use + libc::pthread_rwlock_unlock(&mut rw2 as *mut _); //~ ERROR: can't be moved after first use } } diff --git a/tests/fail-dep/concurrency/libx_pthread_rwlock_moved.stderr b/tests/fail-dep/concurrency/libx_pthread_rwlock_moved.stderr index ce08fa8159..fbc9119f11 100644 --- a/tests/fail-dep/concurrency/libx_pthread_rwlock_moved.stderr +++ b/tests/fail-dep/concurrency/libx_pthread_rwlock_moved.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: pthread_rwlock_t can't be moved after first use +error: Undefined Behavior: `pthread_rwlock_t` can't be moved after first use --> tests/fail-dep/concurrency/libx_pthread_rwlock_moved.rs:LL:CC | LL | libc::pthread_rwlock_unlock(&mut rw2 as *mut _); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pthread_rwlock_t can't be moved after first use + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `pthread_rwlock_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 From e48beb02546062ad27719b161805050a69c05b54 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 12 Oct 2024 14:24:23 +0200 Subject: [PATCH 3/7] pthread_cond: also store ID outside addressable memory --- src/concurrency/sync.rs | 70 +----- src/shims/unix/sync.rs | 199 +++++++++--------- .../libc_pthread_cond_move.init.stderr | 4 +- .../concurrency/libc_pthread_cond_move.rs | 4 +- ...thread_cond_move.static_initializer.stderr | 4 +- 5 files changed, 109 insertions(+), 172 deletions(-) diff --git a/src/concurrency/sync.rs b/src/concurrency/sync.rs index b22684faec..2901cb9ccb 100644 --- a/src/concurrency/sync.rs +++ b/src/concurrency/sync.rs @@ -1,4 +1,3 @@ -use std::any::Any; use std::collections::VecDeque; use std::collections::hash_map::Entry; use std::ops::Not; @@ -129,9 +128,6 @@ struct Condvar { /// Contains the clock of the last thread to /// perform a condvar-signal. clock: VClock, - - /// Additional data that can be set by shim implementations. - data: Option>, } /// The futex state. @@ -220,32 +216,6 @@ 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( - &mut self, - lock: &MPlaceTy<'tcx>, - offset: u64, - get_objs: impl for<'a> Fn(&'a mut MiriInterpCx<'tcx>) -> &'a mut IndexVec, - obj: T, - ) -> InterpResult<'tcx, Id> { - let this = self.eval_context_mut(); - let offset = Size::from_bytes(offset); - assert!(lock.layout.size >= offset + this.machine.layouts.u32.size); - let id_place = lock.offset(offset, this.machine.layouts.u32, this)?; - - let new_index = get_objs(this).push(obj); - this.write_scalar(Scalar::from_u32(new_index.to_u32()), &id_place)?; - interp_ok(new_index) - } - fn condvar_reacquire_mutex( &mut self, mutex: MutexId, @@ -303,45 +273,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } /// Eagerly create and initialize a new condvar. - fn condvar_create( - &mut self, - condvar: &MPlaceTy<'tcx>, - offset: u64, - data: Option>, - ) -> InterpResult<'tcx, CondvarId> { + fn condvar_create(&mut self) -> CondvarId { let this = self.eval_context_mut(); - this.create_id(condvar, offset, |ecx| &mut ecx.machine.sync.condvars, Condvar { - data, - ..Default::default() - }) - } - - fn condvar_get_or_create_id( - &mut self, - lock: &MPlaceTy<'tcx>, - offset: u64, - initialize_data: impl for<'a> FnOnce( - &'a mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, Option>>, - ) -> InterpResult<'tcx, CondvarId> { - let this = self.eval_context_mut(); - this.get_or_create_id( - lock, - offset, - |ecx| &mut ecx.machine.sync.condvars, - |ecx| initialize_data(ecx).map(|data| Condvar { data, ..Default::default() }), - )? - .ok_or_else(|| err_ub_format!("condvar has invalid ID")) - .into() - } - - /// Retrieve the additional data stored for a condvar. - fn condvar_get_data<'a, T: 'static>(&'a mut self, id: CondvarId) -> Option<&'a T> - where - 'tcx: 'a, - { - let this = self.eval_context_ref(); - this.machine.sync.condvars[id].data.as_deref().and_then(|p| p.downcast_ref::()) + this.machine.sync.condvars.push(Default::default()) } #[inline] diff --git a/src/shims/unix/sync.rs b/src/shims/unix/sync.rs index 1a4c4094c5..1942f25996 100644 --- a/src/shims/unix/sync.rs +++ b/src/shims/unix/sync.rs @@ -37,7 +37,7 @@ fn bytewise_equal_atomic_relaxed<'tcx>( /// If `init` is set to this, we consider the primitive initialized. const INIT_COOKIE: u32 = 0xcafe_affe; -fn sync_create<'tcx, T: 'static + Copy>( +fn sync_init<'tcx, T: 'static + Copy>( ecx: &mut MiriInterpCx<'tcx>, primitive: &MPlaceTy<'tcx>, init_offset: Size, @@ -137,6 +137,28 @@ fn mutexattr_set_kind<'tcx>( /// field *not* PTHREAD_MUTEX_DEFAULT but this special flag. const PTHREAD_MUTEX_KIND_UNCHANGED: i32 = 0x8000000; +/// Translates the mutex kind from what is stored in pthread_mutexattr_t to our enum. +fn mutexattr_translate_kind<'tcx>( + ecx: &MiriInterpCx<'tcx>, + kind: i32, +) -> InterpResult<'tcx, MutexKind> { + interp_ok(if kind == (ecx.eval_libc_i32("PTHREAD_MUTEX_NORMAL")) { + MutexKind::Normal + } else if kind == ecx.eval_libc_i32("PTHREAD_MUTEX_ERRORCHECK") { + MutexKind::ErrorCheck + } else if kind == ecx.eval_libc_i32("PTHREAD_MUTEX_RECURSIVE") { + MutexKind::Recursive + } else if kind == ecx.eval_libc_i32("PTHREAD_MUTEX_DEFAULT") + || kind == PTHREAD_MUTEX_KIND_UNCHANGED + { + // We check this *last* since PTHREAD_MUTEX_DEFAULT may be numerically equal to one of the + // others, and we want an explicit `mutexattr_settype` to work as expected. + MutexKind::Default + } else { + throw_unsup_format!("unsupported type of mutex: {kind}"); + }) +} + // # pthread_mutex_t // We store some data directly inside the type, ignoring the platform layout: // - init: u32 @@ -170,7 +192,7 @@ fn mutex_init_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, Size> let offset = Size::from_bytes(offset); // Sanity-check this against PTHREAD_MUTEX_INITIALIZER (but only once): - // the `init` field must start out not equal to MUTEX_INIT_COOKIE. + // the `init` field must start out not equal to INIT_COOKIE. static SANITY: AtomicBool = AtomicBool::new(false); if !SANITY.swap(true, Ordering::Relaxed) { let check_static_initializer = |name| { @@ -208,7 +230,7 @@ fn mutex_create<'tcx>( let mutex = ecx.deref_pointer(mutex_ptr)?; let id = ecx.mutex_create(); let data = MutexData { id, kind }; - sync_create(ecx, &mutex, mutex_init_offset(ecx)?, data)?; + sync_init(ecx, &mutex, mutex_init_offset(ecx)?, data)?; interp_ok(data) } @@ -259,38 +281,13 @@ fn mutex_kind_from_static_initializer<'tcx>( }, _ => {} } - throw_unsup_format!("unsupported static initializer used for `pthread_mutex_t"); -} - -/// Translates the mutex kind from what is stored in pthread_mutexattr_t to our enum. -fn mutex_translate_kind<'tcx>( - ecx: &MiriInterpCx<'tcx>, - kind: i32, -) -> InterpResult<'tcx, MutexKind> { - interp_ok(if kind == (ecx.eval_libc_i32("PTHREAD_MUTEX_NORMAL")) { - MutexKind::Normal - } else if kind == ecx.eval_libc_i32("PTHREAD_MUTEX_ERRORCHECK") { - MutexKind::ErrorCheck - } else if kind == ecx.eval_libc_i32("PTHREAD_MUTEX_RECURSIVE") { - MutexKind::Recursive - } else if kind == ecx.eval_libc_i32("PTHREAD_MUTEX_DEFAULT") - || kind == PTHREAD_MUTEX_KIND_UNCHANGED - { - // We check this *last* since PTHREAD_MUTEX_DEFAULT may be numerically equal to one of the - // others, and we want an explicit `mutexattr_settype` to work as expected. - MutexKind::Default - } else { - throw_unsup_format!("unsupported type of mutex: {kind}"); - }) + throw_unsup_format!("unsupported static initializer used for `pthread_mutex_t`"); } // # pthread_rwlock_t // We store some data directly inside the type, ignoring the platform layout: // - init: u32 -/// If `init` is set to this, we consider the rwlock initialized. -const RWLOCK_INIT_COOKIE: u32 = 0xcafe_affe; - #[derive(Debug, Copy, Clone)] /// Additional data that we attach with each rwlock instance. pub struct RwLockData { @@ -307,14 +304,14 @@ fn rwlock_init_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, Size let offset = Size::from_bytes(offset); // Sanity-check this against PTHREAD_RWLOCK_INITIALIZER (but only once): - // the `init` field must start out not equal to RWLOCK_INIT_COOKIE. + // the `init` field must start out not equal to INIT_COOKIE. static SANITY: AtomicBool = AtomicBool::new(false); if !SANITY.swap(true, Ordering::Relaxed) { let static_initializer = ecx.eval_path(&["libc", "PTHREAD_RWLOCK_INITIALIZER"]); let init_field = static_initializer.offset(offset, ecx.machine.layouts.u32, ecx).unwrap(); let init = ecx.read_scalar(&init_field).unwrap().to_u32().unwrap(); assert_ne!( - init, RWLOCK_INIT_COOKIE, + init, INIT_COOKIE, "PTHREAD_RWLOCK_INITIALIZER is incompatible with our initialization cookie" ); } @@ -333,9 +330,16 @@ fn rwlock_get_data<'tcx>( { interp_ok(data) } else { + if !bytewise_equal_atomic_relaxed( + ecx, + &rwlock, + &ecx.eval_path(&["libc", "PTHREAD_RWLOCK_INITIALIZER"]), + )? { + throw_unsup_format!("unsupported static initializer used for `pthread_rwlock_t`"); + } let id = ecx.rwlock_create(); let data = RwLockData { id }; - sync_create(ecx, &rwlock, init_offset, data)?; + sync_init(ecx, &rwlock, init_offset, data)?; interp_ok(data) } } @@ -366,19 +370,6 @@ fn condattr_get_clock_id<'tcx>( .to_i32() } -fn cond_translate_clock_id<'tcx>( - ecx: &MiriInterpCx<'tcx>, - raw_id: i32, -) -> InterpResult<'tcx, ClockId> { - interp_ok(if raw_id == ecx.eval_libc_i32("CLOCK_REALTIME") { - ClockId::Realtime - } else if raw_id == ecx.eval_libc_i32("CLOCK_MONOTONIC") { - ClockId::Monotonic - } else { - throw_unsup_format!("unsupported clock id: {raw_id}"); - }) -} - fn condattr_set_clock_id<'tcx>( ecx: &mut MiriInterpCx<'tcx>, attr_ptr: &OpTy<'tcx>, @@ -393,30 +384,43 @@ fn condattr_set_clock_id<'tcx>( ) } +/// Translates the clock from what is stored in pthread_condattr_t to our enum. +fn condattr_translate_clock_id<'tcx>( + ecx: &MiriInterpCx<'tcx>, + raw_id: i32, +) -> InterpResult<'tcx, ClockId> { + interp_ok(if raw_id == ecx.eval_libc_i32("CLOCK_REALTIME") { + ClockId::Realtime + } else if raw_id == ecx.eval_libc_i32("CLOCK_MONOTONIC") { + ClockId::Monotonic + } else { + throw_unsup_format!("unsupported clock id: {raw_id}"); + }) +} + // # pthread_cond_t // We store some data directly inside the type, ignoring the platform layout: -// - id: u32 +// - init: u32 -fn cond_id_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, u64> { +fn cond_init_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, Size> { let offset = match &*ecx.tcx.sess.target.os { "linux" | "illumos" | "solaris" | "freebsd" | "android" => 0, - // macOS stores a signature in the first bytes, so we have to move to offset 4. + // macOS stores a signature in the first bytes, so we move to offset 4. "macos" => 4, os => throw_unsup_format!("`pthread_cond` is not supported on {os}"), }; + let offset = Size::from_bytes(offset); // Sanity-check this against PTHREAD_COND_INITIALIZER (but only once): - // the id must start out as 0. + // the `init` field must start out not equal to INIT_COOKIE. static SANITY: AtomicBool = AtomicBool::new(false); if !SANITY.swap(true, Ordering::Relaxed) { let static_initializer = ecx.eval_path(&["libc", "PTHREAD_COND_INITIALIZER"]); - let id_field = static_initializer - .offset(Size::from_bytes(offset), ecx.machine.layouts.u32, ecx) - .unwrap(); - let id = ecx.read_scalar(&id_field).unwrap().to_u32().unwrap(); - assert_eq!( - id, 0, - "PTHREAD_COND_INITIALIZER is incompatible with our pthread_cond layout: id is not 0" + let init_field = static_initializer.offset(offset, ecx.machine.layouts.u32, ecx).unwrap(); + let init = ecx.read_scalar(&init_field).unwrap().to_u32().unwrap(); + assert_ne!( + init, INIT_COOKIE, + "PTHREAD_COND_INITIALIZER is incompatible with our initialization cookie" ); } @@ -429,36 +433,45 @@ enum ClockId { Monotonic, } -#[derive(Debug)] +#[derive(Debug, Copy, Clone)] /// Additional data that we attach with each cond instance. -struct AdditionalCondData { - /// The address of the cond. - address: u64, +struct CondData { + id: CondvarId, + clock: ClockId, +} - /// The clock id of the cond. - clock_id: ClockId, +fn cond_create<'tcx>( + ecx: &mut MiriInterpCx<'tcx>, + cond_ptr: &OpTy<'tcx>, + clock: ClockId, +) -> InterpResult<'tcx, CondData> { + let cond = ecx.deref_pointer(cond_ptr)?; + let id = ecx.condvar_create(); + let data = CondData { id, clock }; + sync_init(ecx, &cond, cond_init_offset(ecx)?, data)?; + interp_ok(data) } -fn cond_get_id<'tcx>( +fn cond_get_data<'tcx>( ecx: &mut MiriInterpCx<'tcx>, cond_ptr: &OpTy<'tcx>, -) -> InterpResult<'tcx, CondvarId> { +) -> InterpResult<'tcx, CondData> { let cond = ecx.deref_pointer(cond_ptr)?; - let address = cond.ptr().addr().bytes(); - let id = ecx.condvar_get_or_create_id(&cond, cond_id_offset(ecx)?, |_ecx| { - // This used the static initializer. The clock there is always CLOCK_REALTIME. - interp_ok(Some(Box::new(AdditionalCondData { address, clock_id: ClockId::Realtime }))) - })?; + let init_offset = cond_init_offset(ecx)?; - // Check that the mutex has not been moved since last use. - let data = ecx - .condvar_get_data::(id) - .expect("data should always exist for pthreads"); - if data.address != address { - throw_ub_format!("pthread_cond_t can't be moved after first use") + if let Some(data) = sync_get_data::(ecx, &cond, init_offset, "pthread_cond_t")? { + interp_ok(data) + } else { + // This used the static initializer. The clock there is always CLOCK_REALTIME. + if !bytewise_equal_atomic_relaxed( + ecx, + &cond, + &ecx.eval_path(&["libc", "PTHREAD_COND_INITIALIZER"]), + )? { + throw_unsup_format!("unsupported static initializer used for `pthread_cond_t`"); + } + cond_create(ecx, cond_ptr, ClockId::Realtime) } - - interp_ok(id) } impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} @@ -531,7 +544,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let kind = if this.ptr_is_null(attr)? { MutexKind::Default } else { - mutex_translate_kind(this, mutexattr_get_kind(this, attr_op)?)? + mutexattr_translate_kind(this, mutexattr_get_kind(this, attr_op)?)? }; mutex_create(this, mutex_op, kind)?; @@ -839,29 +852,23 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } else { condattr_get_clock_id(this, attr_op)? }; - let clock_id = cond_translate_clock_id(this, clock_id)?; + let clock_id = condattr_translate_clock_id(this, clock_id)?; - let cond = this.deref_pointer(cond_op)?; - let address = cond.ptr().addr().bytes(); - this.condvar_create( - &cond, - cond_id_offset(this)?, - Some(Box::new(AdditionalCondData { address, clock_id })), - )?; + cond_create(this, cond_op, clock_id)?; interp_ok(()) } fn pthread_cond_signal(&mut self, cond_op: &OpTy<'tcx>) -> InterpResult<'tcx, ()> { let this = self.eval_context_mut(); - let id = cond_get_id(this, cond_op)?; + let id = cond_get_data(this, cond_op)?.id; this.condvar_signal(id)?; interp_ok(()) } fn pthread_cond_broadcast(&mut self, cond_op: &OpTy<'tcx>) -> InterpResult<'tcx, ()> { let this = self.eval_context_mut(); - let id = cond_get_id(this, cond_op)?; + let id = cond_get_data(this, cond_op)?.id; while this.condvar_signal(id)? {} interp_ok(()) } @@ -874,11 +881,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let id = cond_get_id(this, cond_op)?; + let data = cond_get_data(this, cond_op)?; let mutex_id = mutex_get_data(this, mutex_op)?.id; this.condvar_wait( - id, + data.id, mutex_id, None, // no timeout Scalar::from_i32(0), @@ -898,14 +905,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let id = cond_get_id(this, cond_op)?; + let data = cond_get_data(this, cond_op)?; let mutex_id = mutex_get_data(this, mutex_op)?.id; // Extract the timeout. - let clock_id = this - .condvar_get_data::(id) - .expect("additional data should always be present for pthreads") - .clock_id; let duration = match this .read_timespec(&this.deref_pointer_as(abstime_op, this.libc_ty_layout("timespec"))?)? { @@ -916,7 +919,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { return interp_ok(()); } }; - let timeout_clock = match clock_id { + let timeout_clock = match data.clock { ClockId::Realtime => { this.check_no_isolation("`pthread_cond_timedwait` with `CLOCK_REALTIME`")?; TimeoutClock::RealTime @@ -925,7 +928,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; this.condvar_wait( - id, + data.id, mutex_id, Some((timeout_clock, TimeoutAnchor::Absolute, duration)), Scalar::from_i32(0), @@ -941,7 +944,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Reading the field also has the side-effect that we detect double-`destroy` // since we make the field unint below. - let id = cond_get_id(this, cond_op)?; + let id = cond_get_data(this, cond_op)?.id; if this.condvar_is_awaited(id) { throw_ub_format!("destroying an awaited conditional variable"); } diff --git a/tests/fail-dep/concurrency/libc_pthread_cond_move.init.stderr b/tests/fail-dep/concurrency/libc_pthread_cond_move.init.stderr index 6e90c490a2..9a8ddc0b52 100644 --- a/tests/fail-dep/concurrency/libc_pthread_cond_move.init.stderr +++ b/tests/fail-dep/concurrency/libc_pthread_cond_move.init.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: pthread_cond_t can't be moved after first use +error: Undefined Behavior: `pthread_cond_t` can't be moved after first use --> tests/fail-dep/concurrency/libc_pthread_cond_move.rs:LL:CC | LL | libc::pthread_cond_destroy(cond2.as_mut_ptr()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pthread_cond_t can't be moved after first use + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `pthread_cond_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 diff --git a/tests/fail-dep/concurrency/libc_pthread_cond_move.rs b/tests/fail-dep/concurrency/libc_pthread_cond_move.rs index 8fd0caac75..4db904ab5e 100644 --- a/tests/fail-dep/concurrency/libc_pthread_cond_move.rs +++ b/tests/fail-dep/concurrency/libc_pthread_cond_move.rs @@ -18,7 +18,7 @@ fn check() { // move pthread_cond_t let mut cond2 = cond; - libc::pthread_cond_destroy(cond2.as_mut_ptr()); //~[init] ERROR: pthread_cond_t can't be moved after first use + libc::pthread_cond_destroy(cond2.as_mut_ptr()); //~[init] ERROR: can't be moved after first use } } @@ -32,6 +32,6 @@ fn check() { // move pthread_cond_t let mut cond2 = cond; - libc::pthread_cond_destroy(&mut cond2 as *mut _); //~[static_initializer] ERROR: pthread_cond_t can't be moved after first use + libc::pthread_cond_destroy(&mut cond2 as *mut _); //~[static_initializer] ERROR: can't be moved after first use } } diff --git a/tests/fail-dep/concurrency/libc_pthread_cond_move.static_initializer.stderr b/tests/fail-dep/concurrency/libc_pthread_cond_move.static_initializer.stderr index ba726ac7f3..8a7c0dee12 100644 --- a/tests/fail-dep/concurrency/libc_pthread_cond_move.static_initializer.stderr +++ b/tests/fail-dep/concurrency/libc_pthread_cond_move.static_initializer.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: pthread_cond_t can't be moved after first use +error: Undefined Behavior: `pthread_cond_t` can't be moved after first use --> tests/fail-dep/concurrency/libc_pthread_cond_move.rs:LL:CC | LL | libc::pthread_cond_destroy(&mut cond2 as *mut _); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pthread_cond_t can't be moved after first use + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `pthread_cond_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 From 9cb530589026ec535bb0c0e59a6de34755353217 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 12 Oct 2024 14:36:14 +0200 Subject: [PATCH 4/7] macOS os_unfair_lock: also store ID outside addressable memory --- src/concurrency/sync.rs | 50 ++++++++++-------------------------- src/machine.rs | 6 +++++ src/shims/unix/macos/sync.rs | 19 +++++++++++--- src/shims/unix/sync.rs | 16 +++++------- 4 files changed, 42 insertions(+), 49 deletions(-) diff --git a/src/concurrency/sync.rs b/src/concurrency/sync.rs index 2901cb9ccb..015fe2727f 100644 --- a/src/concurrency/sync.rs +++ b/src/concurrency/sync.rs @@ -236,48 +236,26 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { } } -// 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> { - /// Eagerly create and initialize a new mutex. - fn mutex_create(&mut self) -> MutexId { - let this = self.eval_context_mut(); - this.machine.sync.mutexes.push(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: &MPlaceTy<'tcx>, - offset: u64, - ) -> InterpResult<'tcx, MutexId> { - let this = self.eval_context_mut(); - this.get_or_create_id( - lock, - offset, - |ecx| &mut ecx.machine.sync.mutexes, - |_ecx| interp_ok(Mutex::default()), - )? - .ok_or_else(|| err_ub_format!("mutex has invalid ID")) - .into() +impl SynchronizationObjects { + pub fn mutex_create(&mut self) -> MutexId { + self.mutexes.push(Default::default()) } - /// Eagerly create and initialize a new rwlock. - fn rwlock_create(&mut self) -> RwLockId { - let this = self.eval_context_mut(); - this.machine.sync.rwlocks.push(Default::default()) + pub fn rwlock_create(&mut self) -> RwLockId { + self.rwlocks.push(Default::default()) } - /// Eagerly create and initialize a new condvar. - fn condvar_create(&mut self) -> CondvarId { - let this = self.eval_context_mut(); - this.machine.sync.condvars.push(Default::default()) + pub fn condvar_create(&mut self) -> CondvarId { + self.condvars.push(Default::default()) } +} +// 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> { #[inline] /// Get the id of the thread that currently owns this lock. fn mutex_get_owner(&mut self, id: MutexId) -> ThreadId { diff --git a/src/machine.rs b/src/machine.rs index 60d096b92f..d5734ea3c8 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -362,6 +362,12 @@ impl VisitProvenance for AllocExtra<'_> { } } +impl<'tcx> AllocExtra<'tcx> { + pub fn get_sync(&self, offset: Size) -> Option<&T> { + self.sync.get(&offset).and_then(|s| s.downcast_ref::()) + } +} + /// Precomputed layouts of primitive types pub struct PrimitiveLayouts<'tcx> { pub unit: TyAndLayout<'tcx>, diff --git a/src/shims/unix/macos/sync.rs b/src/shims/unix/macos/sync.rs index 1efe393a0e..f135a0df0a 100644 --- a/src/shims/unix/macos/sync.rs +++ b/src/shims/unix/macos/sync.rs @@ -12,15 +12,26 @@ use crate::*; +struct LockData { + 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> { let this = self.eval_context_mut(); let lock = this.deref_pointer(lock_ptr)?; - // 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, /* offset */ 0) + // 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::(offset) { + interp_ok(data.id) + } else { + let id = machine.sync.mutex_create(); + alloc_extra.sync.insert(offset, Box::new(LockData { id })); + interp_ok(id) + } } } diff --git a/src/shims/unix/sync.rs b/src/shims/unix/sync.rs index 1942f25996..abc2980440 100644 --- a/src/shims/unix/sync.rs +++ b/src/shims/unix/sync.rs @@ -80,11 +80,9 @@ fn sync_get_data<'tcx, T: 'static + Copy>( // 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, _machine) = ecx.get_alloc_extra_mut(alloc)?; + let alloc_extra = ecx.get_alloc_extra(alloc)?; let data = alloc_extra - .sync - .get(&offset) - .and_then(|s| s.downcast_ref::()) + .get_sync::(offset) .ok_or_else(|| err_ub_format!("`{name}` can't be moved after first use"))?; interp_ok(Some(*data)) } else { @@ -174,7 +172,7 @@ enum MutexKind { #[derive(Debug, Clone, Copy)] /// Additional data that we attach with each mutex instance. -pub struct MutexData { +struct MutexData { id: MutexId, kind: MutexKind, } @@ -228,7 +226,7 @@ fn mutex_create<'tcx>( kind: MutexKind, ) -> InterpResult<'tcx, MutexData> { let mutex = ecx.deref_pointer(mutex_ptr)?; - let id = ecx.mutex_create(); + let id = ecx.machine.sync.mutex_create(); let data = MutexData { id, kind }; sync_init(ecx, &mutex, mutex_init_offset(ecx)?, data)?; interp_ok(data) @@ -290,7 +288,7 @@ fn mutex_kind_from_static_initializer<'tcx>( #[derive(Debug, Copy, Clone)] /// Additional data that we attach with each rwlock instance. -pub struct RwLockData { +struct RwLockData { id: RwLockId, } @@ -337,7 +335,7 @@ fn rwlock_get_data<'tcx>( )? { throw_unsup_format!("unsupported static initializer used for `pthread_rwlock_t`"); } - let id = ecx.rwlock_create(); + let id = ecx.machine.sync.rwlock_create(); let data = RwLockData { id }; sync_init(ecx, &rwlock, init_offset, data)?; interp_ok(data) @@ -446,7 +444,7 @@ fn cond_create<'tcx>( clock: ClockId, ) -> InterpResult<'tcx, CondData> { let cond = ecx.deref_pointer(cond_ptr)?; - let id = ecx.condvar_create(); + let id = ecx.machine.sync.condvar_create(); let data = CondData { id, clock }; sync_init(ecx, &cond, cond_init_offset(ecx)?, data)?; interp_ok(data) From 8dd5bcbe4b8ea134d00af7cfb87a94ff355a1b20 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 12 Oct 2024 15:02:08 +0200 Subject: [PATCH 5/7] Windows InitOnce: also store ID outside addressable memory --- src/concurrency/init_once.rs | 17 ---- src/concurrency/sync.rs | 145 ++++++++++++++++++----------------- src/machine.rs | 6 -- src/shims/unix/sync.rs | 90 +++++----------------- src/shims/windows/sync.rs | 21 ++++- 5 files changed, 114 insertions(+), 165 deletions(-) diff --git a/src/concurrency/init_once.rs b/src/concurrency/init_once.rs index 488edc91df..c3c04428c9 100644 --- a/src/concurrency/init_once.rs +++ b/src/concurrency/init_once.rs @@ -2,7 +2,6 @@ use std::collections::VecDeque; use rustc_index::Idx; -use super::sync::EvalContextExtPriv as _; use super::vector_clock::VClock; use crate::*; @@ -27,22 +26,6 @@ pub(super) struct InitOnce { impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { - fn init_once_get_or_create_id( - &mut self, - lock: &MPlaceTy<'tcx>, - offset: u64, - ) -> InterpResult<'tcx, InitOnceId> { - let this = self.eval_context_mut(); - this.get_or_create_id( - lock, - offset, - |ecx| &mut ecx.machine.sync.init_onces, - |_| interp_ok(Default::default()), - )? - .ok_or_else(|| err_ub_format!("init_once has invalid ID")) - .into() - } - #[inline] fn init_once_status(&mut self, id: InitOnceId) -> InitOnceStatus { let this = self.eval_context_ref(); diff --git a/src/concurrency/sync.rs b/src/concurrency/sync.rs index 015fe2727f..e42add0d50 100644 --- a/src/concurrency/sync.rs +++ b/src/concurrency/sync.rs @@ -11,11 +11,6 @@ use super::init_once::InitOnce; use super::vector_clock::VClock; use crate::*; -pub trait SyncId { - fn from_u32(id: u32) -> Self; - fn to_u32(&self) -> u32; -} - /// We cannot use the `newtype_index!` macro because we have to use 0 as a /// sentinel value meaning that the identifier is not assigned. This is because /// the pthreads static initializers initialize memory with zeros (see the @@ -27,16 +22,6 @@ macro_rules! declare_id { #[derive(Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)] pub struct $name(std::num::NonZero); - impl $crate::concurrency::sync::SyncId for $name { - // Panics if `id == 0`. - fn from_u32(id: u32) -> Self { - Self(std::num::NonZero::new(id).unwrap()) - } - fn to_u32(&self) -> u32 { - self.0.get() - } - } - impl $crate::VisitProvenance for $name { fn visit_provenance(&self, _visit: &mut VisitWith<'_>) {} } @@ -55,12 +40,6 @@ macro_rules! declare_id { usize::try_from(self.0.get() - 1).unwrap() } } - - impl $name { - pub fn to_u32_scalar(&self) -> Scalar { - Scalar::from_u32(self.0.get()) - } - } }; } pub(super) use declare_id; @@ -166,56 +145,6 @@ pub struct SynchronizationObjects { // Private extension trait for local helper methods impl<'tcx> EvalContextExtPriv<'tcx> for crate::MiriInterpCx<'tcx> {} pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { - /// Lazily initialize the ID of this Miri sync structure. - /// If memory stores '0', that indicates uninit and we generate a new instance. - /// 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( - &mut self, - lock: &MPlaceTy<'tcx>, - offset: u64, - get_objs: impl for<'a> Fn(&'a mut MiriInterpCx<'tcx>) -> &'a mut IndexVec, - create_obj: impl for<'a> FnOnce(&'a mut MiriInterpCx<'tcx>) -> InterpResult<'tcx, T>, - ) -> InterpResult<'tcx, Option> { - let this = self.eval_context_mut(); - let offset = Size::from_bytes(offset); - assert!(lock.layout.size >= offset + this.machine.layouts.u32.size); - let id_place = lock.offset(offset, this.machine.layouts.u32, this)?; - let next_index = get_objs(this).next_index(); - - // Since we are lazy, this update has to be atomic. - let (old, success) = this - .atomic_compare_exchange_scalar( - &id_place, - &ImmTy::from_uint(0u32, this.machine.layouts.u32), - Scalar::from_u32(next_index.to_u32()), - AtomicRwOrd::Relaxed, // deliberately *no* synchronization - AtomicReadOrd::Relaxed, - false, - )? - .to_scalar_pair(); - - interp_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 obj = create_obj(this)?; - let new_index = get_objs(this).push(obj); - assert_eq!(next_index, new_index); - Some(new_index) - } else { - let id = Id::from_u32(old.to_u32().expect("layout is u32")); - if get_objs(this).get(id).is_none() { - // The in-memory ID is invalid. - None - } else { - Some(id) - } - }) - } - fn condvar_reacquire_mutex( &mut self, mutex: MutexId, @@ -248,6 +177,80 @@ impl SynchronizationObjects { pub fn condvar_create(&mut self) -> CondvarId { self.condvars.push(Default::default()) } + + pub fn init_once_create(&mut self) -> InitOnceId { + self.init_onces.push(Default::default()) + } +} + +impl<'tcx> AllocExtra<'tcx> { + pub fn get_sync(&self, offset: Size) -> Option<&T> { + self.sync.get(&offset).and_then(|s| s.downcast_ref::()) + } +} + +/// We designate an `init`` field in all primitives. +/// 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 init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?; + + 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". + 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, return None. +pub fn lazy_sync_get_data<'tcx, T: 'static + Copy>( + ecx: &mut MiriInterpCx<'tcx>, + primitive: &MPlaceTy<'tcx>, + init_offset: Size, + name: &str, +) -> InterpResult<'tcx, Option> { + let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?; + // 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, success) = ecx + .atomic_compare_exchange_scalar( + &init_field, + &ImmTy::from_scalar(init_cookie, ecx.machine.layouts.u32), + init_cookie, + AtomicRwOrd::Acquire, + AtomicReadOrd::Acquire, + /* 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::(offset) + .ok_or_else(|| err_ub_format!("`{name}` can't be moved after first use"))?; + interp_ok(Some(*data)) + } else { + interp_ok(None) + } } // Public interface to synchronization primitives. Please note that in most diff --git a/src/machine.rs b/src/machine.rs index d5734ea3c8..60d096b92f 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -362,12 +362,6 @@ impl VisitProvenance for AllocExtra<'_> { } } -impl<'tcx> AllocExtra<'tcx> { - pub fn get_sync(&self, offset: Size) -> Option<&T> { - self.sync.get(&offset).and_then(|s| s.downcast_ref::()) - } -} - /// Precomputed layouts of primitive types pub struct PrimitiveLayouts<'tcx> { pub unit: TyAndLayout<'tcx>, diff --git a/src/shims/unix/sync.rs b/src/shims/unix/sync.rs index abc2980440..efbe09a2a5 100644 --- a/src/shims/unix/sync.rs +++ b/src/shims/unix/sync.rs @@ -2,10 +2,13 @@ use std::sync::atomic::{AtomicBool, Ordering}; use rustc_target::abi::Size; +use crate::concurrency::sync::{LAZY_INIT_COOKIE, lazy_sync_get_data, lazy_sync_init}; use crate::*; -/// Do a bytewise comparison of the two places, using relaxed atomic reads. -/// This is used to check if a mutex matches its static initializer value. +/// Do a bytewise comparison of the two places, using relaxed atomic reads. This is used to check if +/// a synchronization primitive matches its static initializer value. +/// +/// The reads happen in chunks of 4, so all racing accesses must also use that access size. fn bytewise_equal_atomic_relaxed<'tcx>( ecx: &MiriInterpCx<'tcx>, left: &MPlaceTy<'tcx>, @@ -33,63 +36,6 @@ fn bytewise_equal_atomic_relaxed<'tcx>( interp_ok(true) } -/// We designate an `init`` field in all primitives. -/// If `init` is set to this, we consider the primitive initialized. -const INIT_COOKIE: u32 = 0xcafe_affe; - -fn sync_init<'tcx, T: 'static + Copy>( - ecx: &mut MiriInterpCx<'tcx>, - primitive: &MPlaceTy<'tcx>, - init_offset: Size, - data: T, -) -> InterpResult<'tcx> { - let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?; - - 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". - ecx.write_scalar_atomic(Scalar::from_u32(INIT_COOKIE), &init_field, AtomicWriteOrd::Relaxed)?; - interp_ok(()) -} - -/// Checks if the primitive is initialized, and return its associated data if so. -/// Otherwise, return None. -fn sync_get_data<'tcx, T: 'static + Copy>( - ecx: &mut MiriInterpCx<'tcx>, - primitive: &MPlaceTy<'tcx>, - init_offset: Size, - name: &str, -) -> InterpResult<'tcx, Option> { - let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?; - // 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(INIT_COOKIE); - let (_init, success) = ecx - .atomic_compare_exchange_scalar( - &init_field, - &ImmTy::from_scalar(init_cookie, ecx.machine.layouts.u32), - init_cookie, - AtomicRwOrd::Acquire, - AtomicReadOrd::Acquire, - /* 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::(offset) - .ok_or_else(|| err_ub_format!("`{name}` can't be moved after first use"))?; - interp_ok(Some(*data)) - } else { - interp_ok(None) - } -} - // # pthread_mutexattr_t // We store some data directly inside the type, ignoring the platform layout: // - kind: i32 @@ -198,7 +144,10 @@ fn mutex_init_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, Size> let init_field = static_initializer.offset(offset, ecx.machine.layouts.u32, ecx).unwrap(); let init = ecx.read_scalar(&init_field).unwrap().to_u32().unwrap(); - assert_ne!(init, INIT_COOKIE, "{name} is incompatible with our initialization cookie"); + assert_ne!( + init, LAZY_INIT_COOKIE, + "{name} is incompatible with our initialization cookie" + ); }; check_static_initializer("PTHREAD_MUTEX_INITIALIZER"); @@ -228,7 +177,7 @@ fn mutex_create<'tcx>( let mutex = ecx.deref_pointer(mutex_ptr)?; let id = ecx.machine.sync.mutex_create(); let data = MutexData { id, kind }; - sync_init(ecx, &mutex, mutex_init_offset(ecx)?, data)?; + lazy_sync_init(ecx, &mutex, mutex_init_offset(ecx)?, data)?; interp_ok(data) } @@ -243,7 +192,7 @@ fn mutex_get_data<'tcx, 'a>( let mutex = ecx.deref_pointer(mutex_ptr)?; if let Some(data) = - sync_get_data::(ecx, &mutex, mutex_init_offset(ecx)?, "pthread_mutex_t")? + lazy_sync_get_data::(ecx, &mutex, mutex_init_offset(ecx)?, "pthread_mutex_t")? { interp_ok(data) } else { @@ -302,14 +251,14 @@ fn rwlock_init_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, Size let offset = Size::from_bytes(offset); // Sanity-check this against PTHREAD_RWLOCK_INITIALIZER (but only once): - // the `init` field must start out not equal to INIT_COOKIE. + // the `init` field must start out not equal to LAZY_INIT_COOKIE. static SANITY: AtomicBool = AtomicBool::new(false); if !SANITY.swap(true, Ordering::Relaxed) { let static_initializer = ecx.eval_path(&["libc", "PTHREAD_RWLOCK_INITIALIZER"]); let init_field = static_initializer.offset(offset, ecx.machine.layouts.u32, ecx).unwrap(); let init = ecx.read_scalar(&init_field).unwrap().to_u32().unwrap(); assert_ne!( - init, INIT_COOKIE, + init, LAZY_INIT_COOKIE, "PTHREAD_RWLOCK_INITIALIZER is incompatible with our initialization cookie" ); } @@ -324,7 +273,8 @@ fn rwlock_get_data<'tcx>( let rwlock = ecx.deref_pointer(rwlock_ptr)?; let init_offset = rwlock_init_offset(ecx)?; - if let Some(data) = sync_get_data::(ecx, &rwlock, init_offset, "pthread_rwlock_t")? + if let Some(data) = + lazy_sync_get_data::(ecx, &rwlock, init_offset, "pthread_rwlock_t")? { interp_ok(data) } else { @@ -337,7 +287,7 @@ fn rwlock_get_data<'tcx>( } let id = ecx.machine.sync.rwlock_create(); let data = RwLockData { id }; - sync_init(ecx, &rwlock, init_offset, data)?; + lazy_sync_init(ecx, &rwlock, init_offset, data)?; interp_ok(data) } } @@ -410,14 +360,14 @@ fn cond_init_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, Size> let offset = Size::from_bytes(offset); // Sanity-check this against PTHREAD_COND_INITIALIZER (but only once): - // the `init` field must start out not equal to INIT_COOKIE. + // the `init` field must start out not equal to LAZY_INIT_COOKIE. static SANITY: AtomicBool = AtomicBool::new(false); if !SANITY.swap(true, Ordering::Relaxed) { let static_initializer = ecx.eval_path(&["libc", "PTHREAD_COND_INITIALIZER"]); let init_field = static_initializer.offset(offset, ecx.machine.layouts.u32, ecx).unwrap(); let init = ecx.read_scalar(&init_field).unwrap().to_u32().unwrap(); assert_ne!( - init, INIT_COOKIE, + init, LAZY_INIT_COOKIE, "PTHREAD_COND_INITIALIZER is incompatible with our initialization cookie" ); } @@ -446,7 +396,7 @@ fn cond_create<'tcx>( let cond = ecx.deref_pointer(cond_ptr)?; let id = ecx.machine.sync.condvar_create(); let data = CondData { id, clock }; - sync_init(ecx, &cond, cond_init_offset(ecx)?, data)?; + lazy_sync_init(ecx, &cond, cond_init_offset(ecx)?, data)?; interp_ok(data) } @@ -457,7 +407,7 @@ fn cond_get_data<'tcx>( let cond = ecx.deref_pointer(cond_ptr)?; let init_offset = cond_init_offset(ecx)?; - if let Some(data) = sync_get_data::(ecx, &cond, init_offset, "pthread_cond_t")? { + if let Some(data) = lazy_sync_get_data::(ecx, &cond, init_offset, "pthread_cond_t")? { interp_ok(data) } else { // This used the static initializer. The clock there is always CLOCK_REALTIME. diff --git a/src/shims/windows/sync.rs b/src/shims/windows/sync.rs index 6755c23039..de780361b5 100644 --- a/src/shims/windows/sync.rs +++ b/src/shims/windows/sync.rs @@ -3,8 +3,14 @@ use std::time::Duration; use rustc_target::abi::Size; use crate::concurrency::init_once::InitOnceStatus; +use crate::concurrency::sync::{lazy_sync_get_data, lazy_sync_init}; use crate::*; +#[derive(Copy, Clone)] +struct InitOnceData { + id: InitOnceId, +} + impl<'tcx> EvalContextExtPriv<'tcx> for crate::MiriInterpCx<'tcx> {} trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { // Windows sync primitives are pointer sized. @@ -12,8 +18,21 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { fn init_once_get_id(&mut self, init_once_ptr: &OpTy<'tcx>) -> InterpResult<'tcx, InitOnceId> { let this = self.eval_context_mut(); + let init_once = this.deref_pointer(init_once_ptr)?; - this.init_once_get_or_create_id(&init_once, 0) + let init_offset = Size::ZERO; + + if let Some(data) = + lazy_sync_get_data::(this, &init_once, init_offset, "INIT_ONCE")? + { + interp_ok(data.id) + } else { + // TODO: check that this is still all-zero. + let id = this.machine.sync.init_once_create(); + let data = InitOnceData { id }; + lazy_sync_init(this, &init_once, init_offset, data)?; + interp_ok(id) + } } /// Returns `true` if we were succssful, `false` if we would block. From d45d3a350c6326ac3d79b6fc3a19e4fa0d454893 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 12 Oct 2024 15:12:46 +0200 Subject: [PATCH 6/7] make lazy_sync_get_data also take a closure to initialize if needed --- src/concurrency/sync.rs | 17 +++++++++------- src/shims/unix/sync.rs | 43 +++++++++++---------------------------- src/shims/windows/sync.rs | 23 +++++++++------------ 3 files changed, 32 insertions(+), 51 deletions(-) diff --git a/src/concurrency/sync.rs b/src/concurrency/sync.rs index e42add0d50..11110c6e75 100644 --- a/src/concurrency/sync.rs +++ b/src/concurrency/sync.rs @@ -201,12 +201,11 @@ pub fn lazy_sync_init<'tcx, T: 'static + Copy>( init_offset: Size, data: T, ) -> InterpResult<'tcx> { - let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?; - 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, @@ -217,18 +216,19 @@ pub fn lazy_sync_init<'tcx, T: 'static + Copy>( /// Helper for lazily initialized `alloc_extra.sync` data: /// Checks if the primitive is initialized, and return its associated data if so. -/// Otherwise, return None. +/// 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, -) -> InterpResult<'tcx, Option> { - let init_field = primitive.offset(init_offset, ecx.machine.layouts.u32, ecx)?; + 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, @@ -239,6 +239,7 @@ pub fn lazy_sync_get_data<'tcx, T: 'static + Copy>( /* 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. @@ -247,9 +248,11 @@ pub fn lazy_sync_get_data<'tcx, T: 'static + Copy>( let data = alloc_extra .get_sync::(offset) .ok_or_else(|| err_ub_format!("`{name}` can't be moved after first use"))?; - interp_ok(Some(*data)) + interp_ok(*data) } else { - interp_ok(None) + let data = new_data(ecx)?; + lazy_sync_init(ecx, primitive, init_offset, data)?; + interp_ok(data) } } diff --git a/src/shims/unix/sync.rs b/src/shims/unix/sync.rs index efbe09a2a5..5b4c136c23 100644 --- a/src/shims/unix/sync.rs +++ b/src/shims/unix/sync.rs @@ -190,19 +190,11 @@ fn mutex_get_data<'tcx, 'a>( mutex_ptr: &OpTy<'tcx>, ) -> InterpResult<'tcx, MutexData> { let mutex = ecx.deref_pointer(mutex_ptr)?; - - if let Some(data) = - lazy_sync_get_data::(ecx, &mutex, mutex_init_offset(ecx)?, "pthread_mutex_t")? - { - interp_ok(data) - } else { - // Not yet initialized. This must be a static initializer, figure out the kind - // from that. We don't need to worry about races since we are the interpreter - // and don't let any other tread take a step. + lazy_sync_get_data(ecx, &mutex, mutex_init_offset(ecx)?, "pthread_mutex_t", |ecx| { let kind = mutex_kind_from_static_initializer(ecx, &mutex)?; - // And then create the mutex like this. - mutex_create(ecx, mutex_ptr, kind) - } + let id = ecx.machine.sync.mutex_create(); + interp_ok(MutexData { id, kind }) + }) } /// Returns the kind of a static initializer. @@ -271,13 +263,7 @@ fn rwlock_get_data<'tcx>( rwlock_ptr: &OpTy<'tcx>, ) -> InterpResult<'tcx, RwLockData> { let rwlock = ecx.deref_pointer(rwlock_ptr)?; - let init_offset = rwlock_init_offset(ecx)?; - - if let Some(data) = - lazy_sync_get_data::(ecx, &rwlock, init_offset, "pthread_rwlock_t")? - { - interp_ok(data) - } else { + lazy_sync_get_data(ecx, &rwlock, rwlock_init_offset(ecx)?, "pthread_rwlock_t", |ecx| { if !bytewise_equal_atomic_relaxed( ecx, &rwlock, @@ -286,10 +272,8 @@ fn rwlock_get_data<'tcx>( throw_unsup_format!("unsupported static initializer used for `pthread_rwlock_t`"); } let id = ecx.machine.sync.rwlock_create(); - let data = RwLockData { id }; - lazy_sync_init(ecx, &rwlock, init_offset, data)?; - interp_ok(data) - } + interp_ok(RwLockData { id }) + }) } // # pthread_condattr_t @@ -405,12 +389,7 @@ fn cond_get_data<'tcx>( cond_ptr: &OpTy<'tcx>, ) -> InterpResult<'tcx, CondData> { let cond = ecx.deref_pointer(cond_ptr)?; - let init_offset = cond_init_offset(ecx)?; - - if let Some(data) = lazy_sync_get_data::(ecx, &cond, init_offset, "pthread_cond_t")? { - interp_ok(data) - } else { - // This used the static initializer. The clock there is always CLOCK_REALTIME. + lazy_sync_get_data(ecx, &cond, cond_init_offset(ecx)?, "pthread_cond_t", |ecx| { if !bytewise_equal_atomic_relaxed( ecx, &cond, @@ -418,8 +397,10 @@ fn cond_get_data<'tcx>( )? { throw_unsup_format!("unsupported static initializer used for `pthread_cond_t`"); } - cond_create(ecx, cond_ptr, ClockId::Realtime) - } + // This used the static initializer. The clock there is always CLOCK_REALTIME. + let id = ecx.machine.sync.condvar_create(); + interp_ok(CondData { id, clock: ClockId::Realtime }) + }) } impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} diff --git a/src/shims/windows/sync.rs b/src/shims/windows/sync.rs index de780361b5..d0e82112a5 100644 --- a/src/shims/windows/sync.rs +++ b/src/shims/windows/sync.rs @@ -3,7 +3,7 @@ use std::time::Duration; use rustc_target::abi::Size; use crate::concurrency::init_once::InitOnceStatus; -use crate::concurrency::sync::{lazy_sync_get_data, lazy_sync_init}; +use crate::concurrency::sync::lazy_sync_get_data; use crate::*; #[derive(Copy, Clone)] @@ -16,23 +16,20 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { // Windows sync primitives are pointer sized. // We only use the first 4 bytes for the id. - fn init_once_get_id(&mut self, init_once_ptr: &OpTy<'tcx>) -> InterpResult<'tcx, InitOnceId> { + fn init_once_get_data( + &mut self, + init_once_ptr: &OpTy<'tcx>, + ) -> InterpResult<'tcx, InitOnceData> { let this = self.eval_context_mut(); let init_once = this.deref_pointer(init_once_ptr)?; let init_offset = Size::ZERO; - if let Some(data) = - lazy_sync_get_data::(this, &init_once, init_offset, "INIT_ONCE")? - { - interp_ok(data.id) - } else { + lazy_sync_get_data(this, &init_once, init_offset, "INIT_ONCE", |this| { // TODO: check that this is still all-zero. let id = this.machine.sync.init_once_create(); - let data = InitOnceData { id }; - lazy_sync_init(this, &init_once, init_offset, data)?; - interp_ok(id) - } + interp_ok(InitOnceData { id }) + }) } /// Returns `true` if we were succssful, `false` if we would block. @@ -74,7 +71,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let id = this.init_once_get_id(init_once_op)?; + let id = this.init_once_get_data(init_once_op)?.id; let flags = this.read_scalar(flags_op)?.to_u32()?; let pending_place = this.deref_pointer(pending_op)?; let context = this.read_pointer(context_op)?; @@ -120,7 +117,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let id = this.init_once_get_id(init_once_op)?; + let id = this.init_once_get_data(init_once_op)?.id; let flags = this.read_scalar(flags_op)?.to_u32()?; let context = this.read_pointer(context_op)?; From 10979ce8576033aee96f30d584ae5beddb4f7d3e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 12 Oct 2024 15:22:59 +0200 Subject: [PATCH 7/7] turns out relaxed accesses suffice here --- src/concurrency/sync.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/concurrency/sync.rs b/src/concurrency/sync.rs index 11110c6e75..2c6a7bf0f5 100644 --- a/src/concurrency/sync.rs +++ b/src/concurrency/sync.rs @@ -234,8 +234,8 @@ pub fn lazy_sync_get_data<'tcx, T: 'static + Copy>( &init_field, &ImmTy::from_scalar(init_cookie, ecx.machine.layouts.u32), init_cookie, - AtomicRwOrd::Acquire, - AtomicReadOrd::Acquire, + AtomicRwOrd::Relaxed, + AtomicReadOrd::Relaxed, /* can_fail_spuriously */ false, )? .to_scalar_pair();