Remove unsafe from proto::h2

[erickt]

Back in #2523, @nox introduced the notion of an UpgradedSendStream, to support the CONNECT method of HTTP/2. This used unsafe {} to support http_body::Body, where Body::Data did not implement Send, since the Data type wouldn't be sent across the stream once upgraded.

Unfortunately, according to this thread, I think this may be undefined behavior, because this relies on us requiring the transmute to execute.

This patch fixes the potential UB by adding the unncessary Send constraints. It appears that all the internal users of
UpgradeSendStream already work with http_body::Body types that have Send-able Data constraints. We can add this constraint without breaking any external APIs, which lets us remove the unsafe {} blocks.

I filed rust-lang/unsafe-code-guidelines#337 for formal guidance on this pattern.

Fixes #2829

[RalfJung]

FWIW, there have even been proposals that would make Neutered<B> have size 0, since it cannot ever be inhabited anyway. So this is definitely an incorrect pattern.

[djkoloski]

That is checked in UpgradedSendStream's new, so I think that should be okay. It's not ideal but it shouldn't produce any UB.

[RalfJung]

Ah, you are guarding against the layout optimization there, okay.

Creating an instance of Neutered<T> is still most definitely UB though. In fact if there is any uncontroversial UB in Rust, unreachable_unchecked would be first on that list, and creating an instance of an uninhabited type is second. The reference is also clear about this and never was there any controversy in the UCG that this code should be UB.

[nox]

There is never a Neutered<T> being created anywhere, that's the thing.

[RalfJung]

Ah, I see, that makes sense then that Miri didn't complain about hyper. :)

[erickt]

This is an alternative version of #2830, where this one adds Send constraint, but #2830 uses unsafe to not add Send.

[nox]

Let's go for #2830 instead, so that we avoid the breaking change.

I'm not sure I understand why you think there is UB though. The transmute doesn't happen on an uninhabited type, it happens on an enum of which one of the variants has an uninhabited field.

[erickt]

@nox - we talked about this more on rust-lang/unsafe-code-guidelines#337. I now think this is sound. The UB I thought we had was that directly transmuting a value to something that contains an Impossible type is UB. But instead you are instead ultimately transmuting a pointer to an Impossible type, which is sound.

The one concern though that’s brought up in the thread is that there’s no guarantee of the layout of a type that includes an impossible type. There’s talk about always treating those types as zero sized.

Let's go for #2830 instead, so that we avoid the breaking change.

Sure, that’s fine with me. My main interest is to at least document it to save other people’s time when auditing hyper.

There’s a chance though this isn’t a breaking change (at least for external users). I traced that the proto::h2::client already requires B::Data: Send, but I haven’t finished going through all the users of proto::h2::server yet. It’s a little challenging with all the generic type parameters.

[RalfJung]

we talked about this more on rust-lang/unsafe-code-guidelines#337. I now think this is sound. The UB I thought we had was that directly transmuting a value to something that contains an Impossible type is UB. But instead you are instead ultimately transmuting a pointer to an Impossible type, which is sound.

As long as it's a raw pointer, that's okay.
For a reference or Box, that's not okay.

[nox]

There is never, ever, a reference to an uninhabited value, or an uninhabited value, being created by my code.

As for Neutered<B> having a different size than B itself, I think that directly contradicts the meaning of #[repr(transparent)].

[nox]

There’s a chance though this isn’t a breaking change (at least for external users). I traced that the proto::h2::client already requires B::Data: Send, but I haven’t finished going through all the users of proto::h2::server yet. It’s a little challenging with all the generic type parameters.

You may be right. H2Stream is marked pub (probably because of the unstable feature), so to me it's a breaking change as it restricts one of the type's impls. (As a side note I kinda dislike that unstable just exposes internal details like that, @seanmonstar do you think we could get rid of it one day?) Maybe we can just add the bound after all.

[RalfJung]

There is never, ever, a reference to an uninhabited value, or an uninhabited value, being created by my code.

All right, sorry for the confusion.

As for Neutered<B> having a different size than B itself, I think that directly contradicts the meaning of #[repr(transparent)].

Yeah, rust-lang/rust#96921 was recently created to discuss this.

[erickt]

You may be right. H2Stream is marked pub (probably because of the unstable feature), so to me it's a breaking change as it restricts one of the type's impls. (As a side note I kinda dislike that unstable just exposes internal details like that, @seanmonstar do you think we could get rid of it one day?) Maybe we can just add the bound after all.

Ah, nuts. So I finished tracing, and H2Stream is one of those quasi-public types:

• hyper::proto::h2::H2Stream is marked pub.
• hyper::proto::h2 is pub(crate) so it's not directly accessible.
• However, H2Stream is used in hyper::common::exec::ConnStreamExec, which is also pub.
• But hyper::common::exec is also pub(crate).
• Finally, ConnStreamExec is used as a type constraint in hyper::server::Server.

[seanmonstar]

Ok, so to recap:

• There was no UB here, since it's repr transparent and only raw pointers?
• This changes a bounds on a public-ish type, which I believe could be a breaking change. To verify, can the single_threaded example in this repo make its Buf type !Send, and then this change breaks that?

[nox]

This changes a bounds on a public-ish type, which I believe could be a breaking change. To verify, can the single_threaded example in this repo make its Buf type !Send, and then this change breaks that?

I think it breaks using custom executors with non-Send buffers. Pretty niche if you ask me.

[erickt]

You are correct, updating single_threaded to change Body to return a !Send impl of Buf does val with `*const () cannot be sent between threads safely", so I'll close this. Thanks all!