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

Avoid lossy ptr-int transmutes by using AtomicPtr #66

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

saethlin
Copy link
Contributor

This project looks pretty inactive, but it is still used very widely by the ecosystem and Miri detects UB in it that looks unambiguously bad to me, so that's why I'm submitting a patch for it.

Rust doesn't have a formal memory model yet, but as far as we can tell it isn't sound to round-trip a pointer through an integer via transmutes in a compiler which does provenance-based optimizations (which LLVM and basically all modern compilers currently do). The fix here is pretty easy: we store an AtomicPtr instead of an AtomicUsize and we create our sentinel values using the wrapping_ pointer methods.

This PR is very similar to rust-lang/rust#95621.

@saethlin saethlin changed the title Avoid probably-UB ptr-in transmutes by using AtomicPtr Avoid probably-UB ptr-int transmutes by using AtomicPtr Apr 17, 2022
src/native/arc_list.rs Outdated Show resolved Hide resolved
@saethlin saethlin changed the title Avoid probably-UB ptr-int transmutes by using AtomicPtr Avoid lossy ptr-int transmutes by using AtomicPtr Sep 17, 2022
@Noratrieb
Copy link

@yoshuawuyts any chance on getting this merged soon? Would be pretty useful since this crate is causing Miri checking to fail for many crates.

@RalfJung
Copy link

Meanwhile, RFC 3559 has been accepted, which states that

This means that a pointer, in general, carries more information than can be captured by an integer type. For instance, transmuting a raw pointer to an array of u8, and then transmuting it back, does not restore the original pointer! (This RFC does not specify what exactly that roundtrip does. Unsafe code authors should conservatively assume that it is UB.)

@yoshuawuyts would be good to see this fix land so this widely-used crate can be brought into the realm of unambiguously sound code. :)

Copy link
Contributor

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trusting @Nilstrieb and @RalfJung that you've looked at this code and determined it correctly upholds the rules. That's good enough for me; thank you! - merging!

@yoshuawuyts yoshuawuyts merged commit cb6f0de into async-rs:master Feb 22, 2024
@yoshuawuyts
Copy link
Contributor

Published as v3.0.3

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.

4 participants