Enable permissive provenance by default by RalfJung · Pull Request #2275 · rust-...
source link: https://github.com/rust-lang/miri/pull/2275
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
This completes the plan laid out in #2133:
- We use permissive provenance with wildcard pointers by default.
- We print a warning on int2ptr casts.
-Zmiri-permissive-provenance
suppresses the warning;-Zmiri-strict-provenance
turns it into a hard error. - Raw pointer tagging is now always enabled, so we remove the
-Zmiri-tag-raw-pointers
flag and the code for untagged pointers. (Passing the flag still works, for compatibility -- but we just ignore it, with a warning.)
We also fix an intptrcast issue:
- Only live allocations are considered when computing the AllocId from an address.
So, finally, Miri has a good story for ptr2int2ptr roundtrips and no weird false negatives when doing raw pointer stuff with Stacked Borrows. :-) Thanks a lot to everyone who helped with this, in particular @carbotaniuman who convinced me this is even possible.
changed the title
Enabled permissive provenance by default
Enable permissive provenance by default
// #[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))] |
||
// static_assert_size!(Pointer<Option<Tag>>, 24); |
||
//static_assert_size!(Pointer<Option<Tag>>, 24); |
Member
Author
RalfJung 11 days ago
I am quite bummed by this... Tag
now has two NonZeroU64
, so we could use one to indicate Wildcard
and one to indicate None
, but it seems rustc is not smart enough for that. :(
I wonder if it's worth implementing the packing by hand, or if having the extra 8 bytes is just not such a big deal...
Contributor
oli-obk left a comment
I'm a bit surprised about all the behaviour changes. I would have expected this to just change the defaults. I guess the subsequent cleanups are the cause here?
I wish we had a good way to run perf. Maybe we could figure out a way to run a separate perfbot suite via the same infrastructure... I'll check that out
Outdated Show resolved
The change to check liveness of the allocation in intptrcast.rs is something I just discovered while working on this. I can factor it into a separate PR if you want. The change I just added to make allocations adjacent again could be made dependent on the provenance mode on master already, I guess. It doesn't work with legacy provenance though. I can factor it into a separate PR if you want. And that should be all the behavior changes I think? The remaining output changes come from the fact that we now enable raw pointer tagging by default. This also means we have to adjust some tests that inadvertantly relied on raw pointers being untagged. A bunch of tests were using int2ptr casts unnecessarily so I made them not do that. And finally I killed the 'untagged' support from Stacked Borrows since I cannot wait to see it go away. :) |
Contributor
oli-obk commented 11 days ago
yea I got that part. the rest seems ok to include here, I was just surprised, as the PR title and description did not make me think of anything beyond the test changes due to changes in defaults. |
Member
Author
RalfJung commented 11 days ago
That test failure is very strange, I cannot reproduce it locally
|
Ah, got it.
Seems to need the "adjacent allocation" change to reproduce. Interesting... |
Member
Author
RalfJung commented 11 days ago
The function pointer lacks provenance. Maybe I didn't think this part about adjacent allocations through carefully enough... I think I'll back that part out again. |
Member
Author
RalfJung commented 10 days ago
Ah bummer, getrandom 1 still needs permissive provenance on macOS... |
When running
Amusingly, one of the doctests runs into the same sort of issue, but in that case the ptr-int cast warning is displayed before the UB error. EDIT: Hm actually I have no idea what's going on here, this could just be a me problem. |
Outdated
use std::sync::atomic::{AtomicBool, Ordering}; |
||
static FIRST_WARNING: AtomicBool = AtomicBool::new(true); |
||
if FIRST_WARNING.swap(false, Ordering::Relaxed) { |
||
register_diagnostic(NonHaltingDiagnostic::Int2Ptr); |
Contributor
saethlin 10 days ago
--> /root/.cargo/registry/src/github.com-1ecc6299db9ec823/tendril-0.4.3/src/tendril.rs:1073:9
|
1073 | (self.ptr.get().get() & !1) as *mut Header<A>
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pointer-to-integer cast
|
= help: this program is using integer-to-pointer casts or (equivalently) `from_exposed_addr`,
The diagnostic says pointer-to-integer, but the span is an int-to-pointer cast. Similarly, this diagnostic is called Int2Ptr
but it is in the function called ptr_from_addr_cast
. Which direction are we trying to highlight?
Member
Author
RalfJung commented 10 days ago
I have seen the warning in
|
Member
Author
RalfJung commented 10 days ago
One thing I was wondering about: currently we only print the warning on the first cast. |
Contributor
saethlin commented 10 days ago
That may be useful to have behind a flag. What I am thinking of for diagnostics, is that when we encounter a problem we should be able to report what Miri knows about what offsets and with what permissions have been exposed in an allocation. I'd have to actually get to implementing this to know how useful it is, but for example in |
Member
Author
RalfJung commented 10 days ago
So many flags.^^ For now I'd like to have a reasonable default.
Provenances (AllocId + SbTag) are exposed, not addresses. And if the AllocId had not been exposed we would not even have gotten to the Stacked Borrows part. But likely the tag that was exposed is already invalid, or never was valid for this location. |
Contributor
saethlin commented 10 days ago
In real code, especially from a test suite, you can expect 1-2 backtraces to fit on a screen. I think if Miri emits 10, people will rapidly decide to always silence them, just like how people learn to always disable isolation. |
Contributor
saethlin commented 10 days ago
Many of the int2ptr warnings I've seen are for |
Member
Author
RalfJung commented 10 days ago
If they are not interested in removing these casts, that's fine.
Well, I do want to make sure people are aware that such a precision-losing cast happens -- even if they cannot do anything about it, IMO it is important that they can know that Miri might miss some bugs here. This warning is load-bearing, in a sense. We could prune the backtrace but I doubt that would help much... |
Member
Author
RalfJung commented 10 days ago
I'm going to land this for now, we can always fine-tune it later. @bors r+ |
Contributor
bors commented 10 days ago
Commit c1eddbc has been approved by |
Contributor
bors commented 10 days ago
Contributor
bors commented 10 days ago
Test successful - checks-actions |
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK