Potential use after free using ActorRequest

[wiktorwieclaw]

Here you are illegally transmuting a DynamicSender<'a, ...> to DynamicSender<'static, ...>.

ector/ector/src/actor.rs

Lines 77 to 89 in 0505b2f

	impl<'a, M, R> ActorRequest<M, R> for DynamicSender<'a, Request<M, R>> {
	async fn request(&self, message: M) -> R {
	let channel: Channel<NoopRawMutex, R, 1> = Channel::new();
	let sender: DynamicSender<'_, R> = channel.sender().into();
	// We guarantee that channel lives until we've been notified on it, at which
	// point its out of reach for the replier.
	let reply_to = unsafe { core::mem::transmute(&sender) };
	let message = Request::new(message, reply_to);
	self.notify(message).await;
	channel.recv().await
	}
	}

You are not actually guaranteeing that the channel will outlive the sender. It's possible to call the request function with embassy_time::with_timeout or embassy_futures::select, which in turn could cancel the recv's future and drop the channel before the sender is used, resulting in a dangling reference.

To illustrate, I edited the examples/request.rs

async fn test(addr: DynamicAddress<Request<&'static str, &'static str>>) {
    let _ = embassy_time::with_timeout(Duration::from_micros(1), addr.request("Hello")).await;
}

full source code and miri output here

ActorRequest::request is a safe API and it should be impossible to cause UB with it, either mark it as unsafe, or rework it. I'm not actually sure you can do this safely without declaring the channel as static or without dynamic allocation.

[lulf]

I agree this should be better documented, thanks for reporting the issue. Marking as unsafe sounds a bit too much, but maybe using #[must_use] to ensure the future is awaited.

[wiktorwieclaw]

Marking as unsafe sounds a bit too much, but maybe using #[must_use] to ensure the future is awaited.

Sorry but I have to strongly disagree. "No matter what, Safe Rust can't cause Undefined Behavior". I'm not an expert, but I think that once you extend the lifetime to static, you have to somehow guarantee that the object will live until the end of the program and I don't think it can be done in this particular case (without alloc).

I've been trying to implement my own no-alloc actors framework and I really wanted to have a notion of oneshot channels that'd allow me to respond to messages, but I came to the conclusion that it just cannot be done in safe Rust without allocation and I decided to drop this idea.

I hope you can prove me wrong ;P

[lulf]

I'm certainly not an expert either, but I've seen exceptions to this rule in various embedded framework, for instance around DMA, because there might be some edge cases where you could cause undefined behavior, but you'd really have to go out of your way to achieve that.

In this case, if you could somewhat enforce that the request() future will await'ed, even if the lifetime of the channel is extended to static, you know that nothing will be referencing it because it cannot be leaked by the receiver (it's private inside the request). So once the notification arrives, you know that the other side will no longer reference the channel (because it's within the actor framework).

But last time i looked at must_use, it didn't properly enforce awaiting the future (rust-lang/rust#78149). I thought there was some changes in this recently but haven't found it.

[xgroleau]

The issue in the end is async cancellation and there are no real way to prevent it since it can be cancelled from "up", thus even if we require this particular future to be awaited, the function that awaits the request can still be cancelled from where it's called.

Ex

async cancelled(addr: DynamicAddress<Request<&'static str, &'static str>>) -> &'static str {
  // Not cancelled here and awaited as requested by the #[must_use]
  addr.request("Hello").await
}

async fn test(addr: DynamicAddress<Request<&'static str, &'static str>>) {
    // Cancelled here
    let _ = embassy_time::with_timeout(Duration::from_micros(1),  cancelled(addr)).await;
}

We could instead let the user provide a static RequestState which would hold the Channel to the function, now you would still received the message from the cancelled request, but that could maybe be managed internally by the RequestState via counting the uncompleted requests. That would hurt the ease of use of the api so I would probably keep the current implementation and document the limitation.

In essence we would have

pub trait ActorRequest<M, R> {
    /// RequestState is created with a 'static channel inside
    async fn request(&self, message: M, state: &RequestState) -> R;
}

Or we could instead of implementing ActorRequest directly on DynamicSender, we could wrap it in a struct that would take the static Channel. Similar to the first solution but we keep the same api, but have a bit more complexity on the creation/init. A macro could probably be provided to ease this

That would look like:

// RequestWrapper is created with a 'static channel inside
impl<'a, M, R> ActorRequest<M, R> for RequestWrapper<'a, Request<M, R>> {
    async fn request(&self, message: M) -> R {
        self.ignore_previous_requests().await;
        let message = Request::new(message, self.reply_addr);
        self.notify(message).await;
        channel.recv().await
    }
}

If we do want to fix this, I personally prefer the second solution since we keep the same API and the trait doesn't change. Also I fell it's a bit strange to provide the state in the function argument of the trait, you can clearly see the implementation detail in the trait signature with the first solution.

[lulf]

@xgroleau Thanks for expanding on this and highlighting the cancellation issue. I prefer the second solution as well for the same reason.