macros: don't take ownership of futures in macros #5087

Darksonn · 2022-10-10T08:09:56Z

This PR mitigates the issue discussed in Surprising soundness trouble around PollFn. The issue is mitigated in two ways:

Never give a PollFn closure ownership of a future. Instead, we give the closure a reference to the future. This way, a reference to the closure is not a reference to a future, so even if a reference to the closure is noalias, it doesn't leak to the future.
Make the PollFn type unconditionally !Unpin so that rustc doesn't mark mutable references to it as noalias.

As far as I understand, either mitigation is enough on its own to stop the problem, but I don't see any harm in adding both. Our PollFn is not part of the public API of Tokio, so there's no problem with making it !Unpin. The type is only marked pub to allow use in code generated by our macros.

Adding a test for this is surprisingly difficult due to rust#52234, since we don't run miri on the tests/ folder, so I haven't done so. However, I have tested locally that the mitigations work.

Darksonn · 2022-10-10T08:34:48Z

This does increase the size of the futures. We could decide to only change poll_fn.rs to avoid that, though that mitigation feels like the more brittle of the two to me.

RalfJung

Not knowing tokio I can't check if those macro changes make any sense, but on the conceptual level this makes sense to me. I left some comments that could help clarify what exactly the reference change in the macro does.

And yes either of these changes should be enough.

tokio/src/macros/join.rs

tokio/src/future/poll_fn.rs

carllerche

I don't really understand the details of the issue... it is unfortunate that the futures get bigger :(

hawkw

this looks correct to me! i commented on a couple very minor nits.

it's kind of a shame that the futures got a word larger, but...what can you do, i suppose. i guess that mitigation is worth having in case the implementation of join! or select! is ever changed to not use tokio's version of PollFn...

tokio/src/macros/join.rs

hawkw · 2022-10-10T20:12:35Z

tokio/tests/macros_join.rs

@@ -64,6 +65,7 @@ async fn two_await() {
 }

 #[test]
+#[cfg(target_pointer_width = "64")]


should we be making similar assertions about the size of the futures on 32-bit platforms as well? or, calculate the expected size by adding either 8 or 4 bytes based on the value of target_pointer_width?

hawkw · 2022-10-10T20:13:04Z

tokio/tests/macros_select.rs

@@ -207,6 +207,7 @@ async fn nested() {
 }

 #[maybe_tokio_test]
+#[cfg(target_pointer_width = "64")]


