Skip to content

Replace transmute with bounded manual Send impl #2830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Replace transmute with bounded manual Send impl #2830

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 18 additions & 26 deletions src/proto/h2/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use http::HeaderMap;
use pin_project_lite::pin_project;
use std::error::Error as StdError;
use std::io::{self, Cursor, IoSlice};
use std::mem;
use std::task::Context;
use tokio::io::{AsyncRead, AsyncWrite, ReadBuf};
use tracing::{debug, trace, warn};
Expand Down Expand Up @@ -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>>);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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_unchecked shouldn't exist.

The goal is to make the stream impossible to use incorrectly in safe code

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


impl<B> UpgradedSendStream<B>
where
B: Buf,
{
unsafe fn new(inner: SendStream<SendBuf<B>>) -> Self {
assert_eq!(mem::size_of::<B>(), mem::size_of::<Neutered<B>>());
Self(mem::transmute(inner))
Self(inner)
}

fn reserve_capacity(&mut self, cnt: usize) {
Expand All @@ -442,30 +440,24 @@ where
}

unsafe fn as_inner_unchecked(&mut self) -> &mut SendStream<SendBuf<B>> {
&mut *(&mut self.0 as *mut _ as *mut _)
&mut self.0
}
}

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

enum Impossible {}

unsafe impl<B> Send for Neutered<B> {}
const _: () = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use an unnamed const rather than a private module?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Private modules need names, const items don't. Less names is better, IMO.

#[repr(transparent)]
struct Sendable<B>(B);

impl<B> Buf for Neutered<B> {
fn remaining(&self) -> usize {
match self.impossible {}
}
unsafe impl<B> Send for Sendable<B> {}

fn chunk(&self) -> &[u8] {
match self.impossible {}
}

fn advance(&mut self, _cnt: usize) {
match self.impossible {}
}
}
// Because `B` can't be sent through an `UpgradedSendStream`, we want to implement `Send` even if
// `B` is not. We can do this by only implementing `Send` on `UpgradedSendStream<B>` if
// `SendStream<StreamBuf<Sendable<B>>>` is also `Send`. This is better than unconditionally
// implementing it because in that case `UpgradedSendStream` would be `Send` even when
// `SendStream<SendBuf<B>>` is not (for some `B: Send`). This could be the case if `SendStream` or
// `SendBuf` stopped being `Send` (even for sendable `B`).
unsafe impl<B> Send for UpgradedSendStream<B>
where
SendStream<SendBuf<Sendable<B>>>: Send,
{}
};