From 1cda61fbda205920517f7b63af90c97c38ff9af6 Mon Sep 17 00:00:00 2001 From: Emil Fresk Date: Sat, 18 Feb 2023 09:43:06 +0100 Subject: [PATCH] Make some linked list operations unsafe, and document their safety at usage --- rtic-arbiter/src/lib.rs | 14 +++++++++----- rtic-channel/src/lib.rs | 15 +++++++++------ rtic-common/src/lib.rs | 4 ++++ rtic-common/src/wait_queue.rs | 36 ++++++++++++++++++----------------- rtic-time/src/lib.rs | 19 ++++++++++++------ rtic-time/src/linked_list.rs | 18 ++++++++++++------ 6 files changed, 66 insertions(+), 40 deletions(-) diff --git a/rtic-arbiter/src/lib.rs b/rtic-arbiter/src/lib.rs index 09d1b2eea3..c70fbf5715 100644 --- a/rtic-arbiter/src/lib.rs +++ b/rtic-arbiter/src/lib.rs @@ -54,7 +54,8 @@ impl Arbiter { pub async fn access(&self) -> ExclusiveAccess<'_, T> { let mut link_ptr: Option> = None; - // Make this future `Drop`-safe, also shadow the original definition so we can't abuse it. + // Make this future `Drop`-safe. + // SAFETY(link_ptr): Shadow the original definition of `link_ptr` so we can't abuse it. let mut link_ptr = LinkPtr(&mut link_ptr as *mut Option>); let mut link_ptr2 = link_ptr.clone(); @@ -89,10 +90,13 @@ impl Arbiter { // Place the link in the wait queue on first run. let link_ref = link.insert(Link::new(cx.waker().clone())); - // SAFETY: The address to the link is stable as it is hidden behind - // `link_ptr`, and `link_ptr` shadows the original making it unmovable. - self.wait_queue - .push(unsafe { Pin::new_unchecked(link_ref) }); + // SAFETY(new_unchecked): The address to the link is stable as it is defined + // outside this stack frame. + // SAFETY(push): `link_ref` lifetime comes from `link_ptr` that is shadowed, + // and we make sure in `dropper` that the link is removed from the queue + // before dropping `link_ptr` AND `dropper` makes sure that the shadowed + // `link_ptr` lives until the end of the stack frame. + unsafe { self.wait_queue.push(Pin::new_unchecked(link_ref)) }; } Poll::Pending diff --git a/rtic-channel/src/lib.rs b/rtic-channel/src/lib.rs index 47e4a77d02..a4e49358ac 100644 --- a/rtic-channel/src/lib.rs +++ b/rtic-channel/src/lib.rs @@ -240,7 +240,8 @@ impl<'a, T, const N: usize> Sender<'a, T, N> { pub async fn send(&mut self, val: T) -> Result<(), NoReceiver> { let mut link_ptr: Option> = None; - // Make this future `Drop`-safe, also shadow the original definition so we can't abuse it. + // Make this future `Drop`-safe. + // SAFETY(link_ptr): Shadow the original definition of `link_ptr` so we can't abuse it. let mut link_ptr = LinkPtr(&mut link_ptr as *mut Option>); let mut link_ptr2 = link_ptr.clone(); @@ -276,11 +277,13 @@ impl<'a, T, const N: usize> Sender<'a, T, N> { // Place the link in the wait queue on first run. let link_ref = link.insert(Link::new(cx.waker().clone())); - // SAFETY: The address to the link is stable as it is hidden behind - // `link_ptr`, and `link_ptr` shadows the original making it unmovable. - self.0 - .wait_queue - .push(unsafe { Pin::new_unchecked(link_ref) }); + // SAFETY(new_unchecked): The address to the link is stable as it is defined + // outside this stack frame. + // SAFETY(push): `link_ref` lifetime comes from `link_ptr` that is shadowed, + // and we make sure in `dropper` that the link is removed from the queue + // before dropping `link_ptr` AND `dropper` makes sure that the shadowed + // `link_ptr` lives until the end of the stack frame. + unsafe { self.0.wait_queue.push(Pin::new_unchecked(link_ref)) }; return None; } diff --git a/rtic-common/src/lib.rs b/rtic-common/src/lib.rs index b8b5e0d931..03d0306373 100644 --- a/rtic-common/src/lib.rs +++ b/rtic-common/src/lib.rs @@ -4,6 +4,10 @@ #![deny(missing_docs)] //deny_warnings_placeholder_for_ci +#[cfg(test)] +#[macro_use] +extern crate std; + pub mod dropper; pub mod wait_queue; pub mod waker_registration; diff --git a/rtic-common/src/wait_queue.rs b/rtic-common/src/wait_queue.rs index 4ced6ab9ae..7387a984b7 100644 --- a/rtic-common/src/wait_queue.rs +++ b/rtic-common/src/wait_queue.rs @@ -68,7 +68,9 @@ impl LinkedList { } /// Put an element at the back of the queue. - pub fn push(&self, link: Pin<&mut Link>) { + /// + /// SAFETY: The link must live until it is removed from the queue. + pub unsafe fn push(&self, link: Pin<&Link>) { cs::with(|_| { // Make sure all previous writes are visible core::sync::atomic::fence(Ordering::SeqCst); @@ -76,17 +78,17 @@ impl LinkedList { let tail = self.tail.load(Self::R); // SAFETY: This datastructure does not move the underlying value. - let link = unsafe { link.get_unchecked_mut() }; + let link = link.get_ref(); if let Some(tail_ref) = unsafe { tail.as_ref() } { // Queue is not empty link.prev.store(tail, Self::R); - self.tail.store(link, Self::R); - tail_ref.next.store(link, Self::R); + self.tail.store(link as *const _ as *mut _, Self::R); + tail_ref.next.store(link as *const _ as *mut _, Self::R); } else { // Queue is empty - self.tail.store(link, Self::R); - self.head.store(link, Self::R); + self.tail.store(link as *const _ as *mut _, Self::R); + self.head.store(link as *const _ as *mut _, Self::R); } }); } @@ -126,7 +128,7 @@ impl Link { } /// Remove this link from a linked list. - pub fn remove_from_list(&mut self, list: &LinkedList) { + pub fn remove_from_list(&self, list: &LinkedList) { cs::with(|_| { // Make sure all previous writes are visible core::sync::atomic::fence(Ordering::SeqCst); @@ -230,17 +232,17 @@ mod tests { fn linked_list() { let wq = LinkedList::::new(); - let mut i1 = Link::new(10); - let mut i2 = Link::new(11); - let mut i3 = Link::new(12); - let mut i4 = Link::new(13); - let mut i5 = Link::new(14); + let i1 = Link::new(10); + let i2 = Link::new(11); + let i3 = Link::new(12); + let i4 = Link::new(13); + let i5 = Link::new(14); - wq.push(unsafe { Pin::new_unchecked(&mut i1) }); - wq.push(unsafe { Pin::new_unchecked(&mut i2) }); - wq.push(unsafe { Pin::new_unchecked(&mut i3) }); - wq.push(unsafe { Pin::new_unchecked(&mut i4) }); - wq.push(unsafe { Pin::new_unchecked(&mut i5) }); + unsafe { wq.push(Pin::new_unchecked(&i1)) }; + unsafe { wq.push(Pin::new_unchecked(&i2)) }; + unsafe { wq.push(Pin::new_unchecked(&i3)) }; + unsafe { wq.push(Pin::new_unchecked(&i4)) }; + unsafe { wq.push(Pin::new_unchecked(&i5)) }; wq.print(); diff --git a/rtic-time/src/lib.rs b/rtic-time/src/lib.rs index 5e4457cc0e..3126e6b77d 100644 --- a/rtic-time/src/lib.rs +++ b/rtic-time/src/lib.rs @@ -208,7 +208,8 @@ impl TimerQueue { let mut link_ptr: Option>> = None; - // Make this future `Drop`-safe, also shadow the original definition so we can't abuse it. + // Make this future `Drop`-safe + // SAFETY(link_ptr): Shadow the original definition of `link_ptr` so we can't abuse it. let mut link_ptr = LinkPtr(&mut link_ptr as *mut Option>>); let mut link_ptr2 = link_ptr.clone(); @@ -226,18 +227,24 @@ impl TimerQueue { } // SAFETY: This pointer is only dereferenced here and on drop of the future - // which happens outside this `poll_fn`'s stack frame. + // which happens outside this `poll_fn`'s stack frame, so this mutable access cannot + // happen at the same time as `dropper` runs. let link = unsafe { link_ptr2.get() }; if link.is_none() { - let mut link_ref = link.insert(Link::new(WaitingWaker { + let link_ref = link.insert(Link::new(WaitingWaker { waker: cx.waker().clone(), release_at: instant, was_poped: AtomicBool::new(false), })); - // SAFETY: The address to the link is stable as it is defined outside this stack - // frame. - let (was_empty, addr) = queue.insert(unsafe { Pin::new_unchecked(&mut link_ref) }); + // SAFETY(new_unchecked): The address to the link is stable as it is defined + //outside this stack frame. + // SAFETY(insert): `link_ref` lifetime comes from `link_ptr` that is shadowed, and + // we make sure in `dropper` that the link is removed from the queue before + // dropping `link_ptr` AND `dropper` makes sure that the shadowed `link_ptr` lives + // until the end of the stack frame. + let (was_empty, addr) = unsafe { queue.insert(Pin::new_unchecked(&link_ref)) }; + marker.store(addr, Ordering::Relaxed); if was_empty { diff --git a/rtic-time/src/linked_list.rs b/rtic-time/src/linked_list.rs index de5ea2a48a..d4256c9d73 100644 --- a/rtic-time/src/linked_list.rs +++ b/rtic-time/src/linked_list.rs @@ -93,10 +93,12 @@ impl LinkedList { /// Insert a new link into the linked list. /// The return is (was_empty, address), where the address of the link is for use with `delete`. - pub fn insert(&self, val: Pin<&mut Link>) -> (bool, usize) { + /// + /// SAFETY: The pinned link must live until it is removed from this list. + pub unsafe fn insert(&self, val: Pin<&Link>) -> (bool, usize) { cs::with(|_| { // SAFETY: This datastructure does not move the underlying value. - let val = unsafe { val.get_unchecked_mut() }; + let val = val.get_ref(); let addr = val as *const _ as usize; // Make sure all previous writes are visible @@ -111,7 +113,8 @@ impl LinkedList { let head_ref = if let Some(head_ref) = unsafe { head.as_ref() } { head_ref } else { - self.head.store(val, Ordering::Relaxed); + self.head + .store(val as *const _ as *mut _, Ordering::Relaxed); return (true, addr); }; @@ -121,7 +124,8 @@ impl LinkedList { val.next.store(head, Ordering::Relaxed); // `val` is now first in the queue - self.head.store(val, Ordering::Relaxed); + self.head + .store(val as *const _ as *mut _, Ordering::Relaxed); return (false, addr); } @@ -139,7 +143,8 @@ impl LinkedList { val.next.store(next, Ordering::Relaxed); // Insert `val` - curr.next.store(val, Ordering::Relaxed); + curr.next + .store(val as *const _ as *mut _, Ordering::Relaxed); return (false, addr); } @@ -150,7 +155,8 @@ impl LinkedList { } // No next, write link to last position in list - curr.next.store(val, Ordering::Relaxed); + curr.next + .store(val as *const _ as *mut _, Ordering::Relaxed); (false, addr) })