From 4bda2dc26d072b8882bfe59929a0c8219c90741c Mon Sep 17 00:00:00 2001 From: joboet Date: Thu, 13 Aug 2026 17:27:40 +0200 Subject: [PATCH] std: simplify SGX's `UnsafeList` --- library/std/src/sys/pal/sgx/waitqueue/mod.rs | 156 +++++++------- .../src/sys/pal/sgx/waitqueue/unsafe_list.rs | 198 ++++++++---------- 2 files changed, 164 insertions(+), 190 deletions(-) diff --git a/library/std/src/sys/pal/sgx/waitqueue/mod.rs b/library/std/src/sys/pal/sgx/waitqueue/mod.rs index 41d1413fcdee9..0bb148849f043 100644 --- a/library/std/src/sys/pal/sgx/waitqueue/mod.rs +++ b/library/std/src/sys/pal/sgx/waitqueue/mod.rs @@ -24,6 +24,7 @@ use super::abi::{thread, usercalls}; use crate::num::NonZero; use crate::ops::{Deref, DerefMut}; use crate::panic::{self, AssertUnwindSafe}; +use crate::ptr::NonNull; use crate::time::Duration; /// An queue entry in a `WaitQueue`. @@ -128,26 +129,29 @@ impl WaitQueue { /// This function does not return until this thread has been awoken. When `before_wait` panics, /// this function will abort. pub fn wait(mut guard: SpinMutexGuard<'_, WaitVariable>, before_wait: F) { - // very unsafe: check requirements of UnsafeList::push - unsafe { - let mut entry = UnsafeListEntry::new(SpinMutex::new(WaitEntry { - tcs: thread::current(), - wake: false, - })); - let entry = guard.queue.inner.push(&mut entry); - drop(guard); - if let Err(_e) = panic::catch_unwind(AssertUnwindSafe(|| before_wait())) { - rtabort!("Panic before wait on wakeup event") - } - while !entry.lock().wake { - // `entry.wake` is only set in `notify_one` and `notify_all` functions. Both ensure - // the entry is removed from the queue _before_ setting this bool. There are no - // other references to `entry`. - // don't panic, this would invalidate `entry` during unwinding - let eventset = rtunwrap!(Ok, usercalls::wait(EV_UNPARK, WAIT_INDEFINITE)); - rtassert!(eventset & EV_UNPARK == EV_UNPARK); - } + let mut entry = + UnsafeListEntry::new(SpinMutex::new(WaitEntry { tcs: thread::current(), wake: false })); + // `entry` is shadowed so that the mutable borrow created here can only + // be invalidated once the function returns and the entry is freed. + let entry = NonNull::from_mut(&mut entry); + // SAFETY: `entry` is fresh and thus cannot be part of a list yet. It + // permits mutable accesses to the entry fields and is not invalidated + // until this function returns. `entry` is removed from the list before + // that happens. + let entry = unsafe { guard.queue.inner.push(entry) }; + drop(guard); + if let Err(_e) = panic::catch_unwind(AssertUnwindSafe(|| before_wait())) { + rtabort!("Panic before wait on wakeup event") } + while !entry.lock().wake { + // don't panic, this would invalidate `entry` during unwinding + let eventset = rtunwrap!(Ok, usercalls::wait(EV_UNPARK, WAIT_INDEFINITE)); + rtassert!(eventset & EV_UNPARK == EV_UNPARK); + } + + // `entry.wake` is only set in `notify_one` and `notify_all` functions. + // Both ensure the entry is removed from the queue _before_ setting this + // bool. Thus it is safe to return now, and destroy the entry. } /// Adds the calling thread to the `WaitVariable`'s wait queue, then wait @@ -159,28 +163,35 @@ impl WaitQueue { timeout: Duration, before_wait: F, ) -> bool { - // very unsafe: check requirements of UnsafeList::push - unsafe { - let mut entry = UnsafeListEntry::new(SpinMutex::new(WaitEntry { - tcs: thread::current(), - wake: false, - })); - let entry_lock = lock.lock().queue.inner.push(&mut entry); - if let Err(_e) = panic::catch_unwind(AssertUnwindSafe(|| before_wait())) { - rtabort!("Panic before wait on wakeup event or timeout") - } - usercalls::wait_timeout(EV_UNPARK, timeout, || entry_lock.lock().wake); - // acquire the wait queue's lock first to avoid deadlock - // and ensure no other function can simultaneously access the list - // (e.g., `notify_one` or `notify_all`) - let mut guard = lock.lock(); - let success = entry_lock.lock().wake; - if !success { - // nobody is waking us up, so remove our entry from the wait queue. - guard.queue.inner.remove(&mut entry); - } - success + let mut entry = + UnsafeListEntry::new(SpinMutex::new(WaitEntry { tcs: thread::current(), wake: false })); + // `entry` is shadowed so that the mutable borrow created here can only + // be invalidated once the function returns and the entry is freed. + let entry = NonNull::from_mut(&mut entry); + // SAFETY: `entry` is fresh and thus cannot be part of a list yet. It + // permits mutable accesses to the entry fields and is not invalidated + // until this function returns. `entry` is removed from the list before + // that happens. + let entry_lock = unsafe { lock.lock().queue.inner.push(entry) }; + if let Err(_e) = panic::catch_unwind(AssertUnwindSafe(|| before_wait())) { + rtabort!("Panic before wait on wakeup event or timeout") + } + usercalls::wait_timeout(EV_UNPARK, timeout, || entry_lock.lock().wake); + // acquire the wait queue's lock first to avoid deadlock + // and ensure no other function can simultaneously access the list + // (e.g., `notify_one` or `notify_all`) + let mut guard = lock.lock(); + let success = entry_lock.lock().wake; + if success { + // another thread removed us from the list, so it is save to return + // and destroy the entry. + } else { + // nobody is waking us up, so remove our entry from the wait queue. + // SAFETY: `entry` is still part of the list. + unsafe { guard.queue.inner.remove(entry) }; } + + success } /// Either find the next waiter on the wait queue, or return the mutex @@ -191,22 +202,19 @@ impl WaitQueue { pub fn notify_one( mut guard: SpinMutexGuard<'_, WaitVariable>, ) -> Result, SpinMutexGuard<'_, WaitVariable>> { - // SAFETY: lifetime of the pop() return value is limited to the map - // closure (The closure return value is 'static). The underlying - // stack frame won't be freed until after the lock on the queue is released - // (i.e., `guard` is dropped). - unsafe { - let tcs = guard.queue.inner.pop().map(|entry| -> Tcs { - let mut entry_guard = entry.lock(); - entry_guard.wake = true; - entry_guard.tcs - }); - - if let Some(tcs) = tcs { - Ok(WaitGuard { mutex_guard: Some(guard), notified_tcs: NotifiedTcs::Single(tcs) }) - } else { - Err(guard) - } + let tcs = guard.queue.inner.pop().map(|entry| -> Tcs { + // `entry` must not be accessed again after the lock is released + // since the wait functions may assume that the entry is removed + // from the list once `wake` is set. + let mut entry_guard = entry.lock(); + entry_guard.wake = true; + entry_guard.tcs + }); + + if let Some(tcs) = tcs { + Ok(WaitGuard { mutex_guard: Some(guard), notified_tcs: NotifiedTcs::Single(tcs) }) + } else { + Err(guard) } } @@ -218,25 +226,23 @@ impl WaitQueue { pub fn notify_all( mut guard: SpinMutexGuard<'_, WaitVariable>, ) -> Result, SpinMutexGuard<'_, WaitVariable>> { - // SAFETY: lifetime of the pop() return values are limited to the - // while loop body. The underlying stack frames won't be freed until - // after the lock on the queue is released (i.e., `guard` is dropped). - unsafe { - let mut count = 0; - while let Some(entry) = guard.queue.inner.pop() { - count += 1; - let mut entry_guard = entry.lock(); - entry_guard.wake = true; - } - - if let Some(count) = NonZero::new(count) { - Ok(WaitGuard { - mutex_guard: Some(guard), - notified_tcs: NotifiedTcs::All { _count: count }, - }) - } else { - Err(guard) - } + let mut count = 0; + while let Some(entry) = guard.queue.inner.pop() { + count += 1; + // `entry` must not be accessed again after the lock is released + // since the wait functions may assume that the entry is removed + // from the list once `wake` is set. + let mut entry_guard = entry.lock(); + entry_guard.wake = true; + } + + if let Some(count) = NonZero::new(count) { + Ok(WaitGuard { + mutex_guard: Some(guard), + notified_tcs: NotifiedTcs::All { _count: count }, + }) + } else { + Err(guard) } } } diff --git a/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs b/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs index c736cab576e4d..7f4411443a514 100644 --- a/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs +++ b/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs @@ -4,153 +4,121 @@ #[cfg(test)] mod tests; -use crate::mem; use crate::ptr::NonNull; pub struct UnsafeListEntry { - next: NonNull>, - prev: NonNull>, - value: Option, + next: Option>>, + prev: Option>>, + value: T, } impl UnsafeListEntry { - fn dummy() -> Self { - UnsafeListEntry { next: NonNull::dangling(), prev: NonNull::dangling(), value: None } - } - pub fn new(value: T) -> Self { - UnsafeListEntry { value: Some(value), ..Self::dummy() } + UnsafeListEntry { next: None, prev: None, value } } } -// WARNING: self-referential struct! pub struct UnsafeList { - head_tail: NonNull>, - head_tail_entry: Option>, + head_tail: Option<(NonNull>, NonNull>)>, } impl UnsafeList { pub const fn new() -> Self { - unsafe { UnsafeList { head_tail: NonNull::new_unchecked(1 as _), head_tail_entry: None } } - } - - /// # Safety - unsafe fn init(&mut self) { - if self.head_tail_entry.is_none() { - self.head_tail_entry = Some(UnsafeListEntry::dummy()); - // SAFETY: `head_tail_entry` must be non-null, which it is because we assign it above. - self.head_tail = - unsafe { NonNull::new_unchecked(self.head_tail_entry.as_mut().unwrap()) }; - // SAFETY: `self.head_tail` must meet all requirements for a mutable reference. - unsafe { self.head_tail.as_mut() }.next = self.head_tail; - unsafe { self.head_tail.as_mut() }.prev = self.head_tail; - } - } - - pub fn is_empty(&self) -> bool { - if self.head_tail_entry.is_some() { - let first = unsafe { self.head_tail.as_ref() }.next; - if first == self.head_tail { - // ,-------> /---------\ next ---, - // | |head_tail| | - // `--- prev \---------/ <-------` - // SAFETY: `self.head_tail` must meet all requirements for a reference. - unsafe { rtassert!(self.head_tail.as_ref().prev == first) }; - true - } else { - false - } - } else { - true - } + UnsafeList { head_tail: None } } /// Pushes an entry onto the back of the list. /// /// # Safety /// - /// The entry must remain allocated until the entry is removed from the - /// list AND the caller who popped is done using the entry. Special - /// care must be taken in the caller of `push` to ensure unwinding does - /// not destroy the stack frame containing the entry. - pub unsafe fn push<'a>(&mut self, entry: &'a mut UnsafeListEntry) -> &'a T { - unsafe { self.init() }; + /// `entry` must be valid for writes to an `UnsafeListEntry` that may not be + /// currently part of an `UnsafeList`, and must remain valid until the entry + /// has been removed from the list. + /// + /// The immutable reference to the value returned by this function is valid + /// for as long as `entry` remains valid. Notably, it will not be invalidated + /// by operations on `self`. + pub unsafe fn push<'a>(&mut self, entry: NonNull>) -> &'a T { + if let Some((head, tail)) = self.head_tail { + // SAFETY: `tail` belongs to the current list and therefore its `next` + // field must be writable. + unsafe { (*tail.as_ptr()).next = Some(entry) }; + self.head_tail = Some((head, entry)); + } else { + // The list was previously empty, so the new entry is both the head + // and tail node. + self.head_tail = Some((entry, entry)); + } - // BEFORE: - // /---------\ next ---> /---------\ - // ... |prev_tail| |head_tail| ... - // \---------/ <--- prev \---------/ - // - // AFTER: - // /---------\ next ---> /-----\ next ---> /---------\ - // ... |prev_tail| |entry| |head_tail| ... - // \---------/ <--- prev \-----/ <--- prev \---------/ - let mut entry = unsafe { NonNull::new_unchecked(entry) }; - let mut prev_tail = mem::replace(&mut unsafe { self.head_tail.as_mut() }.prev, entry); - // SAFETY: `entry` must meet all requirements for a mutable reference. - unsafe { entry.as_mut() }.prev = prev_tail; - unsafe { entry.as_mut() }.next = self.head_tail; - // SAFETY: `prev_tail` must meet all requirements for a mutable reference. - unsafe { prev_tail.as_mut() }.next = entry; - // unwrap ok: always `Some` on non-dummy entries - unsafe { (*entry.as_ptr()).value.as_ref() }.unwrap() + // SAFETY: the value field is only accessed via shared reference for as + // long as the entry is part of the list. + unsafe { &(*entry.as_ptr()).value } } /// Pops an entry from the front of the list. - /// - /// # Safety - /// - /// The caller must make sure to synchronize ending the borrow of the - /// return value and deallocation of the containing entry. - pub unsafe fn pop<'a>(&mut self) -> Option<&'a T> { - unsafe { self.init() }; - - if self.is_empty() { - None + pub fn pop(&mut self) -> Option<&T> { + let (head, tail) = self.head_tail?; + if let Some(next) = unsafe { (*head.as_ptr()).next } { + // SAFETY: the `next` node must still be part of the list, and thus + // its `prev` pointer must be writable. + unsafe { (*next.as_ptr()).prev = None }; + self.head_tail = Some((next, tail)); } else { - // BEFORE: - // /---------\ next ---> /-----\ next ---> /------\ - // ... |head_tail| |first| |second| ... - // \---------/ <--- prev \-----/ <--- prev \------/ - // - // AFTER: - // /---------\ next ---> /------\ - // ... |head_tail| |second| ... - // \---------/ <--- prev \------/ - let mut first = unsafe { self.head_tail.as_mut() }.next; - let mut second = unsafe { first.as_mut() }.next; - unsafe { self.head_tail.as_mut() }.next = second; - unsafe { second.as_mut() }.prev = self.head_tail; - unsafe { first.as_mut() }.next = NonNull::dangling(); - unsafe { first.as_mut() }.prev = NonNull::dangling(); - // unwrap ok: always `Some` on non-dummy entries - Some(unsafe { (*first.as_ptr()).value.as_ref() }.unwrap()) + // There is only a single node in the list, so from now on it will + // be empty. + self.head_tail = None; } + + // SAFETY: the entry pointer passed to `push` may only be invalidated + // once the removal from the list has been observed, which requires + // either access to the list (which would also mark the end of the + // lifetime of this references since we have a mutable reference) or + // another mechanism, where the caller of this function communicates + // the removal to the thread in question and thus is aware of the + // potential invalidation of this reference. + Some(unsafe { &(*head.as_ptr()).value }) } - /// Removes an entry from the list. + /// Removes a given entry from the list. /// /// # Safety /// /// The caller must ensure that `entry` has been pushed onto `self` - /// prior to this call and has not moved since then. - pub unsafe fn remove(&mut self, entry: &mut UnsafeListEntry) { - rtassert!(!self.is_empty()); - // BEFORE: - // /----\ next ---> /-----\ next ---> /----\ - // ... |prev| |entry| |next| ... - // \----/ <--- prev \-----/ <--- prev \----/ - // - // AFTER: - // /----\ next ---> /----\ - // ... |prev| |next| ... - // \----/ <--- prev \----/ - let mut prev = entry.prev; - let mut next = entry.next; - // SAFETY: `prev` and `next` must meet all requirements for a mutable reference.entry - unsafe { prev.as_mut() }.next = next; - unsafe { next.as_mut() }.prev = prev; - entry.next = NonNull::dangling(); - entry.prev = NonNull::dangling(); + /// prior to this call and has not been removed since then. This implies + /// that `entry` must have the same provenance as the pointer passed to + /// `push`. + pub unsafe fn remove(&mut self, entry: NonNull>) { + let Some((head, tail)) = self.head_tail else { + rtabort!("list cannot be empty"); + }; + + // SAFETY: `entry` must be in the list, so its `prev` field must be + // accessible. + let prev = unsafe { (*entry.as_ptr()).prev }; + // SAFETY: same argument as above. + let next = unsafe { (*entry.as_ptr()).next }; + + match (prev, next) { + // SAFETY: same argument as above, these nodes are in the list. + (Some(prev), Some(next)) => unsafe { + (*prev.as_ptr()).next = Some(next); + (*next.as_ptr()).prev = Some(prev); + }, + // SAFETY: same argument as above, these nodes are in the list. + (Some(prev), None) => unsafe { + (*prev.as_ptr()).next = None; + self.head_tail = Some((head, prev)); + }, + // SAFETY: same argument as above, these nodes are in the list. + (None, Some(next)) => unsafe { + (*next.as_ptr()).prev = None; + self.head_tail = Some((next, tail)); + }, + (None, None) => { + // There is only a single node in the list, so from now on it + // will be empty. + self.head_tail = None; + } + } } }