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

[core] Fix a soundness hole in core::sync::Exclusive #106792

Closed
wants to merge 1 commit into from

Conversation

dead-claudia
Copy link

@dead-claudia dead-claudia commented Jan 13, 2023

Just like Mutex<T> is only Sync if T is Send, Exclusive<T> should contain the same constraints.

As an example, sd_journal* instances created per https://www.freedesktop.org/software/systemd/man/sd_journal_open.html# cannot be either Send or Sync, and wrapper types likewise can't.

All functions listed here are thread-agnostic and only a single specific thread may operate on a given object during its entire lifetime. It's safe to allocate multiple independent objects and use each from a specific thread in parallel. However, it's not safe to allocate such an object in one thread, and operate or free it from any other, even if locking is used to ensure these threads don't operate on it at the very same time.

The current API would technically allow a wrapper that (correctly) doesn't implement Send or Sync to be erroneously moved to another thread. This fixes that soundness hole.

Edit: Relevant issue: #98407

@rustbot
Copy link
Collaborator

rustbot commented Jan 13, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 13, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 13, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@dead-claudia dead-claudia changed the title Fix a soundness hole in core::sync::Exclusive [core] Fix a soundness hole in core::sync::Exclusive Jan 13, 2023
@dead-claudia dead-claudia force-pushed the patch-1 branch 2 times, most recently from 70336ec to 278974e Compare January 13, 2023 03:46
Just like `Mutex<T>` is only `Sync` if `T` is `Send`, `Exclusive<T>` should contain the same constraints.
@dead-claudia
Copy link
Author

@rustbot label +T-libs-api -T-libs

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 13, 2023
@clubby789
Copy link
Contributor

clubby789 commented Jan 13, 2023

The purpose of Exclusive is to always implement Sync, so adding bounds goes against the documented behavior. This implementation upholds the contract for Sync:

the safety requirements of Sync state that for Exclusive to be Sync, it must be sound to share across threads, that is, it must be sound for &Exclusive to cross thread boundaries. By design, a &Exclusive has no API whatsoever, making it useless, thus harmless, thus memory safe.

For the given example of sd_journal*, it is also safe to send pointers across threads, as dereferencing them would require additional unsafe.

@ibraheemdev
Copy link
Member

ibraheemdev commented Jan 13, 2023

The current API would technically allow a wrapper that (correctly) doesn't implement Send or Sync to be erroneously moved to another thread.

It does not, because Exclusive<T: !Send> does not implement Send.

@dead-claudia
Copy link
Author

Closed since I can't seem to find a std counterexample to @ibraheemdev's claim.

@dead-claudia dead-claudia deleted the patch-1 branch January 13, 2023 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants