!Unpin retags must still be reads, to check dereferenceable

RalfJung · RalfJung · commit f479404b12ad · 2022-11-27T00:03:07.000+01:00
also fix ICE on deallocation error and avoid redundant find_granting on retag
diff --git a/src/tools/miri/src/stacked_borrows/diagnostics.rs b/src/tools/miri/src/stacked_borrows/diagnostics.rs
@@ -353,10 +353,12 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
 
     /// 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,
@@ -374,8 +376,11 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
     /// 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}]",
@@ -428,14 +433,16 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
     }
 
     #[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,
+                "attempting 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)),
diff --git a/src/tools/miri/src/stacked_borrows/mod.rs b/src/tools/miri/src/stacked_borrows/mod.rs
@@ -392,7 +392,7 @@ impl<'tcx> Stack {
 
         // Step 1: Find granting item.
         let granting_idx =
-            self.find_granting(access, tag, exposed_tags).map_err(|_| dcx.access_error(self))?;
+            self.find_granting(access, tag, exposed_tags).map_err(|()| dcx.access_error(self))?;
 
         // Step 2: Remove incompatible items above them.  Make sure we do not remove protected
         // items.  Behavior differs for reads and writes.
@@ -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() {
@@ -489,39 +488,42 @@ impl<'tcx> Stack {
     }
 
     /// Derive a new pointer from one with the given tag.
-    /// `weak` controls whether this operation is weak or strong: weak granting does not act as
-    /// an access, and they add the new item directly on top of the one it is derived
-    /// from instead of all the way at the top of the stack.
-    /// `range` refers the entire operation, and `offset` refers to the specific location in
-    /// `range` that we are currently checking.
+    ///
+    /// `access` indicates which kind of memory access this retag itself should correspond to.
     fn grant(
         &mut self,
         derived_from: ProvenanceExtra,
         new: Item,
+        access: Option<AccessKind>,
         global: &GlobalStateInner,
         dcx: &mut DiagnosticCx<'_, '_, '_, 'tcx>,
         exposed_tags: &FxHashSet<SbTag>,
     ) -> InterpResult<'tcx> {
         dcx.start_grant(new.perm());
 
-        // Figure out which access `perm` corresponds to.
-        let access =
-            if new.perm().grants(AccessKind::Write) { AccessKind::Write } else { AccessKind::Read };
-
-        // Now 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.
-        let granting_idx = self
-            .find_granting(access, derived_from, exposed_tags)
-            .map_err(|_| dcx.grant_error(new.perm(), self))?;
-
         // Compute where to put the new item.
         // Either way, we ensure that we insert the new item in a way such that between
         // `derived_from` and the new one, there are only items *compatible with* `derived_from`.
