Skip to content
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.

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

io: add T: Unpin bound to ReadHalf::unsplit #5375

Merged
merged 2 commits into from Jan 17, 2023
Merged

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Jan 13, 2023

Motivation

Fixes #5372

Solution

Adds T: Unpin bound to ReadHalf::unsplit.
Similar functions in futures also have the same bound, I believe this should be a reasonable fix here.

https://docs.rs/futures/latest/futures/io/struct.ReadHalf.html#impl-ReadHalf%3CT%3E
https://docs.rs/futures/latest/futures/stream/struct.SplitStream.html#impl-SplitStream%3CS%3E

@taiki-e taiki-e added A-tokio Area: The main tokio crate M-io Module: tokio/io labels Jan 13, 2023
@taiki-e
Copy link
Member Author

taiki-e commented Jan 13, 2023

Hmm, it seems that the change that should be backported must be sent the patch to the old LTS version first?

@notgull
Copy link
Contributor

notgull commented Jan 13, 2023

Isn't this a breaking change?

@taiki-e
Copy link
Member Author

taiki-e commented Jan 14, 2023

A braking change to fix soundness issues is allowed (see also rfc1122).

It depends on the number of crates affected by the breakage and whether there are other (good) ways to fix it, but I don't know of any way to fix this other than the "(auto-ref) specialization + run-time check + run-time panic" (which is not very good).

@carllerche
Copy link
Member

Thanks, the patch looks good, but we need to apply this to the oldest LTS branch first, release, and forward port.

This is similar to #5336 and the "merge changes" https://github.com/tokio-rs/tokio/pulls?q=is%3Apr+author%3Acarllerche+is%3Aclosed

This lets us maintain the history.

I can work on it Tuesday, but if you are able to start the patching, I would appreciate it! For the patches to the LTS branches you can combine the fix + the "release" changes. like #5336 does.

@carllerche
Copy link
Member

Re: "breaking change" I agree w/ @taiki-e. This is to fix a soundness issue, and we are taking the least intrusive approach. It shouldn't break anyone's code. Any code that does break is unsound and should be fixed.

@taiki-e taiki-e changed the base branch from master to tokio-1.18.x January 17, 2023 12:54
@taiki-e
Copy link
Member Author

taiki-e commented Jan 17, 2023

Changed base branch to tokio-1.18.x and added "prepare release" commit.

I will leave it to carllerche for other branches. (I don't think I'm awake at the time this is merged, because it's night in Japan.)

@carllerche carllerche merged commit 171ce0f into tokio-1.18.x Jan 17, 2023
@carllerche carllerche deleted the taiki-e/unsplit branch January 17, 2023 18:02
littledivy pushed a commit to denoland/deno that referenced this pull request Jan 19, 2023
Tokio 1.24.2 fixed an unsoundness issue
tokio-rs/tokio#5375
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-io Module: tokio/io
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tokio::io::ReadHalf<T>::unsplit can violate Pin contract when T: !Unpin
3 participants