Replace transmute with bounded manual Send impl
#2830
Conversation
| enum Impossible {} | ||
|
|
||
| unsafe impl<B> Send for Neutered<B> {} | ||
| const _: () = { |
There was a problem hiding this comment.
Why use an unnamed const rather than a private module?
There was a problem hiding this comment.
Private modules need names, const items don't. Less names is better, IMO.
| @@ -409,15 +408,14 @@ fn h2_to_io_error(e: h2::Error) -> io::Error { | |||
| } | |||
| } | |||
|
|
|||
| struct UpgradedSendStream<B>(SendStream<SendBuf<Neutered<B>>>); | |||
| struct UpgradedSendStream<B>(SendStream<SendBuf<B>>); | |||
There was a problem hiding this comment.
I don't like this. The whole point of Neutered<B> is that you can't create one, whereas here someone can use the field directly and send a B over the channel.
There was a problem hiding this comment.
My impression was that new/as_inner_unchecked were meant to act as guards against sending a B. For example, the safety conditions for new would be "This must be the only instance of the stream" and as_inner_unchecked would be "No messages must be sent through the returned stream".
It's a little unclear to me what the desired API is since this is all private. If the goal is to make the stream impossible to use incorrectly, even in private contexts, then as_inner_unchecked shouldn't exist. Even then, someone with private access could access the inner field and do the transmute by hand, so I don't think it's feasible to prevent misuse in private contexts.
There was a problem hiding this comment.
My impression was that
new/as_inner_uncheckedwere meant to act as guards against sending aB. For example, the safety conditions fornewwould be "This must be the only instance of the stream" andas_inner_uncheckedwould be "No messages must be sent through the returned stream".
Yes they are, but as it doesn't live in its own module, the rest of the code in that module is free to do .0 and access the SendBuf<B> without the unsafety of as_inner_unchecked.
There was a problem hiding this comment.
(Hit cmd-enter too early, sorry)
If the goal is to make the stream impossible to use incorrectly, even in private contexts, then
as_inner_uncheckedshouldn't exist.
The goal is to make the stream impossible to use incorrectly in safe code
There was a problem hiding this comment.
Perhaps we should move UpgradedSendStream into a separate module and make it public. That way we can use visibility to prevent users from accessing .0 while providing an API without using transmute under the hood.
There was a problem hiding this comment.
I attempted this and found that write was being called from the AsyncWrite impl for H2Upgraded. Should these uses additionally have as_inner_unchecked for some reason? Or am I misunderstanding why the original implementation Neutered the stream?
There was a problem hiding this comment.
There is no H2Upgraded in this patch, did you mean another type?
The Neutered<_> thing is to show that we are not using that variant SendBuf::Buf there, only SendBuf::Cursor.
Requires hyperium/h2#614 before this will compile.
We can avoid a transmute and some questionable code by using a specially-crafted conditional
Sendimpl.Sendableand the impl are inside aconst _: () = { ... }block to prevent the otherwise unsoundSendabletype from being visible to the rest of the crate.I believe that both versions are equally sound, but this helps prevent future issues and makes auditing the code more straightforward.