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

Add API to use stack-allocated buffers safely. #11

Open
Dirbaio opened this issue Jan 2, 2021 · 4 comments
Open

Add API to use stack-allocated buffers safely. #11

Dirbaio opened this issue Jan 2, 2021 · 4 comments

Comments

@Dirbaio
Copy link
Member

Dirbaio commented Jan 2, 2021

Currently using stack-allocated buffers is impossible without unsafe. Here are two ideas on how to do this, coming from our conversations in Matrix :)

I'm not sure which one is best, we should explore tradeoffs and decide (or have both?)

Method 1: closure-based

pub struct Buffer<'a> {
    inner: &'a mut [u8],
}

unsafe impl<'a> ReadBuffer for Buffer<'a> {
    type Word = u8;
    unsafe fn read_buffer(&self) -> (*const Self::Word, usize) {
        (self.inner.as_ptr(), self.inner.len())
    }
}

unsafe impl<'a> WriteBuffer for Buffer<'a> {
    type Word = u8;
    unsafe fn write_buffer(&mut self) -> (*mut Self::Word, usize) {
        (self.inner.as_mut_ptr(), self.inner.len())
    }
}

fn with_buffer<'a, R>(buf: &'a mut [u8], f: impl FnOnce(Buffer<'a>) -> (R, Buffer<'a>)) -> R {
    let orig_loc = (buf.as_ptr(), buf.len());
    let (r, ret_buf) = f(Buffer { inner: buf });
    let ret_loc = (ret_buf.inner.as_ptr(), ret_buf.inner.len());
    core::assert_eq!(orig_loc, ret_loc); // Not sure if this is needed for safety.
    r
}

Method 2: flag-based

#[feature(min_const_generics)]
use core::marker::PhantomPinned;
use core::panic;
use core::pin::Pin;
//use embedded_dma::{ReadBuffer, WriteBuffer};
use futures::pin_mut;

struct DmaBuffer<B: ?Sized> {
    pinned: PhantomPinned,
    in_use: bool,
    buf: B,
}

impl<B> DmaBuffer<B> {
    pub fn new(b: B) -> Self {
        Self {
            pinned: PhantomPinned,
            in_use: false,
            buf: b,
        }
    }
}

impl<const N: usize> DmaBuffer<[u8; N]> {
    pub fn borrow<'a>(self: Pin<&'a mut Self>) -> DmaBufferBorrow<'a, [u8]> {
        let this = unsafe { self.get_unchecked_mut() };
        if this.in_use {
            panic!("You can't borrow a buffer two times!")
        }
        this.in_use = true;
        DmaBufferBorrow(unsafe { Pin::new_unchecked(this) })
    }
}

impl<B: ?Sized> Drop for DmaBuffer<B> {
    fn drop(&mut self) {
        if self.in_use {
            panic!("You leaked a buffer borrow!")
        }
    }
}

struct DmaBufferBorrow<'a, B: ?Sized>(Pin<&'a mut DmaBuffer<B>>);

impl<'a, B: ?Sized> Drop for DmaBufferBorrow<'a, B> {
    fn drop(&mut self) {
        unsafe { self.0.as_mut().get_unchecked_mut() }.in_use = false;
    }
}

fn use_buffer<'a>(buf: DmaBufferBorrow<'a, [u8]>) {}

unsafe impl<'a> ReadBuffer for DmaBufferBorrow<'a, [u8]> {
    type Word = u8;
    unsafe fn read_buffer(&self) -> (*const Self::Word, usize) {
        (self.0.buf.as_ptr(), self.0.buf.len())
    }
}

unsafe impl<'a> WriteBuffer for DmaBufferBorrow<'a, [u8]> {
    type Word = u8;
    unsafe fn write_buffer(&mut self) -> (*mut Self::Word, usize) {
        let buf = unsafe { self.0.as_mut().get_unchecked_mut() };
        (buf.buf.as_mut_ptr(), buf.buf.len())
    }
}

fn main() {
    let mut buf = DmaBuffer::new([0u8; 1024]);
    pin_mut!(buf);

    println!("in use: {}", buf.in_use);
    let mut borrow = buf.as_mut().borrow();
    //println!("in use: {}", buf.in_use); This would print true
    use_buffer(borrow);
    println!("in use: {}", buf.in_use);
}
@Finomnis
Copy link

Finomnis commented Aug 23, 2023

Be aware that in your second example, nothing prevents the user from dropping buf while it is still in use. The flag does prevent re-use, but if you forget() the borrow object, we can drop the buf object while the DMA is still running.

We could somewhat mitigate that by checking the in_use flag in drop() of buf, but a forced panic in drop() is usually never a good idea.

EDIT: I guess something similar could be done in the first case, inside of the closure.

@Sympatron
Copy link

I don't think that is true. In the first case you can't forget or drop buf because you don't own it and in the second case you can't drop or forget buf because it is borrowed.

@Finomnis
Copy link

Finomnis commented Aug 23, 2023

I think you misunderstand what I'm trying to say; I'm not talking about forgetting buf in the second case. I'm taking about forgetting borrow, releasing it's 'a reference without running its drop function. But maybe I misunderstand your code, of course.

@Sympatron
Copy link

Sympatron commented Aug 23, 2023

You are right, I misunderstood. If you forget(borrow) and then forget(buf) while buf.in_use == true you would have a bad time.
But that is not a problem, because using the borrow (write_buffer(), read_buffer()) is still unsafe. Therefore you can't cause this problem without using unsafe.

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

No branches or pull requests

3 participants