Fix undefined behavior identified by Miri #280
Conversation
There was a problem hiding this comment.
thanks for the PR. changes look good to me.
the sanitizer tests are failing. I think it may be because rustc updated the dwarf it outputs. there's this in the output
/usr/bin/addr2line: DWARF error: invalid or unhandled FORM value: 0x25
I cannot repro locally but will investigate in the CI environment
PR #283 fixes CI. this will need to wait until that one lands. |
|
#283 is merged now, @jgallagher could you rebase on master? |
|
bors r+ |
|
bors merge |
|
bors r+ |
|
bors ping |
1 similar comment
|
bors ping |
|
pong |
|
bors merge |
|
Build succeeded: |
|
Thanks! |
|
I read a bit more about Miri and the and the docs for
I tried enabling |
|
👋 Since the rules in Rust aren't set, the right way to interpret Miri's errors are the interpreter reporting "This is UB with respect to the model you have requested with whatever combination of I'll admit I don't fully understand this anchoring system, but I've poked around it enough to mostly grasp what's going on from Miri's perspective. First off, it's entirely possible to remove the ptr-int-ptr roundtrips from this code. As is very often the case, the key is to not store a The reason you need this is that pointers in Rust (and probably in C) have provenance, extra shadow state that identifies the allocation they are for. In C and Rust, integers do not have provenance. So a cast from a pointer to an integer loses data and thus cannot necessarily round-trip. Pointers definitely do not round-trip through integers in Stacked Borrows with raw pointer tagging. That's the error you're seeing above. But in the existing Stacked Borrows with Untagged which is the default behavior in Miri, a cast from an integer to a pointer will check to see if there is an allocation at the address represented by the value of the That is the crux of why this code works without raw pointer tagging and as far as I can tell, cannot work with raw pointer tagging: A single anchor needs to be able to hand out multiple provenances. If the anchor is adjusted to contain a pointer, it will always hand out pointers with the provenance of that pointer, and you will get errors of a different sort, about attempting to dereference a pointer way outside its allocation. The anchor will be initialized once for example in the test called |
The tweaks in rust-embedded#280 missed one instance of UB. The get_unchecked_mut inside VacantEntry::Insert can be out of bounds of the initialized region of the backing Vec. When that happens, the call is UB. This is detected both by the standard library's debug assertions which can be enabled with -Zbuild-std and with Miri but only with -Zmiri-tag-raw-pointers. This also adds inherent as_ptr and as_mut_ptr methods to Vec which shadow those provided by the Deref to a slice. Without this shadowing, this change doesn't actually fix the problem identified by the debug assertions or Miri, it just hides it from the debug assertions. The core problem is that references narrow provenance, so if we want to access outside of the initialized region of a Vec we need to get a pointer to the array without passing through a reference to the initialized region first. The pointers from these shadowing methods can be used to access anywhere in the allocation, whereas vec.as_slice().as_ptr() would be UB to use for access into the uninitialized region.
The tweaks in rust-embedded#280 missed one instance of UB. The get_unchecked_mut inside VacantEntry::Insert can be out of bounds of the initialized region of the backing Vec. When that happens, the call is UB. This is detected both by the standard library's debug assertions which can be enabled with -Zbuild-std and with Miri but only with -Zmiri-tag-raw-pointers. This also adds inherent as_ptr and as_mut_ptr methods to Vec which shadow those provided by the Deref to a slice. Without this shadowing, this change doesn't actually fix the problem identified by the debug assertions or Miri, it just hides it from the debug assertions. The core problem is that references narrow provenance, so if we want to access outside of the initialized region of a Vec we need to get a pointer to the array without passing through a reference to the initialized region first. The pointers from these shadowing methods can be used to access anywhere in the allocation, whereas vec.as_slice().as_ptr() would be UB to use for access into the uninitialized region.
The fixes in rust-embedded#280 missed one instance of UB. The get_unchecked_mut inside VacantEntry::Insert can be out of bounds of the initialized region of the backing Vec. When that happens, the call is UB. This is detected both by the standard library's debug assertions which can be enabled with -Zbuild-std and with Miri but only with -Zmiri-tag-raw-pointers. This also adds inherent as_ptr and as_mut_ptr methods to Vec which shadow those provided by the Deref to a slice. Without this shadowing, this change doesn't actually fix the problem identified by the debug assertions or Miri, it just hides it from the debug assertions. The core problem is that references narrow provenance, so if we want to access outside of the initialized region of a Vec we need to get a pointer to the array without passing through a reference to the initialized region first. The pointers from these shadowing methods can be used to access anywhere in the allocation, whereas vec.as_slice().as_ptr() would be UB to use for access into the uninitialized region.
The fixes in rust-embedded#280 missed one instance of UB. The get_unchecked_mut inside VacantEntry::Insert can be out of bounds of the initialized region of the backing Vec. When that happens, the call is UB. This is detected both by the standard library's debug assertions which can be enabled with -Zbuild-std and with Miri but only with -Zmiri-tag-raw-pointers. This also adds inherent as_ptr and as_mut_ptr methods to Vec which shadow those provided by the Deref to a slice. Without this shadowing, the change from get_unchecked_mut to as_mut_ptr.add wouldn't actually fix the problem identified by the debug assertions or Miri, it just hides it from the debug assertions. The core problem is that references narrow provenance, so if we want to access outside of the initialized region of a Vec we need to get a pointer to the array without passing through a reference to the initialized region first. The pointers from these shadowing methods can be used to access anywhere in the allocation, whereas vec.as_slice().as_ptr() would be UB to use for access into the uninitialized region.
300: Fix OOB get_unchecked, shadow Vec::as_ptr methods r=japaric a=saethlin The fixes in #280 missed one instance of UB. The get_unchecked_mut inside VacantEntry::Insert can be out of bounds of the initialized region of the backing Vec. When that happens, the call is UB. This is detected both by the standard library's debug assertions which can be enabled with -Zbuild-std and with Miri but only with -Zmiri-tag-raw-pointers. This also adds inherent as_ptr and as_mut_ptr methods to Vec which shadow those provided by the Deref to a slice. Without this shadowing, the change from get_unchecked_mut to as_mut_ptr.add wouldn't actually fix the problem identified by the debug assertions or Miri, it just hides it from the debug assertions. The core problem is that references narrow provenance, so if we want to access outside of the initialized region of a Vec we need to get a pointer to the array without passing through a reference to the initialized region first. The pointers from these shadowing methods can be used to access anywhere in the allocation, whereas vec.as_slice().as_ptr() would be UB to use for access into the uninitialized region. Co-authored-by: Ben Kimock <[email protected]>
Yeah, using tag-raw-pointers and getting an "untagged" error basically always indicates a false positive. That's why tag-raw-pointers was not enabled by default. Miri has changed a lot the last month to eliminate those false positives. The entire notion of "untagged" pointers disappeared. Can you try again with the latest version of Miri? Of course ideally we'd get the code to work with |
|
(👋 you got mentioned in the Zulip thus the attention) It should in theory be possible to fix anchored pointers to work under strict provenance, though it'll be a bit of work. The short version is that However, |
Hi! We ran into an exception triggered by new undefined behavior checks inserted into the nightly compiler (https://github.com/rust-lang/rust/pull/92686/files#diff-54110dcedc5a4d976321aa5d2a6767ac0744a3ef1363b75ffc62faf81cf14c30R230-L229). Running
heapless's test suite under Miri didn't flag anything at first, but it did once we addedMIRIFLAGS="-Zmiri-tag-raw-pointers". All three of the fixes in this PR were identified viaand the fixes came from copying the implementations from the equivalent methods in
std. Note that I skipped thepool::tests; there is at least one miri failure in them, but it wasn't immediately obvious how to fix it so I skipped it for now. It's probably worth adding the flag above to the CI miri run, but I didn't do that either (since it would immediately cause failures given I didn't fix the problem inpool).The specific output for
poolis