rust-embedded/heapless

Example usage of `heapless::spsc` triggers UB?

cr1901 opened this issue · 2 comments

The split example of heapless::spsc attempts to avoid the need for using a Mutex<RefCell<Queue>> _as well as avoiding static scope for Producer and Consumer by using unsafe code:

fn main() {
    // NOTE(unsafe) beware of aliasing the `consumer` end point
    let mut consumer = unsafe { Q.split().1 };
    ...
fn interrupt_handler() {
    // NOTE(unsafe) beware of aliasing the `producer` end point
    let mut producer = unsafe { Q.split().0 };
    ...

I'm not 100% sure, but I believe this usage is in and of itself UB; the lifetime of consumer and producer is tied to Q, and the mutable borrow of Q lasts as long as consumer or producer lives. If an interrupt happens while consumer exists, Q is mutably borrowed twice when producer is created.

I would appreciate clarification, as I'm not 100% sure if this is UB (and if it is, what an alternate unsafe solution should look like). Here is a playground snippet of me attempting to minimize the spsc code to show UB in Miri: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=597cb05f9f1003515b01e82f79abb9c1

Indeed, this is probably UB.
One should split the queue while having it in static storage and then move the producer/consumer to their usage locations.

I also think it's UB, since this wouldn't be allowed even in a single thread if you paste it in the playground example

let mut q = Queue(0);

let producer = q.split().0;
let consumer = q.split().1;

producer.enqueue();
consumer.dequeue();

what an alternate unsafe solution should look like

I'm not saying it should look like this, this is the best I could come up with so far using an intermediary Mutex<Refcell<Option<_>>> to pass the producer to the handler. Feels very boilerplatey though.

static PRODUCER: Mutex<RefCell<Option<Producer<'static, (), 4>>>> = 
  Mutex::new(RefCell::new(None));

fn main() {
  let mut consumer = {
    let (p, c) = { 
      static mut Q: Queue<(), 4> = Queue::new();
      // SAFETY: Mutable access to `Q` is allowed exclusively in this scope 
      // and `main` is only called once.
      unsafe { Q.split() }
    };

    critical_section::with(move |cs| {
      let mut producer = PRODUCER.borrow_ref_mut(cs);
      *producer = Some(p);
    });

    c
  };
}

fn interrupt() {
  let mut producer = { 
    static mut P: Option<Producer<'static, (), 4>> = None;
    // SAFETY: Mutable access to `P` is allowed exclusively in this scope 
    // and `interrupt` cannot be called directly or preempt itself.
    unsafe { &mut P }
  }.get_or_insert_with(|| {
    critical_section::with(|cs| {
      PRODUCER.borrow_ref_mut(cs).take().unwrap()
    })
  });
}