embassy-net: {Tcp,Udp}Socket::new are unsound

[fghzxm]

TcpSocket::new is defined as

pub fn new<'a, D: Driver>(
    stack: &'a Stack<D>,
    rx_buffer: &'a mut [u8],
    tx_buffer: &'a mut [u8]
) -> TcpSocket<'a> {
    let s = &mut *stack.socket.borrow_mut();
    let rx_buffer: &'static mut [u8] = unsafe { mem::transmute(rx_buffer) }; // ???
    let tx_buffer: &'static mut [u8] = unsafe { mem::transmute(tx_buffer) };
    let handle = s.sockets.add(tcp::Socket::new(
        tcp::SocketBuffer::new(rx_buffer),
        tcp::SocketBuffer::new(tx_buffer),
    ));
}

Although this particular stack is 'a, SocketStack (which is the type of stack.socket modulo RefCell) itself is 'static:

pub(crate) struct SocketStack {
    pub(crate) sockets: SocketSet<'static>, // !!!
    pub(crate) iface: Interface,
    pub(crate) waker: WakerRegistration,
    next_local_port: u16,
}

TcpSocket::new indirectly stores rx_buffer and tx_buffer, which are references whose pointed-to data only lives for 'a, into a SocketStack, which must only contain 'static references. This is unsound:

#[embassy_executor::task]
async fn forgetful_task(stack: &'static Stack<SomeDriver>) {
    let mut rx_buffer = [0u8; 4096];
    let mut tx_buffer = [0u8; 4096];
    let mut sock = TcpSocket::new(stack, &mut rx_buffer[..], &mut tx_buffer[..]);
    unwrap!(sock.accept(somewhere).await);
    // Network stack starts to write to `rx_buffer`
    mem::forget(sock);
    // `rx_buffer` goes out of scope
}

#[embassy_executor::main]
async fn main(spawner: Spawner) {
    let stack_resources = make_static!(StackResources::<10>::new());
    let stack: &_ = make_static!(Stack::new(some_driver, some_config, stack_resources, some_seed));
    unwrap!(spawner.spawn(run_net_stack(stack)));
    unwrap!(spawner.spawn(forgetful_task(stack)));
}

#[embassy_executor::task]
async fn run_net_stack(stack: &'static Stack<SomeDriver>) -> ! {
    stack.run().await
}

The same issue exists for UdpSocket::new.

[Dirbaio]

just "lying" about lifetimes is not UB in and of itself. You need to actually use the buffer beyond its lifetime to get UB. Drop for the sockets removes them from the SocketSet, preventing this.

[fghzxm]

@Dirbaio First of all, thank you for taking time to respond to this issue. :)

It's well known that relying on Drop::drop to run for safety is unsound in Rust, and that core::mem::forget is safe and can be implemented in safe Rust using Rc and RefCell.

I'm fully aware that the vast majority of code using embassy-net will not accidentally leak sockets, which is necessary for use-after-free to happen this way. However, this is not how unsoundness is handled in the Rust ecosystem; Rust programmers consider it necessary for all possible uses of a safe API to never trigger UB, not just "most" uses, or "all reasonable" uses by any non-trivial definition of "reasonable".

This is the reason why standard library containers like BTreeMap and HashMap have to defend against incorrect implementations of Eq, Hash and Ord, why RAII guards like std::collections::binary_heap::PeekMut preventatively sets the heap's length to 1, and why std::thread::scoped was scrapped for the non-zero-overhead std::thread::scope.

---

There are at least three ways to make TcpSocket::new completely sound:

1. Change TcpSocket::new to only accept 'static buffers;
2. Change TcpSocket::new into a closure-passing style API similar to std::thread::scope (alternatively, this can be added as a separate method that coexists with 1.);
3. Make SocketStack and Stack generic over a lifetime, and pass that lifetime to smoltcp::iface::SocketSet (we're currently using SocketSet<'static>, as mentioned in the OP).

They're all breaking changes that entail different trade-offs, which is why I didn't just submit a pull request but opened this issue first.

embassy-net is currently at major version 0, where breaking changes can be freely made between minor versions and patches; I believe it's better to change the API during version 0 than to let it go into version 1 as-is, where embassy will have much more dependents and will not be able to make breaking changes until major version 2.

[reitermarkus]

Hi, I just read the implementation of TcpSocket::new and noticed the same thing. Soundness should not depend on Drop.