Undefined behavior due to unsafe pinning of BodyStream

[sebzim4500]

Expected Behavior

The program below should not segfault when I only write 'safe' code.

Current Behavior

The program below segfaults.

Possible Solution

Change the MessageBody trait to take a Pin<&mut self> instead of a &mut self.

Steps to Reproduce (for bugs)

Run the following code in debug mode.

use futures::task::{noop_waker, Context};
use futures::stream::once;
use actix_http::body::{MessageBody, BodyStream};
use bytes::Bytes;

fn main() {
    let (sender, receiver) = futures::channel::oneshot::channel();
    let mut body_stream = Ok(BodyStream::new(once(async {
        let x = Box::new(0);
        let y = &x;
        receiver.await.unwrap();
        let z = **y;
        Ok::<_, ()>(Bytes::new())
    })));

    let waker = noop_waker();
    let mut context = Context::from_waker(&waker);

    let _ = body_stream.as_mut().unwrap().poll_next(&mut context);
    sender.send(()).unwrap();
    let _ = std::mem::replace(&mut body_stream, Err([0; 32])).unwrap().poll_next(&mut context);
}

Context

I found this problem by searching for 'unsafe' in the actix-web repo.

• Rust Version (I.e, output of rustc -V): rustc 1.41.0-nightly
• Actix Web Version: actix-http v1.0.1

[dunnock]

Confirmed the test is crashing test runner.

Provided possible fix will be massive and break actix-web back compatibility as MessageBody trait is used in actix-http, awc and actix-web crates. Is there any other solution?

[tyranron]

I dunno atm. But it feels that we will have to refactor all the poll-like methods to accept Pin<&mut Self> eventually, one way or another. It's to hard to control and keep in mind all the &mut self<->Pin<&mut Self> bridges with their invariants/tradeoffs. Even more, future contributions will definitely break the invariants, accidentally and quite often. Here the type system should do its job.

[sebzim4500]

I found a similar issue in actix-codec, if you are going to make a breaking change here then you will probably need one there as well.

actix/actix-net#91

[cdbattags]

Would this be a 2.x release though if we decide to do this?

Should we refactor and shoot for a 3.0 transition for this? I think 2+ has been a good halfway point but I really want to make async/await first order if possible soon.

[robjtede]

IMO we should refactor these unsafe's and shoot for a 3.0 release with a reasonably long alpha/beta period.

[cdbattags]

I think that's the best path forward as well now that Future is stable but it'll be hard to "backport" any new work. We most likely wouldn't be able to work on 2+ changes and 3.0 release simultaneously.

[wolverian]

Is this still relevant in 3.0.0-beta.9, or is the advisory DB buggy?

[robjtede]

The advisory is not applicable to 3.0 and up.

There is a bug in cargo audit where it fails to correctly compare pre release versions.

[wolverian]

@robjtede Thank you and sorry for the noise. For others who might be interested, the upstream issue is rustsec/rustsec#300.