Use Pin<Box<S>> in BodyStream and SizedStream

[Aaron1011]

Fixes #1321

A better fix would be to change MessageBody to take a Pin<&mut Self>, rather than a &mut self. This will avoid requiring the
use of Box for all consumers by allowing the caller to determine how
to pin the MessageBody implementation (e.g. via stack pinning).

However, doing so is a breaking change that will affect every user of
MessageBody. By pinning the inner stream ourselves, we can fix the
undefined behavior without breaking the API.

I've included @sebzim4500's reproduction case as a new test case.
However, due to the nature of undefined behavior, this could pass (and
not segfault) even if underlying issue were to regress.

Unfortunately, until rust-lang/unsafe-code-guidelines#148 is resolved,
it's not even possible to write a Miri test that will pass when the bug
is fixed.

[robjtede]

I'm torn on this but having some breaking change PRs merged in recently is making me sway more towards solving this "properly" and not boxing these streams.

[cetra3]

Can you please elaborate on this (I am assuming you didn't meant to say Pin<&mut Self> twice):

A better fix would be to change MessageBody to take a Pin<&mut Self>, rather than a Pin<&mut Self>.

If the Proper solution doesn't involve introducing more unsafe code, then I think that is worthwhile, considering we have a few breaking changes in master.

[Aaron1011]

Can you please elaborate on this (I am assuming you didn't meant to say Pin<&mut Self> twice):

Oops, my bad. That should be "A better fix would be to change MessageBody to take a Pin<&mut Self>, rather than a &mut self". I've updated the PR description.

Neither apporach requires any unsafe code. Taking a Pin<&mut Self> gives the caller control over determining how to construct the Pin<&mut Self> (they could use stack pinning, a Pin<Box<T>>, or something else.

If you're okay with breaking a breaking change (which I agree is the correct solution, at least in the long term), I'm happy to make a different PR. However, be aware that this comment states that this will be a significant breaking change.

[cetra3]

Oh, you're talking about this line, I am assuming:

actix-web/actix-http/src/body.rs

Line 39 in dede6fe

fn poll_next(&mut self, cx: &mut Context<'_>) -> Poll<Option<Result<Bytes, Error>>>;

I think we should definitely change that to Pin<&mut Self>, and it is a bit of a bummer that it slipped through.

I'm going to guess there are going to be other places this change will need to be made, so it might make sense to group them into the one PR (on the assumption that @actix/contributors agree to go down this path).

[dunnock]

Ops, it seems we started working on this in parallel. I actually started preparing separate PR with breaking change #1332 .. It affects a lot of places

[JohnTitor]

I'm fine to merge the change as it is since we can backport this PR to 2.0 branch. Then we should introduce better fixes and release them as alpha version.

[cetra3]

@JohnTitor Yep, I agree. Should we raise an issue for the bigger task?

[JohnTitor]

Should we raise an issue for the bigger task?

Yeah, I think so too.

[JohnTitor]

Thanks!

[RalfJung]

@Aaron1011

Unfortunately, until rust-lang/unsafe-code-guidelines#148 is resolved,
it's not even possible to write a Miri test that will pass when the bug
is fixed.

You could run Miri with -Zmiri-disable-stacked-borrows to still test for other kinds of UB.

[Shnatsel]

Following the 3.0 release of actix-web I'm filing security advisories for the issues it resolved. Advisory draft for this issue can be found here: rustsec/advisory-db#402

I'd appreciate if someone could take a look before it goes live. Also, insights into how easy it this is to trigger (i.e. how unusual the API client code has to be) would be appreciated.