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

Keep provenance intact by avoiding ptr-int-ptr #185

Merged
merged 2 commits into from
Aug 16, 2022

Conversation

saethlin
Copy link
Contributor

@saethlin saethlin commented Jun 28, 2022

once_cell is an extremely widely-used crate, so it would be very nice if it conformed to the stricted/simplest/checkable model we have for aliasing in Rust. To do this, we need to avoid creating a pointer from an integer by cast or transmute. Pointers created this way can be valid, but are a nightmare for a checker like Miri. The situation is generally explained by these docs: https://doc.rust-lang.org/nightly/std/ptr/fn.from_exposed_addr.html

This implementation is mostly gross because all the APIs that would make this ergonomic are behind #![feature(strict_provenance)]. (in particular, it would be great to have map_addr here)

src/imp_std.rs Outdated Show resolved Hide resolved
@saethlin
Copy link
Contributor Author

saethlin commented Jul 2, 2022

When running the tests locally with MIRIFLAGS=-Zmiri-disable-isolation cargo miri test --all-features Miri ends up in a deadlock. It looks like this doesn't reproduce in CI.
This is related to the parking_lot feature which has other problems I am reporting separately.

@matklad
Copy link
Owner

matklad commented Jul 4, 2022

. (in particular, it would be great to have map_addr here)

Could you add a map_addr as a free-standing function ("polyfill")?

src/imp_std.rs Outdated Show resolved Hide resolved
src/imp_std.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

if we have that polyfill we might as well use it? :D

src/imp_std.rs Outdated Show resolved Hide resolved
src/imp_std.rs Outdated Show resolved Hide resolved
src/imp_std.rs Outdated Show resolved Hide resolved
src/imp_std.rs Outdated Show resolved Hide resolved
hawkw added a commit to hawkw/once_cell that referenced this pull request Jul 30, 2022
…enance

This branch merges upstream's `master` branch into PR matklad#185.

I need to patch a dependency on `once_cell` that needs at least 1.13.0
to pick up the provenance fix, but you may want to actually merge this
into the PR as well.
hawkw added a commit to hawkw/mycelium that referenced this pull request Jul 30, 2022
This adds a Cargo patch for the `once_cell` crate to pick up an upstream
branch that fixes an int-to-pointer cast that Miri rejects.

The patch can be removed once matklad/once_cell#185 has merged.
hawkw added a commit to hawkw/mycelium that referenced this pull request Jul 30, 2022
This adds a Cargo patch for the `once_cell` crate to pick up an upstream
branch that fixes an int-to-pointer cast that Miri rejects.

The patch can be removed once matklad/once_cell#185 has merged.
hawkw added a commit to hawkw/mycelium that referenced this pull request Jul 30, 2022
This adds a Cargo patch for the `once_cell` crate to pick up an upstream
branch that fixes an int-to-pointer cast that Miri rejects.

The patch can be removed once matklad/once_cell#185 has merged.
saethlin added 2 commits July 30, 2022 18:06
once_cell is an extremely widely-used crate, so it would be very nice if
it conformed to the stricted/simplest/checkable model we have for
aliasing in Rust. To do this, we need to avoid creating a pointer from
an integer by cast or transmute. Pointers created this way can be valid,
but are a nightmare for a checker like Miri. The situation is generally
explained by these docs: https://doc.rust-lang.org/nightly/std/ptr/fn.from_exposed_addr.html

This implementation is mostly gross because all the APIs that would make
this ergonomic are behind #![feature(strict_provenance)]
@LoganDark
Copy link

Is there a reason to be pasting in sptr rather than depending on it?

@Noratrieb
Copy link

I assume it's to avoid having an extra dependency, as sptr contains more code than is required here, which would have a negative impact on build times.

@LoganDark
Copy link

I assume it's to avoid having an extra dependency, as sptr contains more code than is required here, which would have a negative impact on build times.

sptr is a relatively small crate compared to what you'd usually blame for inflating compile times; a proper benchmark is required to reach a proper conclusion. I'd assume the bulk of the overhead would be the dependency itself (i.e. cargo having to download and compile it separately), not the code inside, but cargo is already very good at compiling large dependency trees.

@matklad
Copy link
Owner

matklad commented Aug 16, 2022

bors r+

I don't think it's worth it to go from 0 deps to >0 deps just for this

@LoganDark
Copy link

Well, at least this is getting merged.

@bors
Copy link
Contributor

bors bot commented Aug 16, 2022

Build succeeded:

@bors bors bot merged commit 08b6dc1 into matklad:master Aug 16, 2022
@matklad
Copy link
Owner

matklad commented Aug 16, 2022

Released as https://crates.io/crates/once_cell/1.13.1

bors bot added a commit that referenced this pull request Feb 14, 2023
219: Use AtomicPtr for race::OnceRef to avoid ptr-int-ptr r=matklad a=Imberflur

Same rationale as #185

Potentially, a `OncePtr` abstraction could be factored out and used in both `OnceBox` and `OnceRef`.

Co-authored-by: Imbris <imbrisf@gmail.com>
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.

5 participants