Actix-web: InternalError can trigger memory unsafety (such as use-after-free or double-free)

[seanmonstar]

There is an unsafe impl Send<T> for InternalError<T>, making it so one can construct an InternalError with any type, and accidentally send it to another thread. However, the implementation of InternalError isn't actually safe to implement Send for any T.

Consider this example:

let my_err = Rc::new("foobar".to_owned());
let int_err = InternalError::new(my_err.clone(), StatusCode::BAD_REQUEST);
thread::spawn(move || {
    let _moved = int_err;
    // clone 1 dropped
})
let my_err2 = my_err.clone();
// clone 2 dropped
// orig dropped

The drop of the InternalError in the other thread can cause the ref count to race with the clone in the first thread. It's now possible that the 2nd drop (out of 3) will free the allocation. Using the 3rd (the original) afterwards would cause use-after-free bugs. Finally dropping it would cause a double-free.

Seems related to #289

[fafhrd91]

This is know problem. Nowadays everyone seems love to put Send in every hole

[fafhrd91]

This is related to failure, it uses Send everywhere

[seanmonstar]

Oh did I open a duplicate issue? Searching didn't bring up any results :(

[seanmonstar]

FWIW, the problem doesn't exist because of failure. It's the unsafe impl of Send and Sync. If those impls were removed, then just the default ones would exist, which are safe.

[fafhrd91]

This is absolutely conscious decision.

[fafhrd91]

@seanmonstar do you think I don’t understand impl Send? :)

[seanmonstar]

I'm just reporting a memory unsafety bug I noticed that can be triggered in safe Rust (safe from a user's perspective). I didn't see an issue mentioning it, so I thought I'd bring it up in case it was missed!

[fafhrd91]

Thanks. I will check again if I can implement it without unsafty. I am not sure it can be fixed though

[fafhrd91]

it fixed partially. it is still possible to trigger memory unsafety though.

[seanmonstar]

Awesome!

Examining the fix, it seems it declares InnerHttpResponse as Send and Sync, which contains a Body, which can contain a user's Box<ActorHttpContext>. The ActorHttpContext trait doesn't require being Send or Sync, so it's possible to still trigger memory safety errors as a user:

struct MyInnocentActor(Rc<String>);

impl ActorHttpContext for MyInnocentActor {
    // ...
}

let my_str = Rc::new("foobar".to_owned());
let res = HttpResponse::with_body(StatusCode::BAD_REQUEST, Box::new(MyInnocentActor(my_str.clone())));
let err = InternalError::from_response(some_err, res);
thread::spawn(move || {
    let _moved = err;
    // race in drop of `MyInnocentActor` can mean double-free
})
let _clone = my_str.clone();
// drop 2 and 3

Would you rather track that in a separate issue?

[fafhrd91]

I’ll add fix a bit later. No need for ticket

[memoryruins]

If not another ticket, shouldn't this issue be re-opened for now? It would be good to have an open record, especially if anyone else comes across it and has an idea for the fix.

[fafhrd91]

I don't need new ticket because it is fixed. but this is annoying and only because of Fail: Send + Sync

[anderejd]

There seems to be more of unsafe impl for Send and Sync in this library:

https://github.com/actix/actix-web/search?q=%22unsafe+impl%22&unscoped_q=%22unsafe+impl%22

Is this library memory safe? If not, then I think it should be documented at the top of the Readme.

Should I open a new issue instead?

By the way, thank you for writing this library :)

[axon-q]

@anderejd Please see #289