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

Add Send + Sync to ArcWake trait to fix unsoundness #1654

Merged
merged 1 commit into from
Jun 6, 2019

Conversation

seanmonstar
Copy link
Contributor

Neither Arc::new nor Arc<T> itself requires Send + Sync, but the into_waker method assumes them when converting into a Waker. So without this change, it's possible to trigger UB with something like Arc::new(Boom(some_rc)).into_waker().

@Nemo157
Copy link
Member

Nemo157 commented Jun 6, 2019

I guess Send is required because of Arc::try_unwrap?

(Just to check I threw together a playground with a full demonstration of the unsoundness, including cloning/dropping an Rc on multiple threads).

@seanmonstar
Copy link
Contributor Author

With an Rc, you could make some clones before putting in the Arc, and even making some clones on the other thread, making unsynchronized ref count changes.

Similar with RefCell and it's borrow count. Etcetera.

@Shnatsel
Copy link
Member

Unsynchronized reference count changes could potentially lead to exploitable memory corruption. Please file a security advisory at https://github.com/RustSec/advisory-db so that dependents of this crate could check if they're affected and upgrade.

@Nemo157
Copy link
Member

Nemo157 commented Aug 31, 2019

As far as I’m aware the unsoundness was only present in alpha releases of a preview crate, that should be sufficient “not production quality code” to not require a security advisory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants