Merge pull request #277 from Liamolucko/fix-peripheral-ub

extras: Fix UB in `Peripheral`

1 files changed, 67 insertions, 8 deletions

diff --git a/embassy-extras/src/peripheral_shared.rs b/embassy-extras/src/peripheral_shared.rs
index c62113396..71d746341 100644
--- a/embassy-extras/src/peripheral_shared.rs
+++ b/embassy-extras/src/peripheral_shared.rs
@@ -1,16 +1,24 @@
-use core::cell::UnsafeCell;
 use core::marker::{PhantomData, PhantomPinned};
 use core::pin::Pin;
 
 use embassy::interrupt::{Interrupt, InterruptExt};
 
-pub trait PeripheralState {
+use crate::peripheral::can_be_preempted;
+
+/// A type which can be used as state with `Peripheral`.
+///
+/// It needs to be `Sync` because references are shared between the 'thread' which owns the `Peripheral` and the interrupt.
+///
+/// It also requires `'static` to be used safely with `Peripheral::register_interrupt`,
+/// because although `Pin` guarantees that the memory of the state won't be invalidated,
+/// it doesn't guarantee that the lifetime will last.
+pub trait PeripheralState: Sync {
     type Interrupt: Interrupt;
     fn on_interrupt(&self);
 }
 
 pub struct Peripheral<S: PeripheralState> {
-    state: UnsafeCell<S>,
+    state: S,
 
     irq_setup_done: bool,
     irq: S::Interrupt,
@@ -19,26 +27,58 @@ pub struct Peripheral<S: PeripheralState> {
     _pinned: PhantomPinned,
 }
 
+impl<S: PeripheralState + 'static> Peripheral<S> {
+    /// Registers `on_interrupt` as the wrapped interrupt's interrupt handler and enables it.
+    ///
+    /// This requires this `Peripheral`'s `PeripheralState` to live for `'static`,
+    /// because `Pin` only guarantees that it's memory won't be repurposed,
+    /// not that it's lifetime will last.
+    ///
+    /// To use non-`'static` `PeripheralState`, use the unsafe `register_interrupt_unchecked`.
+    ///
+    /// Note: `'static` doesn't mean it _has_ to live for the entire program, like an `&'static T`;
+    /// it just means it _can_ live for the entire program - for example, `u8` lives for `'static`.
+    pub fn register_interrupt(self: Pin<&mut Self>) {
+        // SAFETY: `S: 'static`, so there's no way it's lifetime can expire.
+        unsafe { self.register_interrupt_unchecked() }
+    }
+}
+
 impl<S: PeripheralState> Peripheral<S> {
     pub fn new(irq: S::Interrupt, state: S) -> Self {
+        if can_be_preempted(&irq) {
+            panic!("`Peripheral` cannot be created in an interrupt with higher priority than the interrupt it wraps");
+        }
+
         Self {
             irq,
             irq_setup_done: false,
 
-            state: UnsafeCell::new(state),
+            state,
             _not_send: PhantomData,
             _pinned: PhantomPinned,
         }
     }
 
-    pub fn register_interrupt(self: Pin<&mut Self>) {
-        let this = unsafe { self.get_unchecked_mut() };
+    /// Registers `on_interrupt` as the wrapped interrupt's interrupt handler and enables it.
+    ///
+    /// # Safety
+    /// The lifetime of any data in `PeripheralState` that is accessed by the interrupt handler
+    /// must not end without `Drop` being called on this `Peripheral`.
+    ///
+    /// This can be accomplished by either not accessing any data with a lifetime in `on_interrupt`,
+    /// or making sure that nothing like `mem::forget` is used on the `Peripheral`.
+    pub unsafe fn register_interrupt_unchecked(self: Pin<&mut Self>) {
+        let this = self.get_unchecked_mut();
         if this.irq_setup_done {
             return;
         }
 
         this.irq.disable();
         this.irq.set_handler(|p| {
+            // The state can't have been dropped, otherwise the interrupt would have been disabled.
+            // We checked in `new` that the thread owning the `Peripheral` can't preempt the interrupt,
+            // so someone can't have preempted us before this point and dropped the `Peripheral`.
             let state = unsafe { &*(p as *const S) };
             state.on_interrupt();
         });
@@ -50,8 +90,27 @@ impl<S: PeripheralState> Peripheral<S> {
     }
 
     pub fn state(self: Pin<&mut Self>) -> &S {
-        let this = unsafe { self.get_unchecked_mut() };
-        unsafe { &*this.state.get() }
+        &self.into_ref().get_ref().state
+    }
+
+    /// Returns whether the wrapped interrupt is currently in a pending state.
+    pub fn is_pending(&self) -> bool {
+        self.irq.is_pending()
+    }
+
+    /// Forces the wrapped interrupt into a pending state.
+    pub fn pend(&self) {
+        self.irq.pend()
+    }
+
+    /// Forces the wrapped interrupt out of a pending state.
+    pub fn unpend(&self) {
+        self.irq.unpend()
+    }
+
+    /// Gets the priority of the wrapped interrupt.
+    pub fn priority(&self) -> <S::Interrupt as Interrupt>::Priority {
+        self.irq.get_priority()
     }
 }