diff --git a/src/tools/miri/src/concurrency/sync.rs b/src/tools/miri/src/concurrency/sync.rs index 7098cc5b95bef..1f910d885ca5a 100644 --- a/src/tools/miri/src/concurrency/sync.rs +++ b/src/tools/miri/src/concurrency/sync.rs @@ -134,6 +134,9 @@ 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. @@ -344,21 +347,49 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.machine.sync.rwlocks[id].data.as_deref().and_then(|p| p.downcast_ref::()) } + /// Eagerly create and initialize a new condvar. + fn condvar_create( + &mut self, + condvar: &MPlaceTy<'tcx>, + offset: u64, + data: Option>, + ) -> InterpResult<'tcx, 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, - |_| Ok(Default::default()), + |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::()) + } + #[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/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs index dbf0ca0cecc7e..3535eacb447e6 100644 --- a/src/tools/miri/src/shims/unix/sync.rs +++ b/src/tools/miri/src/shims/unix/sync.rs @@ -206,7 +206,7 @@ fn translate_kind<'tcx>(ecx: &MiriInterpCx<'tcx>, kind: i32) -> InterpResult<'tc // - id: u32 #[derive(Debug)] -/// Additional data that may be used by shim implementations. +/// Additional data that we attach with each rwlock instance. pub struct AdditionalRwLockData { /// The address of the rwlock. pub address: u64, @@ -286,6 +286,19 @@ fn condattr_get_clock_id<'tcx>( .to_i32() } +fn translate_clock_id<'tcx>(ecx: &MiriInterpCx<'tcx>, raw_id: i32) -> InterpResult<'tcx, ClockId> { + // To ensure compatibility with PTHREAD_COND_INITIALIZER on all platforms, + // we can't just compare with CLOCK_REALTIME: on Solarish, PTHREAD_COND_INITIALIZER + // makes the clock 0 but CLOCK_REALTIME is 3. + Ok(if raw_id == ecx.eval_libc_i32("CLOCK_REALTIME") || raw_id == 0 { + 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>, @@ -331,16 +344,6 @@ fn cond_id_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, u64> { Ok(offset) } -/// Determines whether this clock represents the real-time clock, CLOCK_REALTIME. -fn is_cond_clock_realtime<'tcx>(ecx: &MiriInterpCx<'tcx>, clock_id: i32) -> bool { - // To ensure compatibility with PTHREAD_COND_INITIALIZER on all platforms, - // we can't just compare with CLOCK_REALTIME: on Solarish, PTHREAD_COND_INITIALIZER - // makes the clock 0 but CLOCK_REALTIME is 3. - // However, we need to always be able to distinguish this from CLOCK_MONOTONIC. - clock_id == ecx.eval_libc_i32("CLOCK_REALTIME") - || (clock_id == 0 && clock_id != ecx.eval_libc_i32("CLOCK_MONOTONIC")) -} - fn cond_clock_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> u64 { // macOS doesn't have a clock attribute, but to keep the code uniform we store // a clock ID in the pthread_cond_t anyway. There's enough space. @@ -355,8 +358,9 @@ fn cond_clock_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> u64 { .offset(Size::from_bytes(offset), ecx.machine.layouts.i32, ecx) .unwrap(); let id = ecx.read_scalar(&id_field).unwrap().to_i32().unwrap(); + let id = translate_clock_id(ecx, id).expect("static initializer should be valid"); assert!( - is_cond_clock_realtime(ecx, id), + matches!(id, ClockId::Realtime), "PTHREAD_COND_INITIALIZER is incompatible with our pthread_cond layout: clock is not CLOCK_REALTIME" ); } @@ -364,25 +368,47 @@ fn cond_clock_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> u64 { offset } +#[derive(Debug, Clone, Copy)] +enum ClockId { + Realtime, + Monotonic, +} + +#[derive(Debug)] +/// Additional data that we attach with each cond instance. +struct AdditionalCondData { + /// The address of the cond. + address: u64, + + /// The clock id of the cond. + clock_id: ClockId, +} + fn cond_get_id<'tcx>( ecx: &mut MiriInterpCx<'tcx>, cond_ptr: &OpTy<'tcx>, ) -> InterpResult<'tcx, CondvarId> { let cond = ecx.deref_pointer(cond_ptr)?; - ecx.condvar_get_or_create_id(&cond, cond_id_offset(ecx)?) -} + let address = cond.ptr().addr().bytes(); + let id = ecx.condvar_get_or_create_id(&cond, cond_id_offset(ecx)?, |ecx| { + let raw_id = if ecx.tcx.sess.target.os == "macos" { + ecx.eval_libc_i32("CLOCK_REALTIME") + } else { + cond_get_clock_id(ecx, cond_ptr)? + }; + let clock_id = translate_clock_id(ecx, raw_id)?; + Ok(Some(Box::new(AdditionalCondData { address, clock_id }))) + })?; -fn cond_reset_id<'tcx>( - ecx: &mut MiriInterpCx<'tcx>, - cond_ptr: &OpTy<'tcx>, -) -> InterpResult<'tcx, ()> { - ecx.deref_pointer_and_write( - cond_ptr, - cond_id_offset(ecx)?, - Scalar::from_i32(0), - ecx.libc_ty_layout("pthread_cond_t"), - ecx.machine.layouts.u32, - ) + // 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") + } + + Ok(id) } fn cond_get_clock_id<'tcx>( @@ -398,20 +424,6 @@ fn cond_get_clock_id<'tcx>( .to_i32() } -fn cond_set_clock_id<'tcx>( - ecx: &mut MiriInterpCx<'tcx>, - cond_ptr: &OpTy<'tcx>, - clock_id: i32, -) -> InterpResult<'tcx, ()> { - ecx.deref_pointer_and_write( - cond_ptr, - cond_clock_offset(ecx), - Scalar::from_i32(clock_id), - ecx.libc_ty_layout("pthread_cond_t"), - ecx.machine.layouts.i32, - ) -} - impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn pthread_mutexattr_init(&mut self, attr_op: &OpTy<'tcx>) -> InterpResult<'tcx, ()> { @@ -820,11 +832,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } else { condattr_get_clock_id(this, attr_op)? }; - - // Write 0 to use the same code path as the static initializers. - cond_reset_id(this, cond_op)?; - - cond_set_clock_id(this, cond_op, clock_id)?; + let clock_id = 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 })), + )?; Ok(()) } @@ -879,7 +895,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let mutex_id = mutex_get_id(this, mutex_op)?; // Extract the timeout. - let clock_id = cond_get_clock_id(this, cond_op)?; + 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"))?)? { @@ -890,13 +909,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { return Ok(()); } }; - let timeout_clock = if is_cond_clock_realtime(this, clock_id) { - this.check_no_isolation("`pthread_cond_timedwait` with `CLOCK_REALTIME`")?; - TimeoutClock::RealTime - } else if clock_id == this.eval_libc_i32("CLOCK_MONOTONIC") { - TimeoutClock::Monotonic - } else { - throw_unsup_format!("unsupported clock id: {}", clock_id); + let timeout_clock = match clock_id { + ClockId::Realtime => { + this.check_no_isolation("`pthread_cond_timedwait` with `CLOCK_REALTIME`")?; + TimeoutClock::RealTime + } + ClockId::Monotonic => TimeoutClock::Monotonic, }; this.condvar_wait( @@ -912,6 +930,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } fn pthread_cond_destroy(&mut self, cond_op: &OpTy<'tcx>) -> InterpResult<'tcx, ()> { + //NOTE: Destroying an uninit pthread_cond is UB. Make sure it's not uninit, + // by accessing at least once all of its fields that we use. + let this = self.eval_context_mut(); let id = cond_get_id(this, cond_op)?; @@ -919,10 +940,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { throw_ub_format!("destroying an awaited conditional variable"); } - // Destroying an uninit pthread_cond is UB, so check to make sure it's not uninit. - cond_get_id(this, cond_op)?; - cond_get_clock_id(this, cond_op)?; - // This might lead to false positives, see comment in pthread_mutexattr_destroy this.write_uninit(&this.deref_pointer_as(cond_op, this.libc_ty_layout("pthread_cond_t"))?)?; // FIXME: delete interpreter state associated with this condvar. diff --git a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_cond_move.init.stderr b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_cond_move.init.stderr new file mode 100644 index 0000000000000..a15451cb3193f --- /dev/null +++ b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_cond_move.init.stderr @@ -0,0 +1,20 @@ +error: Undefined Behavior: pthread_cond_t can't be moved after first use + --> $DIR/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 + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `check` at $DIR/libc_pthread_cond_move.rs:LL:CC +note: inside `main` + --> $DIR/libc_pthread_cond_move.rs:LL:CC + | +LL | check() + | ^^^^^^^ + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + diff --git a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_cond_move.rs b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_cond_move.rs new file mode 100644 index 0000000000000..e4e84eb9fd057 --- /dev/null +++ b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_cond_move.rs @@ -0,0 +1,37 @@ +//@revisions: static_initializer init +//@ignore-target-windows: No pthreads on Windows + +/// Test that moving a pthread_cond between uses fails. + +fn main() { + check() +} + +#[cfg(init)] +fn check() { + unsafe { + use core::mem::MaybeUninit; + let mut cond = MaybeUninit::::uninit(); + + libc::pthread_cond_init(cond.as_mut_ptr(), std::ptr::null()); + + // 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 + } +} + +#[cfg(static_initializer)] +fn check() { + unsafe { + let mut cond = libc::PTHREAD_COND_INITIALIZER; + + libc::pthread_cond_signal(&mut cond as *mut _); + + // 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 + } +} diff --git a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_cond_move.static_initializer.stderr b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_cond_move.static_initializer.stderr new file mode 100644 index 0000000000000..4e4188e2a12d1 --- /dev/null +++ b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_cond_move.static_initializer.stderr @@ -0,0 +1,20 @@ +error: Undefined Behavior: pthread_cond_t can't be moved after first use + --> $DIR/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 + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `check` at $DIR/libc_pthread_cond_move.rs:LL:CC +note: inside `main` + --> $DIR/libc_pthread_cond_move.rs:LL:CC + | +LL | check() + | ^^^^^^^ + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error +