-        let new_idx = if new.perm() == Permission::SharedReadWrite {
-            assert!(
-                access == AccessKind::Write,
-                "this case only makes sense for stack-like accesses"
-            );
+        let new_idx = if let Some(access) = access {
+            // 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`.
+            self.access(access, derived_from, global, dcx, exposed_tags)?;
+
+            // 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.
+            // This ensures U1 and F1.
+            self.len()
+        } else {
+            // The tricky case: creating a new SRW permission without actually being an access.
+            assert!(new.perm() == Permission::SharedReadWrite);
+
+            // 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.
+            let granting_idx = self
+                .find_granting(AccessKind::Write, derived_from, exposed_tags)
+                .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.
@@ -538,17 +540,6 @@ impl<'tcx> Stack {
             // be popped to (i.e., we insert it above all the write-compatible items).
             // This ensures F2b by adding the new item below any potentially existing `SharedReadOnly`.
             self.find_first_write_incompatible(granting_idx)
-        } else {
-            // A "safe" reborrow for a pointer that actually expects some aliasing guarantees.
-            // Here, creating a reference actually counts as an access.
-            // This ensures F2b for `Unique`, by removing offending `SharedReadOnly`.
-            self.access(access, derived_from, global, dcx, exposed_tags)?;
-
-            // 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.
-            // This ensures U1 and F1.
-            self.len()
         };
 
         // Put the new item there.
@@ -864,18 +855,22 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
         // Update the stacks.
         // Make sure that raw pointers and mutable shared references are reborrowed "weak":
         // There could be existing unique pointers reborrowed from them that should remain valid!
-        let perm = match kind {
-            RefKind::Unique { two_phase: false }
-                if place.layout.ty.is_unpin(*this.tcx, this.param_env()) =>
-            {
-                // Only if the type is unpin do we actually enforce uniqueness
-                Permission::Unique
+        let (perm, access) = match kind {
+            RefKind::Unique { two_phase } => {
+                // Permission is Unique only if the type is `Unpin` and this is not twophase
+                let perm = if !two_phase && place.layout.ty.is_unpin(*this.tcx, this.param_env()) {
+                    Permission::Unique
+                } else {
+                    Permission::SharedReadWrite
+                };
+                // We do an access for all full borrows, even if `!Unpin`.
+                let access = if !two_phase { Some(AccessKind::Write) } else { None };
+                (perm, access)
             }
-            RefKind::Unique { .. } => {
-                // Two-phase references and !Unpin references are treated as SharedReadWrite
-                Permission::SharedReadWrite
+            RefKind::Raw { mutable: true } => {
+                // Creating a raw ptr does not count as an access
+                (Permission::SharedReadWrite, None)
             }
-            RefKind::Raw { mutable: true } => Permission::SharedReadWrite,
             RefKind::Shared | RefKind::Raw { mutable: false } => {
                 // Shared references and *const are a whole different kind of game, the
                 // permission is not uniform across the entire range!
@@ -892,10 +887,13 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
                     // Adjust range.
                     range.start += base_offset;
                     // We are only ever `SharedReadOnly` inside the frozen bits.
-                    let perm = if frozen {
-                        Permission::SharedReadOnly
+                    let (perm, access) = if frozen {
+                        (Permission::SharedReadOnly, Some(AccessKind::Read))
                     } else {
-                        Permission::SharedReadWrite
+                        // Inside UnsafeCell, this does *not* count as an access, as there
+                        // might actually be mutable references further up the stack that
+                        // we have to keep alive.
+                        (Permission::SharedReadWrite, None)
                     };
                     let protected = if frozen {
                         protect.is_some()
@@ -914,7 +912,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, &global, dcx, exposed_tags)
+                        stack.grant(orig_tag, item, access, &global, dcx, exposed_tags)
                     })
                 })?;
                 return Ok(Some(alloc_id));
@@ -941,7 +939,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, &global, dcx, exposed_tags)
+            stack.grant(orig_tag, item, access, &global, dcx, exposed_tags)
         })?;
 
         Ok(Some(alloc_id))
diff --git a/src/tools/miri/src/stacked_borrows/stack.rs b/src/tools/miri/src/stacked_borrows/stack.rs
@@ -367,10 +367,10 @@ impl<'tcx> Stack {
 
     /// 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<V: FnMut(Item) -> crate::InterpResult<'tcx>>(
+    pub fn disable_uniques_starting_at(
         &mut self,
         disable_start: usize,
-        mut visitor: V,
+        mut visitor: impl FnMut(Item) -> crate::InterpResult<'tcx>,
     ) -> crate::InterpResult<'tcx> {
         #[cfg(feature = "stack-cache")]
         let unique_range = self.unique_range.clone();
diff --git a/src/tools/miri/tests/fail/stacked_borrows/illegal_dealloc1.rs b/src/tools/miri/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));
+    }
+}
diff --git a/src/tools/miri/tests/fail/stacked_borrows/illegal_dealloc1.stderr b/src/tools/miri/tests/fail/stacked_borrows/illegal_dealloc1.stderr
@@ -0,0 +1,30 @@
+error: Undefined Behavior: attempting 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()) }
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ attempting 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
+
diff --git a/src/tools/miri/tests/fail/stacked_borrows/notunpin_dereferenceable_fakeread.rs b/src/tools/miri/tests/fail/stacked_borrows/notunpin_dereferenceable_fakeread.rs
@@ -0,0 +1,17 @@
+//! Reborrowing a `&mut !Unpin` must still act like a (fake) read.
+use std::marker::PhantomPinned;
+
+struct NotUnpin(i32, PhantomPinned);
+
+fn main() {
+    unsafe {
+        let mut x = NotUnpin(0, PhantomPinned);
+        // Mutable borrow of `Unpin` field (with lifetime laundering)
+        let fieldref = &mut *(&mut x.0 as *mut i32);
+        // Mutable reborrow of the entire `x`, which is `!Unpin` but should
+        // still count as a read since we would add `dereferenceable`.
+        let _xref = &mut x;
+        // That read should have invalidated `fieldref`.
+        *fieldref = 0; //~ ERROR: /write access .* tag does not exist in the borrow stack/
+    }
+}
diff --git a/src/tools/miri/tests/fail/stacked_borrows/notunpin_dereferenceable_fakeread.stderr b/src/tools/miri/tests/fail/stacked_borrows/notunpin_dereferenceable_fakeread.stderr
@@ -0,0 +1,28 @@
+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]
+   |
+   = 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);
+   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+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;
+   |                     ^^^^^^
+   = note: BACKTRACE:
+   = note: inside `main` at $DIR/notunpin_dereferenceable_fakeread.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to previous error
+
