diff --git a/src/shims/sync.rs b/src/shims/sync.rs index 5432c76dfe7..ee2579c22f1 100644 --- a/src/shims/sync.rs +++ b/src/shims/sync.rs @@ -202,6 +202,7 @@ fn condattr_set_clock_id<'mir, 'tcx: 'mir>( // Our chosen memory layout for the emulated conditional variable (does not have // to match the platform layout!): +// bytes 0-3: reserved for signature on macOS // bytes 4-7: the conditional variable id as u32 or 0 if id is not assigned yet. // bytes 8-11: the clock id constant as i32 @@ -275,19 +276,13 @@ fn release_cond_mutex<'mir, 'tcx: 'mir>( active_thread: ThreadId, mutex: MutexId, ) -> InterpResult<'tcx> { - if let Some((owner_thread, current_locked_count)) = ecx.mutex_unlock(mutex) { - if current_locked_count != 0 { - throw_unsup_format!("awaiting on multiple times acquired lock is not supported"); + if let Some((old_owner_thread, old_locked_count)) = ecx.mutex_unlock(mutex)? { + if old_locked_count != 1 { + throw_unsup_format!("awaiting on a lock acquired multiple times is not supported"); } - if owner_thread != active_thread { + if old_owner_thread != active_thread { throw_ub_format!("awaiting on a mutex owned by a different thread"); } - if let Some(thread) = ecx.mutex_dequeue(mutex) { - // We have at least one thread waiting on this mutex. Transfer - // ownership to it. - ecx.mutex_lock(mutex, thread); - ecx.unblock_thread(thread)?; - } } else { throw_ub_format!("awaiting on unlocked mutex"); } @@ -349,7 +344,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx mutexattr_get_kind(this, attr_op)?.not_undef()? }; - let _ = mutex_get_or_create_id(this, mutex_op)?; + // Write 0 to use the same code path as the static initializers. + mutex_set_id(this, mutex_op, Scalar::from_i32(0))?; + mutex_set_kind(this, mutex_op, kind)?; Ok(0) @@ -427,19 +424,10 @@ 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)?; - if let Some((owner_thread, current_locked_count)) = this.mutex_unlock(id) { - if owner_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 current_locked_count == 0 { - // The mutex is unlocked. - if let Some(thread) = this.mutex_dequeue(id) { - // We have at least one thread waiting on this mutex. Transfer - // ownership to it. - this.mutex_lock(id, thread); - this.unblock_thread(thread)?; - } - } Ok(0) } else { if kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")? { @@ -476,11 +464,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let active_thread = this.get_active_thread()?; if this.rwlock_is_write_locked(id) { - this.rwlock_enqueue_reader(id, active_thread); - this.block_thread(active_thread)?; + this.rwlock_enqueue_and_block_reader(id, active_thread)?; Ok(0) } else { - this.rwlock_reader_add(id, active_thread); + this.rwlock_reader_lock(id, active_thread); Ok(0) } } @@ -494,7 +481,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if this.rwlock_is_write_locked(id) { this.eval_libc_i32("EBUSY") } else { - this.rwlock_reader_add(id, active_thread); + this.rwlock_reader_lock(id, active_thread); Ok(0) } } @@ -506,10 +493,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let active_thread = this.get_active_thread()?; if this.rwlock_is_locked(id) { - this.block_thread(active_thread)?; - this.rwlock_enqueue_writer(id, active_thread); + this.rwlock_enqueue_and_block_writer(id, active_thread)?; } else { - this.rwlock_writer_set(id, active_thread); + this.rwlock_writer_lock(id, active_thread); } Ok(0) @@ -524,7 +510,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if this.rwlock_is_locked(id) { this.eval_libc_i32("EBUSY") } else { - this.rwlock_writer_set(id, active_thread); + this.rwlock_writer_lock(id, active_thread); Ok(0) } } @@ -535,18 +521,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let id = rwlock_get_or_create_id(this, rwlock_op)?; let active_thread = this.get_active_thread()?; - if this.rwlock_reader_remove(id, active_thread) { + if this.rwlock_reader_unlock(id, active_thread) { // The thread was a reader. if this.rwlock_is_locked(id) { // No more readers owning the lock. Give it to a writer if there // is any. if let Some(writer) = this.rwlock_dequeue_writer(id) { this.unblock_thread(writer)?; - this.rwlock_writer_set(id, writer); + this.rwlock_writer_lock(id, writer); } } Ok(0) - } else if Some(active_thread) == this.rwlock_writer_remove(id) { + } else if Some(active_thread) == this.rwlock_writer_unlock(id) { // The thread was a writer. // // We are prioritizing writers here against the readers. As a @@ -555,12 +541,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if let Some(writer) = this.rwlock_dequeue_writer(id) { // Give the lock to another writer. this.unblock_thread(writer)?; - this.rwlock_writer_set(id, writer); + this.rwlock_writer_lock(id, writer); } else { // Give the lock to all readers. while let Some(reader) = this.rwlock_dequeue_reader(id) { this.unblock_thread(reader)?; - this.rwlock_reader_add(id, reader); + this.rwlock_reader_lock(id, reader); } } Ok(0) @@ -586,6 +572,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx fn pthread_condattr_init(&mut self, attr_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); + // The default value of the clock attribute shall refer to the system + // clock. + // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_condattr_setclock.html let default_clock_id = this.eval_libc("CLOCK_REALTIME")?; condattr_set_clock_id(this, attr_op, default_clock_id)?; @@ -647,7 +636,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx condattr_get_clock_id(this, attr_op)?.not_undef()? }; - let _ = cond_get_or_create_id(this, cond_op)?; + // Write 0 to use the same code path as the static initializers. + cond_set_id(this, cond_op, Scalar::from_i32(0))?; + cond_set_clock_id(this, cond_op, clock_id)?; Ok(0) diff --git a/src/sync.rs b/src/sync.rs index 7957faeb7e3..a71d4597c66 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -1,6 +1,7 @@ use std::collections::{hash_map::Entry, HashMap, VecDeque}; use std::convert::TryFrom; use std::num::NonZeroU32; +use std::ops::Not; use rustc_index::vec::{Idx, IndexVec}; @@ -142,34 +143,47 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx mutex.lock_count = mutex.lock_count.checked_add(1).unwrap(); } - /// Unlock by decreasing the lock count. If the lock count reaches 0, unset - /// the owner. - fn mutex_unlock(&mut self, id: MutexId) -> Option<(ThreadId, usize)> { + /// Try unlocking by decreasing the lock count and returning the old owner + /// and the old lock count. If the lock count reaches 0, release the lock + /// and potentially give to a new owner. If the lock was not locked, return + /// `None`. + /// + /// 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)>> { let this = self.eval_context_mut(); let mutex = &mut this.machine.threads.sync.mutexes[id]; if let Some(current_owner) = mutex.owner { - mutex.lock_count = mutex - .lock_count + // Mutex is locked. + let old_lock_count = mutex.lock_count; + mutex.lock_count = old_lock_count .checked_sub(1) .expect("invariant violation: lock_count == 0 iff the thread is unlocked"); if mutex.lock_count == 0 { mutex.owner = None; + // The mutex is completely unlocked. Try transfering ownership + // to another thread. + if let Some(new_owner) = this.mutex_dequeue(id) { + this.mutex_lock(id, new_owner); + this.unblock_thread(new_owner)?; + } } - Some((current_owner, mutex.lock_count)) + Ok(Some((current_owner, old_lock_count))) } else { - None + // Mutex is unlocked. + Ok(None) } } #[inline] - /// Take a thread out the queue waiting for the lock. + /// Put the thread into the queue waiting for the lock. fn mutex_enqueue(&mut self, id: MutexId, thread: ThreadId) { let this = self.eval_context_mut(); this.machine.threads.sync.mutexes[id].queue.push_back(thread); } #[inline] - /// Take a thread out the queue waiting for the lock. + /// Take a thread out of the queue waiting for the lock. fn mutex_dequeue(&mut self, id: MutexId) -> Option { let this = self.eval_context_mut(); this.machine.threads.sync.mutexes[id].queue.pop_front() @@ -187,7 +201,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx fn rwlock_is_locked(&mut self, id: RwLockId) -> bool { let this = self.eval_context_mut(); this.machine.threads.sync.rwlocks[id].writer.is_some() - || !this.machine.threads.sync.rwlocks[id].readers.is_empty() + || this.machine.threads.sync.rwlocks[id].readers.is_empty().not() } #[inline] @@ -197,16 +211,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.machine.threads.sync.rwlocks[id].writer.is_some() } - /// Add a reader that collectively with other readers owns the lock. - fn rwlock_reader_add(&mut self, id: RwLockId, reader: ThreadId) { + /// Read-lock the lock by adding the `reader` the list of threads that own + /// this lock. + fn rwlock_reader_lock(&mut self, id: RwLockId, reader: ThreadId) { let this = self.eval_context_mut(); assert!(!this.rwlock_is_write_locked(id), "the lock is write locked"); let count = this.machine.threads.sync.rwlocks[id].readers.entry(reader).or_insert(0); - *count += 1; + *count = count.checked_add(1).expect("the reader counter overflowed"); } - /// Try removing the reader. Returns `true` if succeeded. - fn rwlock_reader_remove(&mut self, id: RwLockId, reader: ThreadId) -> bool { + /// Try read-unlock the lock for `reader`. Returns `true` if succeeded, + /// `false` if this `reader` did not hold the lock. + fn rwlock_reader_unlock(&mut self, id: RwLockId, reader: ThreadId) -> bool { let this = self.eval_context_mut(); match this.machine.threads.sync.rwlocks[id].readers.entry(reader) { Entry::Occupied(mut entry) => { @@ -222,15 +238,20 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } #[inline] - /// Put the reader in the queue waiting for the lock. - fn rwlock_enqueue_reader(&mut self, id: RwLockId, reader: ThreadId) { + /// Put the reader in the queue waiting for the lock and block it. + fn rwlock_enqueue_and_block_reader( + &mut self, + id: RwLockId, + reader: ThreadId, + ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); assert!(this.rwlock_is_write_locked(id), "queueing on not write locked lock"); this.machine.threads.sync.rwlocks[id].reader_queue.push_back(reader); + this.block_thread(reader) } #[inline] - /// Take the reader out the queue waiting for the lock. + /// Take a reader out the queue waiting for the lock. fn rwlock_dequeue_reader(&mut self, id: RwLockId) -> Option { let this = self.eval_context_mut(); this.machine.threads.sync.rwlocks[id].reader_queue.pop_front() @@ -238,25 +259,30 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx #[inline] /// Lock by setting the writer that owns the lock. - fn rwlock_writer_set(&mut self, id: RwLockId, writer: ThreadId) { + fn rwlock_writer_lock(&mut self, id: RwLockId, writer: ThreadId) { let this = self.eval_context_mut(); assert!(!this.rwlock_is_locked(id), "the lock is already locked"); this.machine.threads.sync.rwlocks[id].writer = Some(writer); } #[inline] - /// Try removing the writer. - fn rwlock_writer_remove(&mut self, id: RwLockId) -> Option { + /// Try to unlock by removing the writer. + fn rwlock_writer_unlock(&mut self, id: RwLockId) -> Option { let this = self.eval_context_mut(); this.machine.threads.sync.rwlocks[id].writer.take() } #[inline] /// Put the writer in the queue waiting for the lock. - fn rwlock_enqueue_writer(&mut self, id: RwLockId, writer: ThreadId) { + fn rwlock_enqueue_and_block_writer( + &mut self, + id: RwLockId, + writer: ThreadId, + ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); assert!(this.rwlock_is_locked(id), "queueing on unlocked lock"); this.machine.threads.sync.rwlocks[id].writer_queue.push_back(writer); + this.block_thread(writer) } #[inline] diff --git a/src/thread.rs b/src/thread.rs index 69b31b541ae..e61761e599c 100644 --- a/src/thread.rs +++ b/src/thread.rs @@ -32,7 +32,7 @@ pub enum SchedulingAction { Stop, } -/// Timeout timeout_callbacks can be created by synchronization primitives to tell the +/// Timeout callbacks can be created by synchronization primitives to tell the /// scheduler that they should be called once some period of time passes. type TimeoutCallback<'mir, 'tcx> = Box>) -> InterpResult<'tcx> + 'tcx>; @@ -189,7 +189,7 @@ struct TimeoutCallbackInfo<'mir, 'tcx> { impl<'mir, 'tcx> std::fmt::Debug for TimeoutCallbackInfo<'mir, 'tcx> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "CallBack({:?})", self.call_time) + write!(f, "TimeoutCallback({:?})", self.call_time) } } @@ -394,7 +394,8 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { /// Get a callback that is ready to be called. fn get_ready_callback(&mut self) -> Option<(ThreadId, TimeoutCallback<'mir, 'tcx>)> { - // We use a for loop here to make the scheduler more deterministic. + // We iterate over all threads in the order of their indices because + // this allows us to have a deterministic scheduler. for thread in self.threads.indices() { match self.timeout_callbacks.entry(thread) { Entry::Occupied(entry) => diff --git a/tests/compile-fail/sync/libc_pthread_rwlock_read_wrong_owner.rs b/tests/compile-fail/sync/libc_pthread_rwlock_read_wrong_owner.rs new file mode 100644 index 00000000000..a73a8496a32 --- /dev/null +++ b/tests/compile-fail/sync/libc_pthread_rwlock_read_wrong_owner.rs @@ -0,0 +1,32 @@ +// ignore-windows: No libc on Windows + +#![feature(rustc_private)] + +extern crate libc; + +use std::cell::UnsafeCell; +use std::sync::Arc; +use std::thread; + +struct RwLock(UnsafeCell); + +unsafe impl Send for RwLock {} +unsafe impl Sync for RwLock {} + +fn new_lock() -> Arc { + Arc::new(RwLock(UnsafeCell::new(libc::PTHREAD_RWLOCK_INITIALIZER))) +} + +fn main() { + unsafe { + let lock = new_lock(); + assert_eq!(libc::pthread_rwlock_rdlock(lock.0.get() as *mut _), 0); + + let lock_copy = lock.clone(); + thread::spawn(move || { + assert_eq!(libc::pthread_rwlock_unlock(lock_copy.0.get() as *mut _), 0); //~ ERROR: Undefined Behavior: unlocked an rwlock that was not locked by the active thread + }) + .join() + .unwrap(); + } +} diff --git a/tests/compile-fail/sync/libc_pthread_rwlock_write_wrong_owner.rs b/tests/compile-fail/sync/libc_pthread_rwlock_write_wrong_owner.rs new file mode 100644 index 00000000000..663dedb6f6f --- /dev/null +++ b/tests/compile-fail/sync/libc_pthread_rwlock_write_wrong_owner.rs @@ -0,0 +1,32 @@ +// ignore-windows: No libc on Windows + +#![feature(rustc_private)] + +extern crate libc; + +use std::cell::UnsafeCell; +use std::sync::Arc; +use std::thread; + +struct RwLock(UnsafeCell); + +unsafe impl Send for RwLock {} +unsafe impl Sync for RwLock {} + +fn new_lock() -> Arc { + Arc::new(RwLock(UnsafeCell::new(libc::PTHREAD_RWLOCK_INITIALIZER))) +} + +fn main() { + unsafe { + let lock = new_lock(); + assert_eq!(libc::pthread_rwlock_wrlock(lock.0.get() as *mut _), 0); + + let lock_copy = lock.clone(); + thread::spawn(move || { + assert_eq!(libc::pthread_rwlock_unlock(lock_copy.0.get() as *mut _), 0); //~ ERROR: Undefined Behavior: unlocked an rwlock that was not locked by the active thread + }) + .join() + .unwrap(); + } +}