as with join, could we make a separate set of assertions about the future size on 32-bit platforms?

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.21.2` -> `1.22.0` | | [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.21.2` -> `1.22.0` | --- ### Release Notes <details> <summary>tokio-rs/tokio</summary> ### [`v1.22.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.22.0): Tokio v1.22.0 [Compare Source](tokio-rs/tokio@tokio-1.21.2...tokio-1.22.0) ##### Added - runtime: add `Handle::runtime_flavor` ([#5138]) - sync: add `Mutex::blocking_lock_owned` ([#5130]) - sync: add `Semaphore::MAX_PERMITS` ([#5144]) - sync: add `merge()` to semaphore permits ([#4948]) - sync: add `mpsc::WeakUnboundedSender` ([#5189]) ##### Added (unstable) - process: add `Command::process_group` ([#5114]) - runtime: export metrics about the blocking thread pool ([#5161]) - task: add `task::id()` and `task::try_id()` ([#5171]) ##### Fixed - macros: don't take ownership of futures in macros ([#5087]) - runtime: fix Stacked Borrows violation in `LocalOwnedTasks` ([#5099]) - runtime: mitigate ABA with 32-bit queue indices when possible ([#5042]) - task: wake local tasks to the local queue when woken by the same thread ([#5095]) - time: panic in release mode when `mark_pending` called illegally ([#5093]) - runtime: fix typo in expect message ([#5169]) - runtime: fix `unsync_load` on atomic types ([#5175]) - task: elaborate safety comments in task deallocation ([#5172]) - runtime: fix `LocalSet` drop in thread local ([#5179]) - net: remove libc type leakage in a public API ([#5191]) - runtime: update the alignment of `CachePadded` ([#5106]) ##### Changed - io: make `tokio::io::copy` continue filling the buffer when writer stalls ([#5066]) - runtime: remove `coop::budget` from `LocalSet::run_until` ([#5155]) - sync: make `Notify` panic safe ([#5154]) ##### Documented - io: fix doc for `write_i8` to use signed integers ([#5040]) - net: fix doc typos for TCP and UDP `set_tos` methods ([#5073]) - net: fix function name in `UdpSocket::recv` documentation ([#5150]) - sync: typo in `TryLockError` for `RwLock::try_write` ([#5160]) - task: document that spawned tasks execute immediately ([#5117]) - time: document return type of `timeout` ([#5118]) - time: document that `timeout` checks only before poll ([#5126]) - sync: specify return type of `oneshot::Receiver` in docs ([#5198]) ##### Internal changes - runtime: use const `Mutex::new` for globals ([#5061]) - runtime: remove `Option` around `mio::Events` in io driver ([#5078]) - runtime: remove a conditional compilation clause ([#5104]) - runtime: remove a reference to internal time handle ([#5107]) - runtime: misc time driver cleanup ([#5120]) - runtime: move signal driver to runtime module ([#5121]) - runtime: signal driver now uses I/O driver directly ([#5125]) - runtime: start decoupling I/O driver and I/O handle ([#5127]) - runtime: switch `io::handle` refs with scheduler:Handle ([#5128]) - runtime: remove Arc from I/O driver ([#5134]) - runtime: use signal driver handle via `scheduler::Handle` ([#5135]) - runtime: move internal clock fns out of context ([#5139]) - runtime: remove `runtime::context` module ([#5140]) - runtime: keep driver cfgs in `driver.rs` ([#5141]) - runtime: add `runtime::context` to unify thread-locals ([#5143]) - runtime: rename some confusing internal variables/fns ([#5151]) - runtime: move `coop` mod into `runtime` ([#5152]) - runtime: move budget state to context thread-local ([#5157]) - runtime: move park logic into runtime module ([#5158]) - runtime: move `Runtime` into its own file ([#5159]) - runtime: unify entering a runtime with `Handle::enter` ([#5163]) - runtime: remove handle reference from each scheduler ([#5166]) - runtime: move `enter` into `context` ([#5167]) - runtime: combine context and entered thread-locals ([#5168]) - runtime: fix accidental unsetting of current handle ([#5178]) - runtime: move `CoreStage` methods to `Core` ([#5182]) - sync: name mpsc semaphore types ([#5146]) [#4948]: tokio-rs/tokio#4948 [#5040]: tokio-rs/tokio#5040 [#5042]: tokio-rs/tokio#5042 [#5061]: tokio-rs/tokio#5061 [#5066]: tokio-rs/tokio#5066 [#5073]: tokio-rs/tokio#5073 [#5078]: tokio-rs/tokio#5078 [#5087]: tokio-rs/tokio#5087 [#5093]: tokio-rs/tokio#5093 [#5095]: tokio-rs/tokio#5095 [#5099]: tokio-rs/tokio#5099 [#5104]: tokio-rs/tokio#5104 [#5106]: tokio-rs/tokio#5106 [#5107]: tokio-rs/tokio#5107 [#5114]: tokio-rs/tokio#5114 [#5117]: tokio-rs/tokio#5117 [#5118]: tokio-rs/tokio#5118 [#5120]: tokio-rs/tokio#5120 [#5121]: tokio-rs/tokio#5121 [#5125]: tokio-rs/tokio#5125 [#5126]: tokio-rs/tokio#5126 [#5127]: tokio-rs/tokio#5127 [#5128]: tokio-rs/tokio#5128 [#5130]: tokio-rs/tokio#5130 [#5134]: tokio-rs/tokio#5134 [#5135]: tokio-rs/tokio#5135 [#5138]: tokio-rs/tokio#5138 [#5138]: tokio-rs/tokio#5138 [#5139]: tokio-rs/tokio#5139 [#5140]: tokio-rs/tokio#5140 [#5141]: tokio-rs/tokio#5141 [#5143]: tokio-rs/tokio#5143 [#5144]: tokio-rs/tokio#5144 [#5144]: tokio-rs/tokio#5144 [#5146]: tokio-rs/tokio#5146 [#5150]: tokio-rs/tokio#5150 [#5151]: tokio-rs/tokio#5151 [#5152]: tokio-rs/tokio#5152 [#5154]: tokio-rs/tokio#5154 [#5155]: tokio-rs/tokio#5155 [#5157]: tokio-rs/tokio#5157 [#5158]: tokio-rs/tokio#5158 [#5159]: tokio-rs/tokio#5159 [#5160]: tokio-rs/tokio#5160 [#5161]: tokio-rs/tokio#5161 [#5163]: tokio-rs/tokio#5163 [#5166]: tokio-rs/tokio#5166 [#5167]: tokio-rs/tokio#5167 [#5168]: tokio-rs/tokio#5168 [#5169]: tokio-rs/tokio#5169 [#5171]: tokio-rs/tokio#5171 [#5172]: tokio-rs/tokio#5172 [#5175]: tokio-rs/tokio#5175 [#5178]: tokio-rs/tokio#5178 [#5179]: tokio-rs/tokio#5179 [#5182]: tokio-rs/tokio#5182 [#5189]: tokio-rs/tokio#5189 [#5191]: tokio-rs/tokio#5191 [#5198]: tokio-rs/tokio#5198 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1651 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

