UpgradedSendStream transmute seems to be super unsound

[Manishearth]

https://github.com/hyperium/hyper/blob/1d4ff3597b8e76818c8553dbfa4234cf4208c958/src/proto/h2/mod.rs#L389C8-L397

This isn't sound. Neutered<B> is:

#[repr(transparent)]
struct Neutered<B> {
    _inner: B,
    impossible: Impossible,
}

enum Impossible {}

Firstly, it's unclear what guarantees repr(transparent) confers here, impossible does have size_of == 0 but it's not exactly a zero-sized type (rust-lang/unsafe-code-guidelines#485). The repr(transparent) RFC is not clear about this, and it's quite shaky ground to rely on, especially since empty ("impossible") types are one place Rust does sometimes perform layout optimizations. There's an assertion guard against that for Neutered but it's not clear how transitive this stuff is (rust-lang/unsafe-code-guidelines#486)

Empty types can and do affect enum representations. SendBuf<B> has the following definition:

#[repr(usize)]
enum SendBuf<B> {
    Buf(B),
    Cursor(Cursor<Box<[u8]>>),
    None,
}

There's nothing preventing the compiler from realizing that Buf(Neutered<...>) is impossible and deciding to not include it as a discriminant choice. The asserts in this code are insufficient to check that.

This code seems to work fine under the current compiler but I wouldn't rely on that.

[seanmonstar]

For historical reference, this was discussed in #2831 and determined it wasn't unsound. Could be we got it wrong in there, I'm just including it for context.

[Manishearth]

Yeah, that discussion seems to focus on whether Neutered<B> is constructed; not about whether the transmute is safe.

[Manishearth]

Also stepping back a bit this pattern here is extremely weird and has almost no comments explaining what it does or justifying its safety. It's worth considering moving away from this model anyway, if possible.

[seanmonstar]

cc @nox, you'll remember this better than I.

[RalfJung]

There's nothing preventing the compiler from realizing that Buf(Neutered<...>) is impossible and deciding to not include it as a discriminant choice. The asserts in this code are insufficient to check that.

Agreed.

[nox]

There is no such thing as “not include it as a discriminant choice” as that would be an observable behaviour for repr(usize) and break things related to such enums. Servo for example include uninhabited variants in repr(u32) enums specifically to skip a few variant discriminants to make the actually inhabited variants have their discriminants coincide with other enums. See discussions on the explicit discriminants Rust RFC. I am on PTO with a smartphone with a broken screen so you’ll excuse me for the lack of links in this comment hopefully. Ven 22 déc 2023, à 16:09, Ralf Jung a écrit :

…

> There's nothing preventing the compiler from realizing that Buf(Neutered<...>) is impossible and deciding to not include it as a discriminant choice. The asserts in this code are insufficient to check that. > Agreed. — Reply to this email directly, view it on GitHub <#3500 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA6BV6JLBF3EMSPAHMNBULYKWPDNAVCNFSM6AAAAABA6V6R4SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRXG44TKMZZGI>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[RalfJung]

Ah sorry I had missed that this enum is repr(usize). That indeed makes a difference.

[Manishearth]

@RalfJung does that also work with the transitivity assumption?

[RalfJung]

Not sure which assumption exactly you mean?

Basically the question is, for two repr($int) enum, if they have the same variants in the same order and all fields have the same layout, then do the two enums have the same layout? I don't see any way this could not be the case; AFAIK layout is completely laid down for repr(usize). So I think just like repr(transparent) is "transitive" through repr(C), it should also be "transitive" through repr($int). At least I'd say we should make such a guarantee; I don't know if we do.

Whether that's actually implemented properly for uninhabited types (rust-lang/unsafe-code-guidelines#485), I don't know.

[Manishearth]

Okay, good to know, thanks!

[seanmonstar]

So did that extra information mean that this is fine after all? Or do we need to adjust something?

[Manishearth]

I think it's still potentially an issue given:

Whether that's actually implemented properly for uninhabited types (rust-lang/unsafe-code-guidelines#485), I don't know.

but the bigger issue is a non-issue it seems.