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

Fix soundness hole in join macros #2649

Merged

Conversation

Pointerbender
Copy link
Contributor

There is a soundness bug in the join macros where a new exclusive reference is taken out every time a future is polled. Under the Stacked Borrows model, this invalidates self-referential futures. This is then in conflict with the safety requirements for Pin::new_unchecked, which say that the pointer may never be invalidated after pinning it - and this leads to undefined behavior in safe Rust code.

I added a miri regression test for completeness (this example was made by @jswrenn), and together with @RalfJung we pin-pointed the problem and created a fix (Zulip thread). There is a minor down-side to the fix, the joined future increased in size, due to having to store an extra pinned reference in it, and I updated the tests to account for the increased sizes.

@RalfJung
Copy link
Member

RalfJung commented Oct 4, 2022

soundness bug

I should clarify that this is a bit unclear. ;) If we had proper support for self-referential generators in the language, then I think this code would be fine. But sadly self-referential generators were added in the frontend without first laying the necessary groundwork in the backend (in the form of appropriate primitives to handle the aliasing this generates), and until someone cleans up that mess, we are left with a hack: using the Unpin trait to control the aliasing requirements. Due to the fact that PollFn<F> is Unpin even if F is not, the code violates that hacky aliasing model.

See this IRLO thread for more details.

@Pointerbender
Copy link
Contributor Author

Sidenote: the CI seems to be failing to an unrelated problem that's already present on the master branch:

error: unreachable `pub` item
  --> futures-util/src/stream/futures_unordered/mod.rs:25:32
   |
25 | pub use self::iter::{IntoIter, Iter, IterMut, IterPinMut, IterPinRef};
   | ---                            ^^^^
   | |
   | help: consider restricting its visibility: `pub(crate)`
   |
   = help: or consider exporting it for use by other crates
   = note: `-D unreachable-pub` implied by `-D warnings`

error: unreachable `pub` item
  --> futures-util/src/stream/futures_unordered/mod.rs:25:38
   |
25 | pub use self::iter::{IntoIter, Iter, IterMut, IterPinMut, IterPinRef};
   | ---                                  ^^^^^^^
   | |
   | help: consider restricting its visibility: `pub(crate)`
   |
   = help: or consider exporting it for use by other crates

error: could not compile `futures-util` due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
Error: Process completed with exit code 101.

I ran MIRIFLAGS="-Zmiri-disable-isolation -Zmiri-backtrace=full" cargo miri test --workspace --all-features and cargo test --workspace --all-features locally without issues. Should I see if I can fix those warnings in this PR too or is someone else currently working on that?

@Pointerbender
Copy link
Contributor Author

p.s. the warnings in the CI seem to be false positives, the futures_util::stream::futures_unordered::{Iter, IterMut} items are available to other crates, so I'm not sure why it's complaining about those 2 in particular.

@RalfJung
Copy link
Member

RalfJung commented Oct 7, 2022

FWIW comex has demonstrated an actual miscompilation based on this soundness bug.

@taiki-e
Copy link
Member

taiki-e commented Oct 10, 2022

Thanks!

CI failure has been fixed by #2651. Could you rebase?

add a miri regression test
update failing tests (join sizes increased due to fix)
@Pointerbender Pointerbender force-pushed the fix-self-referential-stacked-borrows branch from afe1be7 to caf7db4 Compare October 12, 2022 17:45
…ts on "non-64-bit pointer" targets (e.g. `i686-unknown-linux-gnu`)

(this is the same fix that was also applied in PR rust-lang#2447)
@Pointerbender
Copy link
Contributor Author

Thank you for the CI fix! I rebased the PR and had to make one small addition to the test cases for join_size and try_join_size by adding a #[cfg_attr(not(target_pointer_width = "64"), ignore)] attribute to them. Due to that the futures now store an extra reference, the size will be different for non-64-bits pointer targets and this makes the tests fail on those targets due to the sizes being hard-coded for 64-bit wide pointers (this is the same fix as that was applied in #2447).

@taiki-e taiki-e merged commit f947828 into rust-lang:master Oct 13, 2022
@taiki-e taiki-e added 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. A-macro Area: macro related labels Oct 13, 2022
taiki-e pushed a commit that referenced this pull request Oct 20, 2022
* fix soundness hole in join macros
add a miri regression test
update failing tests (join sizes increased due to fix)

* fix `CI / cross test` by ignoring `join_size` and `try_join_size` tests on "non-64-bit pointer" targets (e.g. `i686-unknown-linux-gnu`)
(this is the same fix that was also applied in PR #2447)
@taiki-e taiki-e mentioned this pull request Oct 20, 2022
@taiki-e taiki-e added 0.3-backport: completed and removed 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels Oct 20, 2022
taiki-e pushed a commit that referenced this pull request Oct 20, 2022
* fix soundness hole in join macros
add a miri regression test
update failing tests (join sizes increased due to fix)

* fix `CI / cross test` by ignoring `join_size` and `try_join_size` tests on "non-64-bit pointer" targets (e.g. `i686-unknown-linux-gnu`)
(this is the same fix that was also applied in PR #2447)
@taiki-e
Copy link
Member

taiki-e commented Oct 20, 2022

Published in 0.3.25.

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Oct 20, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [futures](https://rust-lang.github.io/futures-rs) ([source](https://github.com/rust-lang/futures-rs)) | dependencies | patch | `0.3.24` -> `0.3.25` |

---

### Release Notes

<details>
<summary>rust-lang/futures-rs</summary>

### [`v0.3.25`](https://github.com/rust-lang/futures-rs/blob/HEAD/CHANGELOG.md#&#8203;0325---2022-10-20)

[Compare Source](rust-lang/futures-rs@0.3.24...0.3.25)

-   Fix soundness issue in `join!` and `try_join!` macros ([#&#8203;2649](rust-lang/futures-rs#2649))
-   Implement `Clone` for `sink::Drain` ([#&#8203;2650](rust-lang/futures-rs#2650))

</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 this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yNDAuNSIsInVwZGF0ZWRJblZlciI6IjMyLjI0MC41In0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1595
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>
jswrenn added a commit to tokio-rs/async-backtrace that referenced this pull request Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants