diff --git a/library/std/src/sys/sync/once/queue.rs b/library/std/src/sys/sync/once/queue.rs index 730cdb768bd27..715d543a661a6 100644 --- a/library/std/src/sys/sync/once/queue.rs +++ b/library/std/src/sys/sync/once/queue.rs @@ -59,27 +59,35 @@ use crate::cell::Cell; use crate::fmt; use crate::ptr; use crate::sync as public; -use crate::sync::atomic::{AtomicBool, AtomicPtr, Ordering}; +use crate::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use crate::sync::once::ExclusiveState; use crate::thread::{self, Thread}; -type Masked = (); +// FIXME(#121950): don't use fuzzy provenance. +// Using pointers here can result in ABA-problems because CAS does not check for +// equal provenance, which means it can succeed even though the pointer in the +// `next` field needs updating. This can lead to a situation where we try to access +// a freshly allocated `Waiter` with a pointer to a previous allocation at the same +// address, which is UB. By exposing all `Waiters`, we ensure that the +// `from_exposed_addr` can guess the right provenance. +type State = usize; +type AtomicState = AtomicUsize; pub struct Once { - state_and_queue: AtomicPtr, + state_and_queue: AtomicState, } pub struct OnceState { poisoned: bool, - set_state_on_drop_to: Cell<*mut Masked>, + set_state_on_drop_to: Cell, } // Four states that a Once can be in, encoded into the lower bits of // `state_and_queue` in the Once structure. -const INCOMPLETE: usize = 0x0; -const POISONED: usize = 0x1; +const INCOMPLETE: State = 0x0; +const POISONED: State = 0x1; const RUNNING: usize = 0x2; -const COMPLETE: usize = 0x3; +const COMPLETE: State = 0x3; // Mask to learn about the state. All other bits are the queue of waiters if // this is in the RUNNING state. @@ -95,22 +103,22 @@ const STATE_MASK: usize = 0x3; struct Waiter { thread: Cell>, signaled: AtomicBool, - next: *const Waiter, + next: usize, // actually an exposed pointer } // Head of a linked list of waiters. // Every node is a struct on the stack of a waiting thread. // Will wake up the waiters when it gets dropped, i.e. also on panic. struct WaiterQueue<'a> { - state_and_queue: &'a AtomicPtr, - set_state_on_drop_to: *mut Masked, + state_and_queue: &'a AtomicState, + set_state_on_drop_to: State, } impl Once { #[inline] #[rustc_const_stable(feature = "const_once_new", since = "1.32.0")] pub const fn new() -> Once { - Once { state_and_queue: AtomicPtr::new(ptr::without_provenance_mut(INCOMPLETE)) } + Once { state_and_queue: AtomicState::new(INCOMPLETE) } } #[inline] @@ -119,12 +127,12 @@ impl Once { // operations visible to us, and, this being a fast path, weaker // ordering helps with performance. This `Acquire` synchronizes with // `Release` operations on the slow path. - self.state_and_queue.load(Ordering::Acquire).addr() == COMPLETE + self.state_and_queue.load(Ordering::Acquire) == COMPLETE } #[inline] pub(crate) fn state(&mut self) -> ExclusiveState { - match self.state_and_queue.get_mut().addr() { + match *self.state_and_queue.get_mut() { INCOMPLETE => ExclusiveState::Incomplete, POISONED => ExclusiveState::Poisoned, COMPLETE => ExclusiveState::Complete, @@ -148,7 +156,7 @@ impl Once { pub fn call(&self, ignore_poisoning: bool, init: &mut dyn FnMut(&public::OnceState)) { let mut state_and_queue = self.state_and_queue.load(Ordering::Acquire); loop { - match state_and_queue.addr() { + match state_and_queue { COMPLETE => break, POISONED if !ignore_poisoning => { // Panic to propagate the poison. @@ -158,7 +166,7 @@ impl Once { // Try to register this thread as the one RUNNING. let exchange_result = self.state_and_queue.compare_exchange( state_and_queue, - ptr::without_provenance_mut(RUNNING), + RUNNING, Ordering::Acquire, Ordering::Acquire, ); @@ -170,14 +178,14 @@ impl Once { // wake them up on drop. let mut waiter_queue = WaiterQueue { state_and_queue: &self.state_and_queue, - set_state_on_drop_to: ptr::without_provenance_mut(POISONED), + set_state_on_drop_to: POISONED, }; // Run the initialization function, letting it know if we're // poisoned or not. let init_state = public::OnceState { inner: OnceState { - poisoned: state_and_queue.addr() == POISONED, - set_state_on_drop_to: Cell::new(ptr::without_provenance_mut(COMPLETE)), + poisoned: state_and_queue == POISONED, + set_state_on_drop_to: Cell::new(COMPLETE), }, }; init(&init_state); @@ -187,7 +195,7 @@ impl Once { _ => { // All other values must be RUNNING with possibly a // pointer to the waiter queue in the more significant bits. - assert!(state_and_queue.addr() & STATE_MASK == RUNNING); + assert!(state_and_queue & STATE_MASK == RUNNING); wait(&self.state_and_queue, state_and_queue); state_and_queue = self.state_and_queue.load(Ordering::Acquire); } @@ -196,13 +204,13 @@ impl Once { } } -fn wait(state_and_queue: &AtomicPtr, mut current_state: *mut Masked) { +fn wait(state_and_queue: &AtomicState, mut current_state: State) { // Note: the following code was carefully written to avoid creating a // mutable reference to `node` that gets aliased. loop { // Don't queue this thread if the status is no longer running, // otherwise we will not be woken up. - if current_state.addr() & STATE_MASK != RUNNING { + if current_state & STATE_MASK != RUNNING { return; } @@ -210,15 +218,15 @@ fn wait(state_and_queue: &AtomicPtr, mut current_state: *mut Masked) { let node = Waiter { thread: Cell::new(Some(thread::current())), signaled: AtomicBool::new(false), - next: current_state.with_addr(current_state.addr() & !STATE_MASK) as *const Waiter, + next: current_state & !STATE_MASK, }; - let me = core::ptr::addr_of!(node) as *const Masked as *mut Masked; + let me = core::ptr::addr_of!(node).expose_addr(); // Try to slide in the node at the head of the linked list, making sure // that another thread didn't just replace the head of the linked list. let exchange_result = state_and_queue.compare_exchange( current_state, - me.with_addr(me.addr() | RUNNING), + me | RUNNING, Ordering::Release, Ordering::Relaxed, ); @@ -257,7 +265,7 @@ impl Drop for WaiterQueue<'_> { self.state_and_queue.swap(self.set_state_on_drop_to, Ordering::AcqRel); // We should only ever see an old state which was RUNNING. - assert_eq!(state_and_queue.addr() & STATE_MASK, RUNNING); + assert_eq!(state_and_queue & STATE_MASK, RUNNING); // Walk the entire linked list of waiters and wake them up (in lifo // order, last to register is first to wake up). @@ -266,15 +274,12 @@ impl Drop for WaiterQueue<'_> { // free `node` if there happens to be has a spurious wakeup. // So we have to take out the `thread` field and copy the pointer to // `next` first. - let mut queue = - state_and_queue.with_addr(state_and_queue.addr() & !STATE_MASK) as *const Waiter; + let mut queue = ptr::from_exposed_addr::(state_and_queue & !STATE_MASK); while !queue.is_null() { let next = (*queue).next; let thread = (*queue).thread.take().unwrap(); (*queue).signaled.store(true, Ordering::Release); - // ^- FIXME (maybe): This is another case of issue #55005 - // `store()` has a potentially dangling ref to `signaled`. - queue = next; + queue = ptr::from_exposed_addr::(next); thread.unpark(); } } @@ -289,6 +294,6 @@ impl OnceState { #[inline] pub fn poison(&self) { - self.set_state_on_drop_to.set(ptr::without_provenance_mut(POISONED)); + self.set_state_on_drop_to.set(POISONED); } } diff --git a/library/std/src/sys/sync/rwlock/queue.rs b/library/std/src/sys/sync/rwlock/queue.rs index dce966086b8ff..848e6e3a1a4b5 100644 --- a/library/std/src/sys/sync/rwlock/queue.rs +++ b/library/std/src/sys/sync/rwlock/queue.rs @@ -110,9 +110,9 @@ use crate::cell::OnceCell; use crate::hint::spin_loop; use crate::mem; -use crate::ptr::{self, null_mut, without_provenance_mut, NonNull}; +use crate::ptr::{self, null_mut, NonNull}; use crate::sync::atomic::{ - AtomicBool, AtomicPtr, + AtomicBool, AtomicPtr, AtomicUsize, Ordering::{AcqRel, Acquire, Relaxed, Release}, }; use crate::sys_common::thread_info; @@ -123,10 +123,17 @@ use crate::thread::Thread; // `spin_loop` will be called `2.pow(SPIN_COUNT) - 1` times. const SPIN_COUNT: usize = 7; -type State = *mut (); -type AtomicState = AtomicPtr<()>; - -const UNLOCKED: State = without_provenance_mut(0); +// FIXME(#121950): don't use fuzzy provenance. +// Using pointers here can result in ABA-problems because CAS does not check for +// equal provenance, which means it can succeed even though the pointer in the +// `next` field needs updating. This can lead to a situation where we try to access +// a freshly allocated `Node` with a pointer to a previous allocation at the same +// address, which is UB. By exposing all `Node`, we ensure that the +// `from_exposed_addr` can guess the right provenance. +type State = usize; +type AtomicState = AtomicUsize; + +const UNLOCKED: State = 0; const LOCKED: usize = 1; const QUEUED: usize = 2; const QUEUE_LOCKED: usize = 4; @@ -136,15 +143,15 @@ const MASK: usize = !(QUEUE_LOCKED | QUEUED | LOCKED); /// Marks the state as write-locked, if possible. #[inline] fn write_lock(state: State) -> Option { - let state = state.wrapping_byte_add(LOCKED); - if state.addr() & LOCKED == LOCKED { Some(state) } else { None } + let state = state.wrapping_add(LOCKED); + if state & LOCKED == LOCKED { Some(state) } else { None } } /// Marks the state as read-locked, if possible. #[inline] fn read_lock(state: State) -> Option { - if state.addr() & QUEUED == 0 && state.addr() != LOCKED { - Some(without_provenance_mut(state.addr().checked_add(SINGLE)? | LOCKED)) + if state & QUEUED == 0 && state != LOCKED { + Some(state.checked_add(SINGLE)? | LOCKED) } else { None } @@ -156,7 +163,7 @@ fn read_lock(state: State) -> Option { /// The state must contain a valid pointer to a queue node. #[inline] unsafe fn to_node(state: State) -> NonNull { - unsafe { NonNull::new_unchecked(state.mask(MASK)).cast() } + unsafe { NonNull::new_unchecked(ptr::from_exposed_addr_mut::(state & MASK)) } } /// An atomic node pointer with relaxed operations. @@ -178,7 +185,7 @@ impl AtomicLink { #[repr(align(8))] struct Node { - next: AtomicLink, + next: AtomicUsize, // actually an exposed pointer prev: AtomicLink, tail: AtomicLink, write: bool, @@ -190,7 +197,7 @@ impl Node { /// Create a new queue node. fn new(write: bool) -> Node { Node { - next: AtomicLink::new(None), + next: AtomicUsize::new(0), prev: AtomicLink::new(None), tail: AtomicLink::new(None), write, @@ -261,7 +268,8 @@ unsafe fn add_backlinks_and_find_tail(head: NonNull) -> NonNull { // All `next` fields before the first node with a `set` tail are // non-null and valid (invariant 3). None => unsafe { - let next = c.next.get().unwrap_unchecked(); + let next = c.next.load(Relaxed); + let next = NonNull::new_unchecked(ptr::from_exposed_addr_mut::(next)); next.as_ref().prev.set(Some(current)); current = next; }, @@ -281,7 +289,7 @@ pub struct RwLock { impl RwLock { #[inline] pub const fn new() -> RwLock { - RwLock { state: AtomicPtr::new(UNLOCKED) } + RwLock { state: AtomicState::new(UNLOCKED) } } #[inline] @@ -303,7 +311,7 @@ impl RwLock { // "ldseta" on modern AArch64), and therefore is more efficient than // `fetch_update(lock(true))`, which can spuriously fail if a new node // is appended to the queue. - self.state.fetch_or(LOCKED, Acquire).addr() & LOCKED == 0 + self.state.fetch_or(LOCKED, Acquire) & LOCKED == 0 } #[inline] @@ -326,7 +334,7 @@ impl RwLock { Ok(_) => return, Err(new) => state = new, } - } else if state.addr() & QUEUED == 0 && count < SPIN_COUNT { + } else if state & QUEUED == 0 && count < SPIN_COUNT { // If the lock is not available and no threads are queued, spin // for a while, using exponential backoff to decrease cache // contention. @@ -343,13 +351,11 @@ impl RwLock { // pointer to the next node in the queue. Otherwise set it to // the lock count if the state is read-locked or to zero if it // is write-locked. - node.next.0 = AtomicPtr::new(state.mask(MASK).cast()); + node.next = AtomicUsize::new(state & MASK); node.prev = AtomicLink::new(None); - let mut next = ptr::from_ref(&node) - .map_addr(|addr| addr | QUEUED | (state.addr() & LOCKED)) - as State; + let mut next = ptr::from_ref(&node).expose_addr() | QUEUED | (state & LOCKED); - if state.addr() & QUEUED == 0 { + if state & QUEUED == 0 { // If this is the first node in the queue, set the tail field to // the node itself to ensure there is a current `tail` field in // the queue (invariants 1 and 2). This needs to use `set` to @@ -359,7 +365,7 @@ impl RwLock { // Otherwise, the tail of the queue is not known. node.tail.set(None); // Try locking the queue to eagerly add backlinks. - next = next.map_addr(|addr| addr | QUEUE_LOCKED); + next = next | QUEUE_LOCKED; } // Register the node, using release ordering to propagate our @@ -378,7 +384,7 @@ impl RwLock { // If the current thread locked the queue, unlock it again, // linking it in the process. - if state.addr() & (QUEUE_LOCKED | QUEUED) == QUEUED { + if state & (QUEUE_LOCKED | QUEUED) == QUEUED { unsafe { self.unlock_queue(next); } @@ -403,9 +409,9 @@ impl RwLock { #[inline] pub unsafe fn read_unlock(&self) { match self.state.fetch_update(Release, Acquire, |state| { - if state.addr() & QUEUED == 0 { - let count = state.addr() - (SINGLE | LOCKED); - Some(if count > 0 { without_provenance_mut(count | LOCKED) } else { UNLOCKED }) + if state & QUEUED == 0 { + let count = state - (SINGLE | LOCKED); + Some(if count > 0 { count + LOCKED } else { UNLOCKED }) } else { None } @@ -431,7 +437,7 @@ impl RwLock { // The lock count is stored in the `next` field of `tail`. // Decrement it, making sure to observe all changes made to the queue // by the other lock owners by using acquire-release ordering. - let was_last = tail.next.0.fetch_byte_sub(SINGLE, AcqRel).addr() - SINGLE == 0; + let was_last = tail.next.fetch_sub(SINGLE, AcqRel) - SINGLE == 0; if was_last { // SAFETY: // Other threads cannot read-lock while threads are queued. Also, @@ -443,9 +449,7 @@ impl RwLock { #[inline] pub unsafe fn write_unlock(&self) { - if let Err(state) = - self.state.compare_exchange(without_provenance_mut(LOCKED), UNLOCKED, Release, Relaxed) - { + if let Err(state) = self.state.compare_exchange(LOCKED, UNLOCKED, Release, Relaxed) { // SAFETY: // Since other threads cannot acquire the lock, the state can only // have changed because there are threads queued on the lock. @@ -460,11 +464,11 @@ impl RwLock { unsafe fn unlock_contended(&self, mut state: State) { loop { // Atomically release the lock and try to acquire the queue lock. - let next = state.map_addr(|a| (a & !LOCKED) | QUEUE_LOCKED); + let next = (state - LOCKED) | QUEUE_LOCKED; match self.state.compare_exchange_weak(state, next, AcqRel, Relaxed) { // The queue lock was acquired. Release it, waking up the next // waiter in the process. - Ok(_) if state.addr() & QUEUE_LOCKED == 0 => unsafe { + Ok(_) if state & QUEUE_LOCKED == 0 => unsafe { return self.unlock_queue(next); }, // Another thread already holds the queue lock, leave waking up @@ -481,17 +485,18 @@ impl RwLock { /// # Safety /// The queue lock must be held by the current thread. unsafe fn unlock_queue(&self, mut state: State) { - debug_assert_eq!(state.addr() & (QUEUED | QUEUE_LOCKED), QUEUED | QUEUE_LOCKED); + debug_assert_eq!(state & (QUEUED | QUEUE_LOCKED), QUEUED | QUEUE_LOCKED); loop { - let tail = unsafe { add_backlinks_and_find_tail(to_node(state)) }; + let head = unsafe { to_node(state) }; + let tail = unsafe { add_backlinks_and_find_tail(head) }; - if state.addr() & LOCKED == LOCKED { + if state & LOCKED == LOCKED { // Another thread has locked the lock. Leave waking up waiters // to them by releasing the queue lock. match self.state.compare_exchange_weak( state, - state.mask(!QUEUE_LOCKED), + state - QUEUE_LOCKED, Release, Acquire, ) { @@ -513,14 +518,14 @@ impl RwLock { // (invariant 2). Invariant 4 is fullfilled since `find_tail` // was called on this node, which ensures all backlinks are set. unsafe { - to_node(state).as_ref().tail.set(Some(prev)); + head.as_ref().tail.set(Some(prev)); } // Release the queue lock. Doing this by subtraction is more // efficient on modern processors since it is a single instruction // instead of an update loop, which will fail if new threads are // added to the list. - self.state.fetch_byte_sub(QUEUE_LOCKED, Release); + self.state.fetch_sub(QUEUE_LOCKED, Release); // The tail was split off and the lock released. Mark the node as // completed.