zonyitoo/context-rs

Memory unsafety with Context

vorner opened this issue · 4 comments

The usage of this crate is not memory safe, it is possible to write segfaulting code without any unsafe code on the caller side.

The problem is, the Context::new takes the stack as a reference. However, it does not in any way enforce the stack outlives the context, making it possible to deallocate the stack and then switch to it.

This code demonstrates the problem:

extern crate context;

use context::{Context, Transfer};
use context::stack::ProtectedFixedSizeStack;

extern "C" fn do_stuff(t: Transfer) -> ! {
    // Make sure a bit of stack is actually used
    println!("Hello world");
    // Return back to the main thread (if we ever get so far as here)
    t.context.resume(0);
    unreachable!();
}

fn new_context() -> Context {
    let stack = ProtectedFixedSizeStack::default();
    Context::new(&stack, do_stuff)
    // The stack is dropped here, the context lives on
}

fn main() {
    new_context().resume(0);
}

I don't have an idea what to do about it (I don't see a clean way to take ownership of the stack object, as the context is consumed on every resume and a new one is created on the other side). But maybe at least making Context::new() an unsafe function and documenting the need for the stack to live long enough?

@vorner If I remember correctly I actually tried to add lifetimes to the Context to track the referenced Stack, but quickly noticed that it's inherently impossible to do so with context switching.
I believe we diceded against making all those methods unsafe because it would have been cumbersome to use in coio-rs. But maybe this might have been the wrong choice in the end, considering Rust's spirit of safety.

Nowadays I'd actually be in favor of making all methods unsafe, but if we do that we have to make it a new major release. Maybe @zonyitoo can lean in on that?

Sure!

@lhecker Please review this commit, :)

Already published as v2.0.0 to crates.io. Closing.