macros: don't take ownership of futures in macros

ccf30c5

Darksonn added A-tokio Area: The main tokio crate M-macros Module: macros in the main Tokio crate labels Oct 10, 2022

Darksonn requested review from carllerche and hawkw October 10, 2022 08:11

Darksonn added 2 commits October 10, 2022 10:18

fix

4a4c137

Fix size tests

12fd8ce

Darksonn added 3 commits October 10, 2022 10:46

Don't test macro sizes on 32-bit platforms

f734620

Merge 'upstream/master' into 'alice/mitigate-pollfn-trouble'

d9eea3e

Fix unused import

da99e6d

RalfJung reviewed Oct 10, 2022

View reviewed changes

tokio/src/macros/join.rs Show resolved Hide resolved

tokio/src/future/poll_fn.rs Outdated Show resolved Hide resolved

Darksonn added 3 commits October 10, 2022 12:35

Add comments

0bae230

Remove unused import

8cf8a03

Comment on why pin-project is not used

624dff4

carllerche approved these changes Oct 10, 2022

View reviewed changes

hawkw approved these changes Oct 10, 2022

View reviewed changes

Darksonn merged commit 6929dec into master Oct 14, 2022

Darksonn deleted the alice/mitigate-pollfn-trouble branch October 14, 2022 07:45

Aaron1011 mentioned this pull request Oct 20, 2022

poll_fn and Unpin: fix pinning rust-lang/rust#102737

Merged

Darksonn mentioned this pull request Nov 17, 2022

chore: prepare Tokio v1.22.0 #5203

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

macros: don't take ownership of futures in macros #5087

macros: don't take ownership of futures in macros #5087

Darksonn commented Oct 10, 2022

Darksonn commented Oct 10, 2022

RalfJung left a comment •

edited

Loading

carllerche left a comment

hawkw left a comment

hawkw Oct 10, 2022

hawkw Oct 10, 2022

macros: don't take ownership of futures in macros #5087

macros: don't take ownership of futures in macros #5087

Conversation

Darksonn commented Oct 10, 2022

Darksonn commented Oct 10, 2022

RalfJung left a comment • edited Loading

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw Oct 10, 2022

Choose a reason for hiding this comment

hawkw Oct 10, 2022

Choose a reason for hiding this comment

RalfJung left a comment •

edited

Loading