search

rust-lang / backtrace-rs

Backtraces in Rust
https://docs.rs/backtrace
Other
537 stars 246 forks source link

Be compatible with -Zmiri-strict-provenance #477

Closed saethlin closed 2 years ago

saethlin commented 2 years ago

CI for https://github.com/rust-lang/backtrace-rs/pull/476 is currently failing, because CI asserts that we are compatible with strict provenance but we aren't.

For a crate like this I don't know if the right choice is to disable the checks or not. The use of pointers here is a bit unconventional. (decision: remove the strict provenance check)

saethlin commented 2 years ago

No idea what's going on with the CI at the moment. My best guess is that something with the windows runners is broken. I'll try again tomorrow.

alexcrichton commented 2 years ago

Is it possible to perhaps disable this in miri? I'd rather not lose the ability to have debugging assertions and otherwise it seems like larger refactorings would be necessary in this crate to truly support the strict-provenance changes.

saethlin commented 2 years ago

The problem with the existing code is that the backtrace-rs CI runs Miri with -Zmiri-strict-provenance, which reports an error if the code attempts to cast an integer to a pointer. The reason we have this behavior is honestly that the strict_provenance feature has not been stabilized, so we do not have any special intrinsics or any way to indicate that "I am converting a pointer to an integer and this allocation is now exposed" and "I am converting an integer to a pointer, and I expect to be able to dereference this pointer". Inside the strict provenance feature, these are called ptr::expose_addr and ptr::from_exposed_addr.

What backtrace-rs wants is a way to convert pointers to integers and back but with no expectation that the pointer will be dereferenced. Personally reading the code it is very strange to me that these pointers are ever used as usize. But currently, the way we express the operation that backtrace-rs wants is transmute (because the strict provenance feature is not stabilized), which I used in the first example.

Very pragmatically, since this repo sees relatively little attention and the whole strict provenance checking in Miri is unstable and the APIs to work in the model are still unstable, I think the best thing to do here is just remove the -Zmiri-strict-provenance flag from the CI. The whole point of the flag is to opt into a stricter model which is more checkable, and this codebase could probably follow that model but migrating to it might be a big PR (a lot of usize would probably have to be changed to some pointer type).

RalfJung commented 2 years ago

What is the problem with these debug assertions? I think they can be kept.

If the strict provenance API was stable, we could use ptr::invalid -- that is exactly the function to use for pointers that will not be dereferenced. This PRs just inlines that function since it is not stable yet.

saethlin commented 2 years ago

We could also add a dependency on sptr. (Or I guess paste its invalid impl into this crate)

saethlin commented 2 years ago

I believe CI still won't pass because of the problem with the 32-bit Windows check, but :shrug: this will get it farther than before

alexcrichton commented 2 years ago

Ok I think this is reasonable enough to land for now. If it becomes more necessary though to enable strict provenance in the standard library and CI I think it would probably be a good time to revisit all of the API details here to figure out an API that better suits that world.