Merge #1177

1177: STD driver needs a reentrant mutex; logic fixed to be reentrancy-safe r=Dirbaio a=ivmarkov

...or to summarize it in another way, the code in the alarm thread loop is written as if - when calling the user-supplied callback - the callback will *never, ever* call `alarm.set_alarm()`.

But this happens of course - at least with the generic timer queue implementation. Not sure if that would happen with `embassy-executor`'s own queue, but probably yes?

The end result on Linux is that the code deadlocks because when calling the user-supplied callback, the mutex of the alarms is locked, yet - the code in `set_alarm` tries to take the lock again leading to UB. (I suspect on Windows this will crash rather than deadlock but that's a bit irrelevant.)

(Note also that calling the user-supplied callback *outside* of the alarms' lock is also NOK, because at that time, the callback and/or context itself might be invalid as well, as the user might had changed it with a new one by calling `set_callback`. Right?)

I also had to fix the logic that computed the next timestamp when the alarm should fire; it was running a simple `for {}` loop, not anticipating that the just-traversed alarm might get a new timestamp.

The new code is slightly less efficient, in that on each `loop {}` iteration it always starts traversing the alarms from the beginning, whereas in reality only the timestamp of the alarm that just-fired could've changed, but given the complexities introduced by `RefCell`, I don't think we should bother with these micro-optimizations, for just 4 alarms in total.


Co-authored-by: ivmarkov <ivan.markov@gmail.com>

embassy-time/src/driver_std.rs

@@ -1,10 +1,12 @@
-use std::cell::UnsafeCell;
+use std::cell::{RefCell, UnsafeCell};
 use std::mem::MaybeUninit;
 use std::sync::{Condvar, Mutex, Once};
 use std::time::{Duration as StdDuration, Instant as StdInstant};
 use std::{mem, ptr, thread};
 
 use atomic_polyfill::{AtomicU8, Ordering};
+use embassy_sync::blocking_mutex::raw::CriticalSectionRawMutex;
+use embassy_sync::blocking_mutex::Mutex as EmbassyMutex;
 
 use crate::driver::{AlarmHandle, Driver};
 
@@ -35,7 +37,10 @@ struct TimeDriver {
     alarm_count: AtomicU8,
 
     once: Once,
-    alarms: UninitCell<Mutex<[AlarmState; ALARM_COUNT]>>,
+    // The STD Driver implementation requires the alarms' mutex to be reentrant, which the STD Mutex isn't
+    // Fortunately, mutexes based on the `critical-section` crate are reentrant, because the critical sections
+    // themselves are reentrant
+    alarms: UninitCell<EmbassyMutex<CriticalSectionRawMutex, RefCell<[AlarmState; ALARM_COUNT]>>>,
     zero_instant: UninitCell<StdInstant>,
     signaler: UninitCell<Signaler>,
 }
@@ -53,7 +58,8 @@ crate::time_driver_impl!(static DRIVER: TimeDriver = TimeDriver {
 impl TimeDriver {
     fn init(&self) {
         self.once.call_once(|| unsafe {
-            self.alarms.write(Mutex::new([ALARM_NEW; ALARM_COUNT]));
+            self.alarms
+                .write(EmbassyMutex::new(RefCell::new([ALARM_NEW; ALARM_COUNT])));
             self.zero_instant.write(StdInstant::now());
             self.signaler.write(Signaler::new());
 
@@ -66,26 +72,38 @@ impl TimeDriver {
         loop {
             let now = DRIVER.now();
 
-            let mut next_alarm = u64::MAX;
+            let next_alarm = unsafe { DRIVER.alarms.as_ref() }.lock(|alarms| {
-            {
+                loop {
-                let alarms = &mut *unsafe { DRIVER.alarms.as_ref() }.lock().unwrap();
+                    let pending = alarms
-                for alarm in alarms {
+                        .borrow_mut()
-                    if alarm.timestamp <= now {
+                        .iter_mut()
+                        .find(|alarm| alarm.timestamp <= now)
+                        .map(|alarm| {
                             alarm.timestamp = u64::MAX;
 
-                        // Call after clearing alarm, so the callback can set another alarm.
+                            (alarm.callback, alarm.ctx)
+                        });
 
+                    if let Some((callback, ctx)) = pending {
                         // safety:
                         // - we can ignore the possiblity of `f` being unset (null) because of the safety contract of `allocate_alarm`.
                         // - other than that we only store valid function pointers into alarm.callback
-                        let f: fn(*mut ()) = unsafe { mem::transmute(alarm.callback) };
+                        let f: fn(*mut ()) = unsafe { mem::transmute(callback) };
-                        f(alarm.ctx);
+                        f(ctx);
                     } else {
-                        next_alarm = next_alarm.min(alarm.timestamp);
+                        // No alarm due
-                    }
+                        break;
                     }
                 }
 
+                alarms
+                    .borrow()
+                    .iter()
+                    .map(|alarm| alarm.timestamp)
+                    .min()
+                    .unwrap_or(u64::MAX)
+            });
+
             // Ensure we don't overflow
             let until = zero
                 .checked_add(StdDuration::from_micros(next_alarm))
@@ -121,18 +139,23 @@ impl Driver for TimeDriver {
 
     fn set_alarm_callback(&self, alarm: AlarmHandle, callback: fn(*mut ()), ctx: *mut ()) {
         self.init();
-        let mut alarms = unsafe { self.alarms.as_ref() }.lock().unwrap();
+        unsafe { self.alarms.as_ref() }.lock(|alarms| {
+            let mut alarms = alarms.borrow_mut();
             let alarm = &mut alarms[alarm.id() as usize];
             alarm.callback = callback as *const ();
             alarm.ctx = ctx;
+        });
     }
 
     fn set_alarm(&self, alarm: AlarmHandle, timestamp: u64) -> bool {
         self.init();
-        let mut alarms = unsafe { self.alarms.as_ref() }.lock().unwrap();
+        unsafe { self.alarms.as_ref() }.lock(|alarms| {
+            let mut alarms = alarms.borrow_mut();
+
             let alarm = &mut alarms[alarm.id() as usize];
             alarm.timestamp = timestamp;
             unsafe { self.signaler.as_ref() }.signal();
+        });
 
         true
     }