Skip to content

Commit

Permalink
Auto merge of #359 - Nilstrieb:retag-me-im-a-field, r=Amanieu
Browse files Browse the repository at this point in the history
Fix issue with field retagging in scopeguard

With field retagging in SB, this causes UB because the read out value is invalidated by the move into `mem::forget`. Using `ManuallyDrop<T>` avoids the problem. With that issue fixed, we can now enable field retagging in CI. Field retagging is required to justify some of the `noalias` optimizations that rustc
performs currently.

<details>
<summary>Miri error</summary>

```rust
Undefined Behavior: trying to retag from <565074> for Unique permission at alloc212719[0x20], but that tag does not exist in the borrow stack for this location
    --> src/scopeguard.rs:40:13
     |
40   |             value
     |             ^^^^^
     |             |
     |             trying to retag from <565074> for Unique permission at alloc212719[0x20], but that tag does not exist in the borrow stack for this location
     |             this error occurs as part of retag at alloc212719[0x20..0x40]
     |
     = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
     = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <565074> was created by a Unique retag at offsets [0x20..0x40]
    --> src/scopeguard.rs:33:25
     |
33   |             let value = ptr::read(&guard.value);
     |                         ^^^^^^^^^^^^^^^^^^^^^^^
help: <565074> was later invalidated at offsets [0x20..0x40] by a Unique retag
    --> src/scopeguard.rs:39:25
     |
39   |             mem::forget(guard);
     |                         ^^^^^
     = note: backtrace:
     = note: inside `scopeguard::ScopeGuard::<&mut raw::RawTable<(i32, i32)>, [closure@src/raw/mod.rs:1637:45: 1637:52]>::into_inner` at src/scopeguard.rs:40:13
note: inside `<raw::RawTable<(i32, i32)> as core::clone::Clone>::clone_from` at src/raw/mod.rs:1671:17
    --> src/raw/mod.rs:1671:17
     |
1671 |                 ScopeGuard::into_inner(self_);
     |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `<map::HashMap<i32, i32> as core::clone::Clone>::clone_from` at src/map.rs:202:9
    --> src/map.rs:202:9
     |
202  |         self.table.clone_from(&source.table);
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `map::test_map::test_clone_from` at src/map.rs:6692:9
    --> src/map.rs:6692:9
     |
6692 |         m2.clone_from(&m);
     |         ^^^^^^^^^^^^^^^^^
note: inside closure at src/map.rs:6684:5
    --> src/map.rs:6684:5
     |
6683 |       #[test]
     |       ------- in this procedural macro expansion
6684 | /     fn test_clone_from() {
6685 | |         let mut m = HashMap::new();
6686 | |         let mut m2 = HashMap::new();
6687 | |         assert_eq!(m.len(), 0);
...    |
6695 | |         assert_eq!(m2.len(), 2);
6696 | |     }
     | |_____^
     = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
```

</details>
  • Loading branch information
bors committed Sep 9, 2022
2 parents ea7c3ba + 1f751c6 commit 2a7c322
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 9 deletions.
2 changes: 1 addition & 1 deletion ci/miri.sh
Expand Up @@ -9,4 +9,4 @@ rustup toolchain install nightly --component miri
rustup override set nightly
cargo miri setup

cargo miri test
MIRIFLAGS='-Zmiri-retag-fields' cargo miri test
14 changes: 6 additions & 8 deletions src/scopeguard.rs
@@ -1,6 +1,6 @@
// Extracted from the scopeguard crate
use core::{
mem,
mem::ManuallyDrop,
ops::{Deref, DerefMut},
ptr,
};
Expand Down Expand Up @@ -28,15 +28,13 @@ where
#[inline]
pub fn into_inner(guard: Self) -> T {
// Cannot move out of Drop-implementing types, so
// ptr::read the value and forget the guard.
// ptr::read the value out of a ManuallyDrop<Self>
// Don't use mem::forget as that might invalidate value
let guard = ManuallyDrop::new(guard);
unsafe {
let value = ptr::read(&guard.value);
// read the closure so that it is dropped, and assign it to a local
// variable to ensure that it is only dropped after the guard has
// been forgotten. (In case the Drop impl of the closure, or that
// of any consumed captured variable, panics).
let _dropfn = ptr::read(&guard.dropfn);
mem::forget(guard);
// read the closure so that it is dropped
let _ = ptr::read(&guard.dropfn);
value
}
}
Expand Down

0 comments on commit 2a7c322

Please sign in to comment.