fix ICE on deallocation error and avoid redundant find_granting on retag

Author: RalfJung

src/stacked_borrows/diagnostics.rs

@@ -373,10 +373,12 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir
 
     /// Report a descriptive error when `new` could not be granted from `derived_from`.
     #[inline(never)] // This is only called on fatal code paths
-    pub(super) fn grant_error(&self, perm: Permission, stack: &Stack) -> InterpError<'tcx> {
+    pub(super) fn grant_error(&self, stack: &Stack) -> InterpError<'tcx> {
         let Operation::Retag(op) = &self.operation else {
             unreachable!("grant_error should only be called during a retag")
         };
+        let perm =
+            op.permission.expect("`start_grant` must be called before calling `grant_error`");
         let action = format!(
             "trying to retag from {:?} for {:?} permission at {:?}[{:#x}]",
             op.orig_tag,
@@ -394,8 +396,11 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir
     /// Report a descriptive error when `access` is not permitted based on `tag`.
     #[inline(never)] // This is only called on fatal code paths
     pub(super) fn access_error(&self, stack: &Stack) -> InterpError<'tcx> {
-        let Operation::Access(op) = &self.operation  else {
-            unreachable!("access_error should only be called during an access")
+        // Deallocation and retagging also do an access as part of their thing, so handle that here, too.
+        let op = match &self.operation {
+            Operation::Access(op) => op,
+            Operation::Retag(_) => return self.grant_error(stack),
+            Operation::Dealloc(_) => return self.dealloc_error(stack),
         };
         let action = format!(
             "attempting a {access} using {tag:?} at {alloc_id:?}[{offset:#x}]",
@@ -447,14 +452,16 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir
     }
 
     #[inline(never)] // This is only called on fatal code paths
-    pub fn dealloc_error(&self) -> InterpError<'tcx> {
+    pub fn dealloc_error(&self, stack: &Stack) -> InterpError<'tcx> {
         let Operation::Dealloc(op) = &self.operation else {
             unreachable!("dealloc_error should only be called during a deallocation")
         };
         err_sb_ub(
             format!(
-                "no item granting write access for deallocation to tag {:?} at {:?} found in borrow stack",
-                op.tag, self.history.id,
+                "attemtping deallocation using {tag:?} at {alloc_id:?}{cause}",
+                tag = op.tag,
+                alloc_id = self.history.id,
+                cause = error_cause(stack, op.tag),
             ),
             None,
             op.tag.and_then(|tag| self.get_logs_relevant_to(tag, None)),

src/stacked_borrows/mod.rs

@@ -476,8 +476,7 @@ impl<'tcx> Stack {
     ) -> InterpResult<'tcx> {
         // Step 1: Make a write access.
         // As part of this we do regular protector checking, i.e. even weakly protected items cause UB when popped.
-        self.access(AccessKind::Write, tag, global, dcx, exposed_tags)
-            .map_err(|_| dcx.dealloc_error())?;
+        self.access(AccessKind::Write, tag, global, dcx, exposed_tags)?;
 
         // Step 2: Pretend we remove the remaining items, checking if any are strongly protected.
         for idx in (0..self.len()).rev() {
@@ -509,10 +508,6 @@ impl<'tcx> Stack {
             // Simple case: We are just a regular memory access, and then push our thing on top,
             // like a regular stack.
             // This ensures F2b for `Unique`, by removing offending `SharedReadOnly`.
-            // FIXME: we are calling `find_granting` first since `access` will otherwise ICE in `dcx.access_error`...
-            let _granting_idx = self
-                .find_granting(access, derived_from, exposed_tags)
-                .map_err(|()| dcx.grant_error(new.perm(), self))?;
             self.access(access, derived_from, global, dcx, exposed_tags)?;
 
             // We insert "as far up as possible": We know only compatible items are remaining
@@ -528,7 +523,7 @@ impl<'tcx> Stack {
             // We use that to determine where to put the new item.
             let granting_idx = self
                 .find_granting(AccessKind::Write, derived_from, exposed_tags)
-                .map_err(|()| dcx.grant_error(new.perm(), self))?;
+                .map_err(|()| dcx.grant_error(self))?;
 
             let (Some(granting_idx), ProvenanceExtra::Concrete(_)) = (granting_idx, derived_from) else {
                 // The parent is a wildcard pointer or matched the unknown bottom.

tests/fail/stacked_borrows/illegal_dealloc1.rs

@@ -0,0 +1,14 @@
+//@error-pattern: /deallocation .* tag does not exist in the borrow stack/
+use std::alloc::{alloc, dealloc, Layout};
+
+fn main() {
+    unsafe {
+        let x = alloc(Layout::from_size_align_unchecked(1, 1));
+        let ptr1 = (&mut *x) as *mut u8;
+        let ptr2 = (&mut *ptr1) as *mut u8;
+        // Invalidate ptr2 by writing to ptr1.
+        ptr1.write(0);
+        // Deallocate through ptr2.
+        dealloc(ptr2, Layout::from_size_align_unchecked(1, 1));
+    }
+}

tests/fail/stacked_borrows/illegal_dealloc1.stderr

@@ -0,0 +1,30 @@
+error: Undefined Behavior: attemtping deallocation using <TAG> at ALLOC, but that tag does not exist in the borrow stack for this location
+  --> RUSTLIB/alloc/src/alloc.rs:LL:CC
+   |
+LL |     unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) }
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ attemtping deallocation using <TAG> at ALLOC, but that tag does not exist in the borrow stack for this location
+   |
+   = 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..0x1]
+  --> $DIR/illegal_deALLOC.rs:LL:CC
+   |
+LL |         let ptr2 = (&mut *ptr1) as *mut u8;
+   |                    ^^^^^^^^^^^^
+help: <TAG> was later invalidated at offsets [0x0..0x1] by a write access
+  --> $DIR/illegal_deALLOC.rs:LL:CC
+   |
+LL |         ptr1.write(0);
+   |         ^^^^^^^^^^^^^
+   = note: BACKTRACE:
+   = note: inside `std::alloc::dealloc` at RUSTLIB/alloc/src/alloc.rs:LL:CC
+note: inside `main` at $DIR/illegal_deALLOC.rs:LL:CC
+  --> $DIR/illegal_deALLOC.rs:LL:CC
+   |
+LL |         dealloc(ptr2, Layout::from_size_align_unchecked(1, 1));
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to previous error
+

tests/fail/stacked_borrows/notunpin_dereferenceable_fakeread.stderr

@@ -1,24 +1,24 @@
 error: Undefined Behavior: attempting a write access using <TAG> at ALLOC[0x0], but that tag does not exist in the borrow stack for this location
   --> $DIR/notunpin_dereferenceable_fakeread.rs:LL:CC
    |
-LL |     *fieldref = 0;
-   |     ^^^^^^^^^^^^^
-   |     |
-   |     attempting a write 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]
+LL |         *fieldref = 0;
+   |         ^^^^^^^^^^^^^
+   |         |
+   |         attempting a write 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 Unique retag at offsets [0x0..0x4]
   --> $DIR/notunpin_dereferenceable_fakeread.rs:LL:CC
    |
-LL |     let fieldref = &mut *(&mut x.0 as *mut i32);
-   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL |         let fieldref = &mut *(&mut x.0 as *mut i32);
+   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 help: <TAG> was later invalidated at offsets [0x0..0x4] by a SharedReadWrite retag
   --> $DIR/notunpin_dereferenceable_fakeread.rs:LL:CC
    |
-LL |     let _xref = &mut x;
-   |                 ^^^^^^
+LL |         let _xref = &mut x;
+   |                     ^^^^^^
    = note: BACKTRACE:
    = note: inside `main` at $DIR/notunpin_dereferenceable_fakeread.rs:LL:CC