justify return-position noalias: make Box clear stack on some retags

Author: RalfJung

compiler/rustc_middle/src/mir/mod.rs

@@ -1444,6 +1444,7 @@ impl Debug for Statement<'_> {
                 "Retag({}{:?})",
                 match kind {
                     RetagKind::FnEntry => "[fn entry] ",
+                    RetagKind::FnReturn => "[fn return] ",
                     RetagKind::TwoPhase => "[2phase] ",
                     RetagKind::Raw => "[raw] ",
                     RetagKind::Default => "",

compiler/rustc_middle/src/mir/syntax.rs

@@ -323,7 +323,7 @@ pub enum StatementKind<'tcx> {
     /// For code that is not specific to stacked borrows, you should consider retags to read and
     /// modify the place in an opaque way.
     ///
-    /// Only `RetagKind::Default` and `RetagKind::FnEntry` are permitted.
+    /// Only `RetagKind::Default`, `RetagKind::FnEntry`, `RetagKind::FnReturn` are permitted.
     Retag(RetagKind, Box<Place<'tcx>>),
 
     /// Encodes a user's type ascription. These need to be preserved
@@ -412,6 +412,8 @@ impl std::fmt::Display for NonDivergingIntrinsic<'_> {
 pub enum RetagKind {
     /// The initial retag of arguments when entering a function.
     FnEntry,
+    /// The retag of a value that was just returned from another function.
+    FnReturn,
     /// Retag preparing for a two-phase borrow.
     TwoPhase,
     /// Retagging raw pointers.

compiler/rustc_mir_transform/src/add_retag.rs

@@ -112,7 +112,7 @@ impl<'tcx> MirPass<'tcx> for AddRetag {
                 0,
                 Statement {
                     source_info,
-                    kind: StatementKind::Retag(RetagKind::Default, Box::new(dest_place)),
+                    kind: StatementKind::Retag(RetagKind::FnReturn, Box::new(dest_place)),
                 },
             );
         }

src/test/mir-opt/retag.main.SimplifyCfg-elaborate-drops.after.mir

@@ -76,7 +76,7 @@ fn main() -> () {
     }
 
     bb1: {
-        Retag(_3);                       // scope 1 at $DIR/retag.rs:+3:17: +3:36
+        Retag([fn return] _3);           // scope 1 at $DIR/retag.rs:+3:17: +3:36
         StorageDead(_6);                 // scope 1 at $DIR/retag.rs:+3:35: +3:36
         StorageDead(_4);                 // scope 1 at $DIR/retag.rs:+3:35: +3:36
         StorageDead(_7);                 // scope 1 at $DIR/retag.rs:+3:36: +3:37
@@ -122,7 +122,7 @@ fn main() -> () {
     }
 
     bb3: {
-        Retag(_15);                      // scope 6 at $DIR/retag.rs:+15:14: +15:19
+        Retag([fn return] _15);          // scope 6 at $DIR/retag.rs:+15:14: +15:19
         StorageDead(_17);                // scope 6 at $DIR/retag.rs:+15:18: +15:19
         StorageDead(_16);                // scope 6 at $DIR/retag.rs:+15:18: +15:19
         StorageDead(_18);                // scope 6 at $DIR/retag.rs:+15:19: +15:20
@@ -148,7 +148,7 @@ fn main() -> () {
     }
 
     bb4: {
-        Retag(_19);                      // scope 7 at $DIR/retag.rs:+18:5: +18:24
+        Retag([fn return] _19);          // scope 7 at $DIR/retag.rs:+18:5: +18:24
         StorageDead(_22);                // scope 7 at $DIR/retag.rs:+18:23: +18:24
         StorageDead(_20);                // scope 7 at $DIR/retag.rs:+18:23: +18:24
         StorageDead(_23);                // scope 7 at $DIR/retag.rs:+18:24: +18:25

src/tools/miri/src/borrow_tracker/mod.rs

@@ -87,7 +87,7 @@ pub struct GlobalStateInner {
     /// Table storing the "base" tag for each allocation.
     /// The base tag is the one used for the initial pointer.
     /// We need this in a separate table to handle cyclic statics.
-    pub base_ptr_tags: FxHashMap<AllocId, BorTag>,
+    base_ptr_tags: FxHashMap<AllocId, BorTag>,
     /// Next unused call ID (for protectors).
     pub next_call_id: CallId,
     /// All currently protected tags.

src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs

@@ -193,7 +193,7 @@ struct RetagOp {
 #[derive(Debug, Clone, Copy, PartialEq)]
 pub enum RetagCause {
     Normal,
-    FnReturn,
+    FnReturnPlace,
     FnEntry,
     TwoPhase,
 }
@@ -496,7 +496,7 @@ impl RetagCause {
         match self {
             RetagCause::Normal => "retag",
             RetagCause::FnEntry => "FnEntry retag",
-            RetagCause::FnReturn => "FnReturn retag",
+            RetagCause::FnReturnPlace => "return-place retag",
             RetagCause::TwoPhase => "two-phase retag",
         }
         .to_string()

src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs

@@ -48,6 +48,8 @@ pub struct Stacks {
 enum NewPermission {
     Uniform {
         perm: Permission,
+        /// If true, indicates that the stack should be cleared of all other tags.
+        clear: bool,
         access: Option<AccessKind>,
         protector: Option<ProtectorKind>,
     },
@@ -77,19 +79,22 @@ impl NewPermission {
                     assert!(protector.is_none()); // RetagKind can't be both FnEntry and TwoPhase.
                     NewPermission::Uniform {
                         perm: Permission::SharedReadWrite,
+                        clear: false,
                         access: None,
                         protector: None,
                     }
                 } else if pointee.is_unpin(*cx.tcx, cx.param_env()) {
                     // A regular full mutable reference.
                     NewPermission::Uniform {
                         perm: Permission::Unique,
+                        clear: false,
                         access: Some(AccessKind::Write),
                         protector,
                     }
                 } else {
                     NewPermission::Uniform {
                         perm: Permission::SharedReadWrite,
+                        clear: false,
                         // FIXME: We emit `dereferenceable` for `!Unpin` mutable references, so we
                         // should do fake accesses here. But then we run into
                         // <https://github.com/rust-lang/unsafe-code-guidelines/issues/381>, so for now
@@ -104,6 +109,7 @@ impl NewPermission {
                 // Mutable raw pointer. No access, not protected.
                 NewPermission::Uniform {
                     perm: Permission::SharedReadWrite,
+                    clear: false,
                     access: None,
                     protector: None,
                 }
@@ -371,11 +377,13 @@ impl<'tcx> Stack {
 
     /// Derive a new pointer from one with the given tag.
     ///
+    /// `clear` indicates whether everything else should be removed from the stack.
     /// `access` indicates which kind of memory access this retag itself should correspond to.
     fn grant(
         &mut self,
         derived_from: ProvenanceExtra,
         new: Item,
+        clear: bool,
         access: Option<AccessKind>,
         global: &GlobalStateInner,
         dcx: &mut DiagnosticCx<'_, '_, '_, 'tcx>,
@@ -392,6 +400,15 @@ impl<'tcx> Stack {
             // This ensures F2b for `Unique`, by removing offending `SharedReadOnly`.
             self.access(access, derived_from, global, dcx, exposed_tags)?;
 
+            if clear {
+                // Remove all items, also checking their protectors.
+                for idx in (0..self.len()).rev() {
+                    let item = self.get(idx).unwrap();
+                    Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Conflict)?;
+                }
+                self.clear();
+            }
+
             // We insert "as far up as possible": We know only compatible items are remaining
             // on top of `derived_from`, and we want the new item at the top so that we
             // get the strongest possible guarantees.
@@ -400,6 +417,7 @@ impl<'tcx> Stack {
         } else {
             // The tricky case: creating a new SRW permission without actually being an access.
             assert!(new.perm() == Permission::SharedReadWrite);
+            assert!(!clear);
 
             // First we figure out which item grants our parent (`derived_from`) this kind of access.
             // We use that to determine where to put the new item.
@@ -723,7 +741,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
 
         // Update the stacks, according to the new permission information we are given.
         match new_perm {
-            NewPermission::Uniform { perm, access, protector } => {
+            NewPermission::Uniform { perm, clear, access, protector } => {
                 assert!(perm != Permission::SharedReadOnly);
                 // Here we can avoid `borrow()` calls because we have mutable references.
                 // Note that this asserts that the allocation is mutable -- but since we are creating a
@@ -741,7 +759,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
                     alloc_range(base_offset, size),
                 );
                 stacked_borrows.for_each(range, dcx, |stack, dcx, exposed_tags| {
-                    stack.grant(orig_tag, item, access, &global, dcx, exposed_tags)
+                    stack.grant(orig_tag, item, clear, access, &global, dcx, exposed_tags)
                 })?;
                 drop(global);
                 if let Some(access) = access {
@@ -784,7 +802,8 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
                         alloc_range(base_offset, size),
                     );
                     stacked_borrows.for_each(range, dcx, |stack, dcx, exposed_tags| {
-                        stack.grant(orig_tag, item, access, &global, dcx, exposed_tags)
+                        let clear = false;
+                        stack.grant(orig_tag, item, clear, access, &global, dcx, exposed_tags)
                     })?;
                     drop(global);
                     if let Some(access) = access {
@@ -862,7 +881,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let new_perm = NewPermission::from_ref_ty(val.layout.ty, kind, this);
         let retag_cause = match kind {
             RetagKind::TwoPhase { .. } => RetagCause::TwoPhase,
-            RetagKind::FnEntry => unreachable!(),
+            RetagKind::FnEntry | RetagKind::FnReturn => unreachable!(),
             RetagKind::Raw | RetagKind::Default => RetagCause::Normal,
         };
         this.sb_retag_reference(val, new_perm, retag_cause)
@@ -878,7 +897,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let retag_cause = match kind {
             RetagKind::Raw | RetagKind::TwoPhase { .. } => unreachable!(), // these can only happen in `retag_ptr_value`
             RetagKind::FnEntry => RetagCause::FnEntry,
-            RetagKind::Default => RetagCause::Normal,
+            RetagKind::Default | RetagKind::FnReturn => RetagCause::Normal,
         };
         let mut visitor = RetagVisitor { ecx: this, kind, retag_cause, retag_fields };
         return visitor.visit_value(place);
@@ -915,12 +934,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
             }
 
             fn visit_box(&mut self, place: &PlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> {
-                // Boxes get a weak protectors, since they may be deallocated.
+                // We cannot protect boxes since they might get invalidated if passed to an `id`
+                // function (due to the return-position `noalias`). However, doing a `clear` on
+                // `FnEntry` also has the effect that using any other pointer to access this memory
+                // is *immediate* UB.
+                // FIXME: should we `clear` on *all* `Box` retags? Currently (2022-12-27) this leads
+                // to UB somewhere in thread-local dtor handling.
                 let new_perm = NewPermission::Uniform {
                     perm: Permission::Unique,
+                    clear: matches!(self.kind, RetagKind::FnEntry | RetagKind::FnReturn),
                     access: Some(AccessKind::Write),
-                    protector: (self.kind == RetagKind::FnEntry)
-                        .then_some(ProtectorKind::WeakProtector),
+                    protector: None,
                 };
                 self.retag_ptr_inplace(place, new_perm, self.retag_cause)
             }
@@ -995,10 +1019,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         // Reborrow it. With protection! That is part of the point.
         let new_perm = NewPermission::Uniform {
             perm: Permission::Unique,
+            clear: false,
             access: Some(AccessKind::Write),
             protector: Some(ProtectorKind::StrongProtector),
         };
-        let val = this.sb_retag_reference(&val, new_perm, RetagCause::FnReturn)?;
+        let val = this.sb_retag_reference(&val, new_perm, RetagCause::FnReturnPlace)?;
         // And use reborrowed pointer for return place.
         let return_place = this.ref_to_mplace(&val)?;
         this.frame_mut().return_place = return_place.into();

src/tools/miri/src/borrow_tracker/stacked_borrows/stack.rs

@@ -252,8 +252,8 @@ impl<'tcx> Stack {
         // and this check actually ensures we do not access an invalid cache.
         // When a stack is created and when items are removed from the top of the borrow stack, we
         // need some valid value to populate the cache. In both cases, we try to use the bottom
-        // item. But when the stack is cleared in `set_unknown_bottom` there is nothing we could
-        // place in the cache that is correct. But due to the way we populate the cache in
+        // item. But when the stack is cleared in `clear` or `set_unknown_bottom` there is nothing
+        // we could place in the cache that is correct. But due to the way we populate the cache in
         // `StackCache::add`, we know that when the borrow stack has grown larger than the cache,
         // every slot in the cache is valid.
         if self.borrows.len() <= CACHE_LEN {
@@ -368,6 +368,19 @@ impl<'tcx> Stack {
         }
     }
 
+    // Empty the stack.
+    pub fn clear(&mut self) {
+        // We clear the borrow stack but the lookup cache doesn't support clearing per se. Instead,
+        // there is a check explained in `find_granting_cache` which protects against accessing the
+        // cache when it has been cleared and not yet refilled.
+        self.borrows.clear();
+        self.unknown_bottom = None;
+        #[cfg(feature = "stack-cache")]
+        {
+            self.unique_range = 0..0;
+        }
+    }
+
     /// Find all `Unique` elements in this borrow stack above `granting_idx`, pass a copy of them
     /// to the `visitor`, then set their `Permission` to `Disabled`.
     pub fn disable_uniques_starting_at(

src/tools/miri/tests/fail/stacked_borrows/box_noalias_violation.rs

@@ -2,13 +2,15 @@ unsafe fn test(mut x: Box<i32>, y: *const i32) -> i32 {
     // We will call this in a way that x and y alias.
     *x = 5;
     std::mem::forget(x);
-    *y //~ERROR: weakly protected
+    *y //~ERROR: does not exist in the borrow stack
 }
 
 fn main() {
     unsafe {
         let mut v = 42;
         let ptr = &mut v as *mut i32;
-        test(Box::from_raw(ptr), ptr);
+        let b = Box::from_raw(ptr);
+        let ptr = &*b as *const i32;
+        test(b, ptr);
     }
 }

src/tools/miri/tests/fail/stacked_borrows/box_noalias_violation.stderr

@@ -1,28 +1,31 @@
-error: Undefined Behavior: not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is weakly protected because it is an argument of call ID
+error: Undefined Behavior: attempting a read access using <TAG> at ALLOC[0x0], but that tag does not exist in the borrow stack for this location
   --> $DIR/box_noalias_violation.rs:LL:CC
    |
 LL |     *y
-   |     ^^ not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is weakly protected because it is an argument of call ID
+   |     ^^
+   |     |
+   |     attempting a read access using <TAG> at ALLOC[0x0], but that tag does not exist in the borrow stack for this location
+   |     this error occurs as part of an access at ALLOC[0x0..0x4]
    |
    = 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: <TAG> was created by a SharedReadWrite retag at offsets [0x0..0x4]
+help: <TAG> was created by a SharedReadOnly retag at offsets [0x0..0x4]
   --> $DIR/box_noalias_violation.rs:LL:CC
    |
-LL |         let ptr = &mut v as *mut i32;
-   |                   ^^^^^^
-help: <TAG> is this argument
+LL |         let ptr = &*b as *const i32;
+   |                   ^^^
+help: <TAG> was later invalidated at offsets [0x0..0x4] by a Unique retag
   --> $DIR/box_noalias_violation.rs:LL:CC
    |
-LL | unsafe fn test(mut x: Box<i32>, y: *const i32) -> i32 {
-   |                ^^^^^
+LL |         test(b, ptr);
+   |              ^
    = note: BACKTRACE (of the first span):
    = note: inside `test` at $DIR/box_noalias_violation.rs:LL:CC
 note: inside `main`
   --> $DIR/box_noalias_violation.rs:LL:CC
    |
-LL |         test(Box::from_raw(ptr), ptr);
-   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL |         test(b, ptr);
+   |         ^^^^^^^^^^^^
 
 note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
 

src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.rs

@@ -1,4 +1,5 @@
 //@error-pattern: /deallocating while item \[Unique for .*\] is strongly protected/
+use std::alloc::{dealloc, Layout};
 
 fn inner(x: &mut i32, f: fn(&mut i32)) {
     // `f` may mutate, but it may not deallocate!
@@ -7,7 +8,7 @@ fn inner(x: &mut i32, f: fn(&mut i32)) {
 
 fn main() {
     inner(Box::leak(Box::new(0)), |x| {
-        let raw = x as *mut _;
-        drop(unsafe { Box::from_raw(raw) });
+        let raw = x as *mut i32 as *mut u8;
+        drop(unsafe { dealloc(raw, Layout::new::<i32>()) });
     });
 }

src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.stderr

@@ -8,15 +8,11 @@ LL |     unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) }
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
    = note: BACKTRACE:
    = note: inside `std::alloc::dealloc` at RUSTLIB/alloc/src/alloc.rs:LL:CC
-   = note: inside `<std::alloc::Global as std::alloc::Allocator>::deallocate` at RUSTLIB/alloc/src/alloc.rs:LL:CC
-   = note: inside `alloc::alloc::box_free::<i32, std::alloc::Global>` at RUSTLIB/alloc/src/alloc.rs:LL:CC
-   = note: inside `std::ptr::drop_in_place::<std::boxed::Box<i32>> - shim(Some(std::boxed::Box<i32>))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC
-   = note: inside `std::mem::drop::<std::boxed::Box<i32>>` at RUSTLIB/core/src/mem/mod.rs:LL:CC
 note: inside closure
   --> $DIR/deallocate_against_protector1.rs:LL:CC
    |
-LL |         drop(unsafe { Box::from_raw(raw) });
-   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL |         drop(unsafe { dealloc(raw, Layout::new::<i32>()) });
+   |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: inside `<[closure@$DIR/deallocate_against_protector1.rs:LL:CC] as std::ops::FnOnce<(&mut i32,)>>::call_once - shim` at RUSTLIB/core/src/ops/function.rs:LL:CC
 note: inside `inner`
   --> $DIR/deallocate_against_protector1.rs:LL:CC
@@ -27,8 +23,8 @@ note: inside `main`
   --> $DIR/deallocate_against_protector1.rs:LL:CC
    |
 LL | /     inner(Box::leak(Box::new(0)), |x| {
-LL | |         let raw = x as *mut _;
-LL | |         drop(unsafe { Box::from_raw(raw) });
+LL | |         let raw = x as *mut i32 as *mut u8;
+LL | |         drop(unsafe { dealloc(raw, Layout::new::<i32>()) });
 LL | |     });
    | |______^
 

src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.rs

@@ -1,4 +1,5 @@
 //@error-pattern: /deallocating while item \[SharedReadWrite for .*\] is strongly protected/
+use std::alloc::{dealloc, Layout};
 use std::marker::PhantomPinned;
 
 pub struct NotUnpin(i32, PhantomPinned);
@@ -10,7 +11,7 @@ fn inner(x: &mut NotUnpin, f: fn(&mut NotUnpin)) {
 
 fn main() {
     inner(Box::leak(Box::new(NotUnpin(0, PhantomPinned))), |x| {
-        let raw = x as *mut _;
-        drop(unsafe { Box::from_raw(raw) });
+        let raw = x as *mut NotUnpin as *mut u8;
+        drop(unsafe { dealloc(raw, Layout::new::<NotUnpin>()) });
     });
 }