Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Example usage of heapless::spsc triggers UB? #314

Closed
cr1901 opened this issue Aug 28, 2022 · 2 comments · Fixed by #323
Closed

Example usage of heapless::spsc triggers UB? #314

cr1901 opened this issue Aug 28, 2022 · 2 comments · Fixed by #323

Comments

@cr1901
Copy link

cr1901 commented Aug 28, 2022

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

@korken89
Copy link
Contributor

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.

@reitermarkus
Copy link
Member

reitermarkus commented Sep 11, 2022

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()
    })
  });
}

bors bot added a commit that referenced this issue Oct 20, 2022
323: spsc::Queue: make example less unsafe r=japaric a=japaric

closes #314

Co-authored-by: Jorge Aparicio <jorge.aparicio@ferrous-systems.com>
@bors bors bot closed this as completed in 3e4a253 Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants