Confusing stacked borrows violation #1364
Comments
|
Pepyakin and me have further minimized both tests and found out the following: #[test]
fn evil2() {
let array: [u8; 2] = [b'A', b'B'];
let array = UnsafeCell::new(array);
let (_loaded_a, _loaded_b) = unsafe {
let a = &mut (*array.get())[0];
let b = &mut *{
let cached_entries = &mut *array.get();
&mut cached_entries[1]
};
(a, b)
};
}
#[test]
fn saint2() {
let array: [u8; 2] = [b'A', b'B'];
let array = UnsafeCell::new(array);
let (_loaded_a, _loaded_b) = unsafe {
let b = &mut *{
let cached_entries = &mut *array.get();
&mut cached_entries[1]
};
let a = &mut (*array.get())[0];
(a, b)
};
}The problem is that there is a So with the above minification we believe that edit: When trying to fix the real codebase using the above implications I tried to delay the pointer dereferencing a bit. Basically changing unsafe { (
&mut *self.load_through_cache(a).as_ptr(),
&mut *self.load_through_cache(b).as_ptr(),
) };to unsafe {
let loaded_a = self.load_through_cache(a).as_ptr();
let loaded_b = self.load_through_cache(b).as_ptr();
(&mut *loaded_a, &mut *loaded_b)
};Which also fixed the problem for |
|
What is confusing me here is why you are using In this line: let cached_entries = &mut *array.get();you are creating a new mutable reference covering the entire array. Mutable references must be unique, so this pretty much says "there cannot be any other pointer to this array, this is the only one". Any other pointer that already exists becomes invalid, in particular So, your original The reason Btw, I am not sure if that's also useful for your original code, but you can write all this without any pub fn evil_fixed() {
let mut array: [u8; 2] = [b'A', b'B'];
let (a, rest) = array.split_first_mut().unwrap();
let (b, _) = rest.split_first_mut().unwrap();
// Now `a` and `b` are mutable references to the two array elements.
}(I did not read your 2nd post yet, this is just responding to the original one.) |
|
Just had a chat about this problem with @oli-obk and the conclusion of it was that unsafe { (
&mut *self.load_through_cache(a).as_ptr(),
&mut *self.load_through_cache(b).as_ptr(),
) };is failing unsafe {
let loaded_a = self.load_through_cache(a).as_ptr();
let loaded_b = self.load_through_cache(b).as_ptr();
(&mut *loaded_a, &mut *loaded_b)
};it is the result of the first code example being translated by While this is a strange conclusion to me the problem is now fixed and we can actually close this. :) |
|
(Now responding to the 2nd post) unsafe {
let loaded_a = self.load_through_cache(a).as_ptr();
let loaded_b = self.load_through_cache(b).as_ptr();
(&mut *loaded_a, &mut *loaded_b)
};I think this works "by accident" -- when you use raw pointers, Miri currently is not very strict at all, so that we can start by finding the much more pressing issues with people using references wrong. But longer-term we will need to change that. And if In that post you also have But if this fails unsafe { (
&mut *self.load_through_cache(a).as_ptr(),
&mut *self.load_through_cache(b).as_ptr(),
) };then there likely still is a bug, as the second call is invalidating the first reference. |
RalfJung
added
A-aliasing
Please note that the first example are just minimizations of the second post examples. |
Usage of
So we need the |
That seems reasonable.
Exactly. And that is also a problem with the variant that delays creating the mutable reference: you have a raw pointer, and then you create a mutable reference (not derived from that raw pointer) that overlaps with the raw pointer. That makes the raw pointer invalid. Miri just does not catch that because you later create another raw pointer and it "confused" the two raw pointers because we do not track precisely enough. |
|
This line here is a problem:
This looks like it does the same: I thought by making Miri accept more code I'd help people, here it seems that got confusing. ;) |
Wow okay thank you for the explanation. Seems like I am super confused by now. The
I actually consider using |
There actually is another difference because the |
If you then use shared references to the array of
But since you said later you have an indirection with a So this is probably actually okay then: That is probably why you are not seeing Miri failures with the BTree thing. |
|
Okay thank you again for the explanation. I now think I got a grasp of the underlying problem. Thank you a lot! |
Details to the found in rust-lang#1364. Note that this was not a found in a `master` or production release of ink!, however without analysing the code via `miri` this could have potentially happened.
|
All right, I am closing this ticket then. Glad that it was helpful :) |
…alfJung Add miri trophy for LazyArray::swap (ink! PR) Details to the found in #1364. Note that this was not a found in a `master` or production release of ink!, however without analysing the code via `miri` this could have potentially happened.
…alfJung Add miri trophy for LazyArray::swap (ink! PR) Details to the found in #1364. Note that this was not a found in a `master` or production release of ink!, however without analysing the code via `miri` this could have potentially happened.
Running
mirion one of our codebases revealed some violation of its stacked borrow model. https://github.com/pepyakin was able to craft a minimized version of the problem:While
mirihas no problems with thesaintversion it finds a stacked borrows violation inevil. Note that a difference between the two is thatevilpacksloaded_aandloaded_binto a tuple.The output of
miriof testingevilis the following:The text was updated successfully, but these errors were encountered: