From b27feb061936d191f456edc22b2f89d4fc172520 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Mon, 25 Apr 2022 22:09:04 +0200 Subject: [PATCH] executor: fix unsoundness in InterruptExecutor::start. The initial closure is not actually called in the interrupt, so this is illegally sending non-Send futures to the interrupt. Remove the closure, and return a SendSpawner instead. --- embassy/src/executor/arch/cortex_m.rs | 20 ++++++++++---------- examples/nrf/src/bin/multiprio.rs | 10 ++++------ examples/stm32f3/src/bin/multiprio.rs | 10 ++++------ examples/stm32f4/src/bin/multiprio.rs | 10 ++++------ 4 files changed, 22 insertions(+), 28 deletions(-) diff --git a/embassy/src/executor/arch/cortex_m.rs b/embassy/src/executor/arch/cortex_m.rs index d23a595ff..87c9c3c87 100644 --- a/embassy/src/executor/arch/cortex_m.rs +++ b/embassy/src/executor/arch/cortex_m.rs @@ -1,7 +1,7 @@ use core::marker::PhantomData; use core::ptr; -use super::{raw, Spawner}; +use super::{raw, SendSpawner, Spawner}; use crate::interrupt::{Interrupt, InterruptExt}; /// Thread mode executor, using WFE/SEV. @@ -107,13 +107,13 @@ impl InterruptExecutor { /// Start the executor. /// - /// The `init` closure is called from interrupt mode, with a [`Spawner`] that spawns tasks on - /// this executor. Use it to spawn the initial task(s). After `init` returns, - /// the interrupt is configured so that the executor starts running the tasks. - /// Once the executor is started, `start` returns. + /// This initializes the executor, configures and enables the interrupt, and returns. + /// The executor keeps running in the background through the interrupt. /// - /// To spawn more tasks later, you may keep copies of the [`Spawner`] (it is `Copy`), - /// for example by passing it as an argument to the initial tasks. + /// This returns a [`SendSpawner`] you can use to spawn tasks on it. A [`SendSpawner`] + /// is returned instead of a [`Spawner`] because the executor effectively runs in a + /// different "thread" (the interrupt), so spawning tasks on it is effectively + /// sending them. /// /// This function requires `&'static mut self`. This means you have to store the /// Executor instance in a place where it'll live forever and grants you mutable @@ -122,16 +122,16 @@ impl InterruptExecutor { /// - a [Forever](crate::util::Forever) (safe) /// - a `static mut` (unsafe) /// - a local variable in a function you know never returns (like `fn main() -> !`), upgrading its lifetime with `transmute`. (unsafe) - pub fn start(&'static mut self, init: impl FnOnce(Spawner) + Send) { + pub fn start(&'static mut self) -> SendSpawner { self.irq.disable(); - init(self.inner.spawner()); - self.irq.set_handler(|ctx| unsafe { let executor = &*(ctx as *const raw::Executor); executor.poll(); }); self.irq.set_handler_context(&self.inner as *const _ as _); self.irq.enable(); + + self.inner.spawner().make_send() } } diff --git a/examples/nrf/src/bin/multiprio.rs b/examples/nrf/src/bin/multiprio.rs index e69f87d85..54f6606a9 100644 --- a/examples/nrf/src/bin/multiprio.rs +++ b/examples/nrf/src/bin/multiprio.rs @@ -124,17 +124,15 @@ fn main() -> ! { let irq = interrupt::take!(SWI1_EGU1); irq.set_priority(interrupt::Priority::P6); let executor = EXECUTOR_HIGH.put(InterruptExecutor::new(irq)); - executor.start(|spawner| { - unwrap!(spawner.spawn(run_high())); - }); + let spawner = executor.start(); + unwrap!(spawner.spawn(run_high())); // Medium-priority executor: SWI0_EGU0, priority level 7 let irq = interrupt::take!(SWI0_EGU0); irq.set_priority(interrupt::Priority::P7); let executor = EXECUTOR_MED.put(InterruptExecutor::new(irq)); - executor.start(|spawner| { - unwrap!(spawner.spawn(run_med())); - }); + let spawner = executor.start(); + unwrap!(spawner.spawn(run_med())); // Low priority executor: runs in thread mode, using WFE/SEV let executor = EXECUTOR_LOW.put(Executor::new()); diff --git a/examples/stm32f3/src/bin/multiprio.rs b/examples/stm32f3/src/bin/multiprio.rs index 1c9401549..02380de72 100644 --- a/examples/stm32f3/src/bin/multiprio.rs +++ b/examples/stm32f3/src/bin/multiprio.rs @@ -124,17 +124,15 @@ fn main() -> ! { let irq = interrupt::take!(UART4); irq.set_priority(interrupt::Priority::P6); let executor = EXECUTOR_HIGH.put(InterruptExecutor::new(irq)); - executor.start(|spawner| { - unwrap!(spawner.spawn(run_high())); - }); + let spawner = executor.start(); + unwrap!(spawner.spawn(run_high())); // Medium-priority executor: SWI0_EGU0, priority level 7 let irq = interrupt::take!(UART5); irq.set_priority(interrupt::Priority::P7); let executor = EXECUTOR_MED.put(InterruptExecutor::new(irq)); - executor.start(|spawner| { - unwrap!(spawner.spawn(run_med())); - }); + let spawner = executor.start(); + unwrap!(spawner.spawn(run_med())); // Low priority executor: runs in thread mode, using WFE/SEV let executor = EXECUTOR_LOW.put(Executor::new()); diff --git a/examples/stm32f4/src/bin/multiprio.rs b/examples/stm32f4/src/bin/multiprio.rs index 1c9401549..02380de72 100644 --- a/examples/stm32f4/src/bin/multiprio.rs +++ b/examples/stm32f4/src/bin/multiprio.rs @@ -124,17 +124,15 @@ fn main() -> ! { let irq = interrupt::take!(UART4); irq.set_priority(interrupt::Priority::P6); let executor = EXECUTOR_HIGH.put(InterruptExecutor::new(irq)); - executor.start(|spawner| { - unwrap!(spawner.spawn(run_high())); - }); + let spawner = executor.start(); + unwrap!(spawner.spawn(run_high())); // Medium-priority executor: SWI0_EGU0, priority level 7 let irq = interrupt::take!(UART5); irq.set_priority(interrupt::Priority::P7); let executor = EXECUTOR_MED.put(InterruptExecutor::new(irq)); - executor.start(|spawner| { - unwrap!(spawner.spawn(run_med())); - }); + let spawner = executor.start(); + unwrap!(spawner.spawn(run_med())); // Low priority executor: runs in thread mode, using WFE/SEV let executor = EXECUTOR_LOW.put(Executor::new());