diff --git a/src/shims/sync.rs b/src/shims/sync.rs index f34799f7425..4fe35347398 100644 --- a/src/shims/sync.rs +++ b/src/shims/sync.rs @@ -288,15 +288,12 @@ fn release_cond_mutex<'mir, 'tcx: 'mir>( active_thread: ThreadId, mutex: MutexId, ) -> InterpResult<'tcx> { - if let Some((old_owner_thread, old_locked_count)) = ecx.mutex_unlock(mutex)? { + if let Some(old_locked_count) = ecx.mutex_unlock(mutex, active_thread)? { if old_locked_count != 1 { throw_unsup_format!("awaiting on a lock acquired multiple times is not supported"); } - if old_owner_thread != active_thread { - throw_ub_format!("awaiting on a mutex owned by a different thread"); - } } else { - throw_ub_format!("awaiting on unlocked mutex"); + throw_ub_format!("awaiting on unlocked or owned by a different thread mutex"); } ecx.block_thread(active_thread)?; Ok(()) @@ -321,7 +318,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let this = self.eval_context_mut(); let kind = this.read_scalar(kind_op)?.not_undef()?; - if kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")? + if kind == this.eval_libc("PTHREAD_MUTEX_DEFAULT")? + || kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")? || kind == this.eval_libc("PTHREAD_MUTEX_ERRORCHECK")? || kind == this.eval_libc("PTHREAD_MUTEX_RECURSIVE")? { @@ -380,6 +378,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx Ok(0) } else { // Trying to acquire the same mutex again. + if kind == this.eval_libc("PTHREAD_MUTEX_DEFAULT")? { + // FIXME: Sometimes this is actually a Deadlock. + // https://github.com/rust-lang/miri/issues/1419 + throw_ub_format!( + "trying to acquire already locked PTHREAD_MUTEX_DEFAULT (see #1419)" + ); + } if kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")? { throw_machine_stop!(TerminationInfo::Deadlock); } else if kind == this.eval_libc("PTHREAD_MUTEX_ERRORCHECK")? { @@ -388,7 +393,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.mutex_lock(id, active_thread); Ok(0) } else { - throw_ub_format!("called pthread_mutex_lock on an unsupported type of mutex"); + throw_unsup_format!( + "called pthread_mutex_lock on an unsupported type of mutex" + ); } } } else { @@ -410,7 +417,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if owner_thread != active_thread { this.eval_libc_i32("EBUSY") } else { - if kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")? + if kind == this.eval_libc("PTHREAD_MUTEX_DEFAULT")? + || kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")? || kind == this.eval_libc("PTHREAD_MUTEX_ERRORCHECK")? { this.eval_libc_i32("EBUSY") @@ -418,7 +426,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.mutex_lock(id, active_thread); Ok(0) } else { - throw_ub_format!( + throw_unsup_format!( "called pthread_mutex_trylock on an unsupported type of mutex" ); } @@ -435,21 +443,29 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let kind = mutex_get_kind(this, mutex_op)?.not_undef()?; let id = mutex_get_or_create_id(this, mutex_op)?; + let active_thread = this.get_active_thread()?; - if let Some((old_owner_thread, _old_locked_count)) = this.mutex_unlock(id)? { - if old_owner_thread != this.get_active_thread()? { - throw_ub_format!("called pthread_mutex_unlock on a mutex owned by another thread"); - } + if let Some(_old_locked_count) = this.mutex_unlock(id, active_thread)? { + // The mutex was locked by the current thread. Ok(0) } else { - if kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")? { - throw_ub_format!("unlocked a PTHREAD_MUTEX_NORMAL mutex that was not locked"); + // 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. + if kind == this.eval_libc("PTHREAD_MUTEX_DEFAULT")? { + throw_ub_format!( + "unlocked a PTHREAD_MUTEX_DEFAULT mutex that was not locked by the current thread" + ); + } else if kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")? { + throw_ub_format!( + "unlocked a PTHREAD_MUTEX_NORMAL mutex that was not locked by the current thread" + ); } else if kind == this.eval_libc("PTHREAD_MUTEX_ERRORCHECK")? { this.eval_libc_i32("EPERM") } else if kind == this.eval_libc("PTHREAD_MUTEX_RECURSIVE")? { this.eval_libc_i32("EPERM") } else { - throw_ub_format!("called pthread_mutex_unlock on an unsupported type of mutex"); + throw_unsup_format!("called pthread_mutex_unlock on an unsupported type of mutex"); } } } @@ -505,6 +521,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let active_thread = this.get_active_thread()?; if this.rwlock_is_locked(id) { + // Note: this will deadlock if the lock is already locked by this + // thread in any way. + // + // Relevant documentation: + // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_rwlock_wrlock.html + // An in depth discussion on this topic: + // https://github.com/rust-lang/rust/issues/53127 + // + // FIXME: Detect and report the deadlock proactively. (We currently + // report the deadlock only when no thread can continue execution, + // but we could detect that this lock is already locked and report + // an error.) this.rwlock_enqueue_and_block_writer(id, active_thread)?; } else { this.rwlock_writer_lock(id, active_thread); @@ -719,19 +747,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let clock_id = cond_get_clock_id(this, cond_op)?.to_i32()?; let duration = { let tp = this.deref_operand(abstime_op)?; - let mut offset = Size::from_bytes(0); - let layout = this.libc_ty_layout("time_t")?; - let seconds_place = tp.offset(offset, MemPlaceMeta::None, layout, this)?; + let seconds_place = this.mplace_field(tp, 0)?; let seconds = this.read_scalar(seconds_place.into())?; - offset += layout.size; - let layout = this.libc_ty_layout("c_long")?; - let nanoseconds_place = tp.offset(offset, MemPlaceMeta::None, layout, this)?; + let nanoseconds_place = this.mplace_field(tp, 1)?; let nanoseconds = this.read_scalar(nanoseconds_place.into())?; - let (seconds, nanoseconds) = if this.pointer_size().bytes() == 8 { - (seconds.to_u64()?, nanoseconds.to_u64()?.try_into().unwrap()) - } else { - (seconds.to_u32()?.into(), nanoseconds.to_u32()?) - }; + let (seconds, nanoseconds) = ( + seconds.to_machine_usize(this)?, + nanoseconds.to_machine_usize(this)?.try_into().unwrap(), + ); Duration::new(seconds, nanoseconds) }; @@ -740,7 +763,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } else if clock_id == this.eval_libc_i32("CLOCK_MONOTONIC")? { Time::Monotonic(this.machine.time_anchor.checked_add(duration).unwrap()) } else { - throw_unsup_format!("Unsupported clock id."); + throw_unsup_format!("unsupported clock id: {}", clock_id); }; // Register the timeout callback. diff --git a/src/sync.rs b/src/sync.rs index cbae29bdbb3..026542926ed 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -30,10 +30,12 @@ macro_rules! declare_id { // We use 0 as a sentinel value (see the comment above) and, // therefore, need to shift by one when converting from an index // into a vector. - $name(NonZeroU32::new(u32::try_from(idx).unwrap() + 1).unwrap()) + let shifted_idx = u32::try_from(idx).unwrap().checked_add(1).unwrap(); + $name(NonZeroU32::new(shifted_idx).unwrap()) } fn index(self) -> usize { // See the comment in `Self::new`. + // (This cannot underflow because self is NonZeroU32.) usize::try_from(self.0.get() - 1).unwrap() } } @@ -150,11 +152,19 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx /// /// Note: It is the caller's responsibility to check that the thread that /// unlocked the lock actually is the same one, which owned it. - fn mutex_unlock(&mut self, id: MutexId) -> InterpResult<'tcx, Option<(ThreadId, usize)>> { + fn mutex_unlock( + &mut self, + id: MutexId, + expected_owner: ThreadId, + ) -> InterpResult<'tcx, Option> { let this = self.eval_context_mut(); let mutex = &mut this.machine.threads.sync.mutexes[id]; if let Some(current_owner) = mutex.owner { // Mutex is locked. + if current_owner != expected_owner { + // Only the owner can unlock the mutex. + return Ok(None); + } let old_lock_count = mutex.lock_count; mutex.lock_count = old_lock_count .checked_sub(1) @@ -168,7 +178,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.unblock_thread(new_owner)?; } } - Ok(Some((current_owner, old_lock_count))) + Ok(Some(old_lock_count)) } else { // Mutex is unlocked. Ok(None) diff --git a/src/thread.rs b/src/thread.rs index 70c2419c4d4..45b07477fa2 100644 --- a/src/thread.rs +++ b/src/thread.rs @@ -159,6 +159,7 @@ impl<'mir, 'tcx> Default for Thread<'mir, 'tcx> { } } +/// A specific moment in time. #[derive(Debug)] pub enum Time { Monotonic(Instant), @@ -449,9 +450,9 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { // // Documentation: // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_cond_timedwait.html# - if let Some(sleep_time) = - self.timeout_callbacks.values().map(|info| info.call_time.get_wait_time()).min() - { + let potential_sleep_time = + self.timeout_callbacks.values().map(|info| info.call_time.get_wait_time()).min(); + if let Some(sleep_time) = potential_sleep_time { if sleep_time == Duration::new(0, 0) { return Ok(SchedulingAction::ExecuteTimeoutCallback); } @@ -479,9 +480,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { // We have not found a thread to execute. if self.threads.iter().all(|thread| thread.state == ThreadState::Terminated) { unreachable!("all threads terminated without the main thread terminating?!"); - } else if let Some(sleep_time) = - self.timeout_callbacks.values().map(|info| info.call_time.get_wait_time()).min() - { + } else if let Some(sleep_time) = potential_sleep_time { // All threads are currently blocked, but we have unexecuted // timeout_callbacks, which may unblock some of the threads. Hence, // sleep until the first callback. diff --git a/tests/compile-fail/sync/libc_pthread_mutex_normal_deadlock.rs b/tests/compile-fail/sync/libc_pthread_mutex_normal_deadlock.rs index 7034bf64ec9..4af8ee5df4b 100644 --- a/tests/compile-fail/sync/libc_pthread_mutex_normal_deadlock.rs +++ b/tests/compile-fail/sync/libc_pthread_mutex_normal_deadlock.rs @@ -11,6 +11,8 @@ fn main() { let mut mutex: libc::pthread_mutex_t = std::mem::zeroed(); assert_eq!(libc::pthread_mutex_init(&mut mutex as *mut _, &mutexattr as *const _), 0); assert_eq!(libc::pthread_mutex_lock(&mut mutex as *mut _), 0); - libc::pthread_mutex_lock(&mut mutex as *mut _); //~ ERROR deadlock + // FIXME: The error should be deadlock. See issue + // https://github.com/rust-lang/miri/issues/1419. + libc::pthread_mutex_lock(&mut mutex as *mut _); //~ ERROR Undefined Behavior } } diff --git a/tests/compile-fail/sync/libc_pthread_mutex_wrong_owner.rs b/tests/compile-fail/sync/libc_pthread_mutex_wrong_owner.rs index 3009721abe2..e67e8d366eb 100644 --- a/tests/compile-fail/sync/libc_pthread_mutex_wrong_owner.rs +++ b/tests/compile-fail/sync/libc_pthread_mutex_wrong_owner.rs @@ -24,7 +24,7 @@ fn main() { let lock_copy = lock.clone(); thread::spawn(move || { - assert_eq!(libc::pthread_mutex_unlock(lock_copy.0.get() as *mut _), 0); //~ ERROR: Undefined Behavior: called pthread_mutex_unlock on a mutex owned by another thread + assert_eq!(libc::pthread_mutex_unlock(lock_copy.0.get() as *mut _), 0); //~ ERROR: Undefined Behavior: unlocked a PTHREAD_MUTEX_DEFAULT mutex that was not locked }) .join() .unwrap(); diff --git a/tests/run-pass/concurrency/sync.rs b/tests/run-pass/concurrency/sync.rs index faf47851bd0..2009c01ce9f 100644 --- a/tests/run-pass/concurrency/sync.rs +++ b/tests/run-pass/concurrency/sync.rs @@ -91,7 +91,8 @@ fn check_conditional_variables_timed_wait_timeout() { let now = Instant::now(); let (_guard, timeout) = cvar.wait_timeout(guard, Duration::from_millis(100)).unwrap(); assert!(timeout.timed_out()); - assert!(now.elapsed().as_millis() >= 100); + let elapsed_time = now.elapsed().as_millis(); + assert!(100 <= elapsed_time && elapsed_time <= 300); } /// Test that signaling a conditional variable when waiting with a timeout works @@ -243,7 +244,7 @@ fn get_cached_val() -> usize { fn expensive_computation() -> usize { let mut i = 1; let mut c = 1; - while i < 10000 { + while i < 1000 { i *= c; c += 1; } @@ -257,7 +258,7 @@ fn check_once() { thread::spawn(|| { thread::yield_now(); let val = get_cached_val(); - assert_eq!(val, 40320); + assert_eq!(val, 5040); }) }) .collect();