Merge pull request #4947 from quiode/tb-strong-mode

tree borrows: implicit writes

Author: RalfJung

src/tools/miri/README.md

@@ -511,6 +511,8 @@ to Miri failing to detect cases of undefined behavior in a program.
   of Rust will be stricter than Tree Borrows. In other words, if you use Tree Borrows,
   even if your code is accepted today, it might be declared UB in the future.
   This is much less likely with Stacked Borrows.
+* `-Zmiri-tree-borrows-implicit-writes` enables implicit writes for all `&mut` function arguments.
+  This makes Tree Borrows less permissive.
 * `-Zmiri-tree-borrows-no-precise-interior-mut` makes Tree Borrows
   track interior mutable data on the level of references instead of on the
   byte-level as is done by default.  Therefore, with this flag, Tree

src/tools/miri/src/bin/miri.rs

@@ -515,6 +515,7 @@ fn main() -> ExitCode {
             miri_config.borrow_tracker =
                 Some(BorrowTrackerMethod::TreeBorrows(TreeBorrowsParams {
                     precise_interior_mut: true,
+                    implicit_writes: false,
                 }));
         } else if arg == "-Zmiri-tree-borrows-no-precise-interior-mut" {
             match &mut miri_config.borrow_tracker {
@@ -526,6 +527,16 @@ fn main() -> ExitCode {
                         "`-Zmiri-tree-borrows` is required before `-Zmiri-tree-borrows-no-precise-interior-mut`"
                     ),
             };
+        } else if arg == "-Zmiri-tree-borrows-implicit-writes" {
+            match &mut miri_config.borrow_tracker {
+                Some(BorrowTrackerMethod::TreeBorrows(params)) => {
+                    params.implicit_writes = true;
+                }
+                _ =>
+                    fatal_error!(
+                        "`-Zmiri-tree-borrows` is required before `-Zmiri-tree-borrows-implicit-writes`"
+                    ),
+            };
         } else if arg == "-Zmiri-disable-data-race-detector" {
             miri_config.data_race_detector = false;
             miri_config.weak_memory_emulation = false;

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

@@ -224,6 +224,8 @@ pub enum BorrowTrackerMethod {
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub struct TreeBorrowsParams {
     pub precise_interior_mut: bool,
+    /// Controls whether `&mut` function arguments are immediately activated with an implicit write.
+    pub implicit_writes: bool,
 }
 
 impl BorrowTrackerMethod {

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

@@ -15,7 +15,7 @@ use crate::*;
 #[derive(Clone, Copy, Debug)]
 pub enum AccessCause {
     Explicit(AccessKind),
-    Reborrow,
+    Reborrow(AccessKind),
     Dealloc,
     FnExit(AccessKind),
 }
@@ -24,7 +24,7 @@ impl fmt::Display for AccessCause {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
             Self::Explicit(kind) => write!(f, "{kind}"),
-            Self::Reborrow => write!(f, "reborrow"),
+            Self::Reborrow(_) => write!(f, "reborrow"),
             Self::Dealloc => write!(f, "deallocation"),
             // This is dead code, since the protector release access itself can never
             // cause UB (while the protector is active, if some other access invalidates
@@ -40,7 +40,7 @@ impl AccessCause {
         let rel = if is_foreign { "foreign" } else { "child" };
         match self {
             Self::Explicit(kind) => format!("{rel} {kind}"),
-            Self::Reborrow => format!("reborrow (acting as a {rel} read access)"),
+            Self::Reborrow(kind) => format!("reborrow (acting as a {rel} {kind})"),
             Self::Dealloc => format!("deallocation (acting as a {rel} write access)"),
             Self::FnExit(kind) => format!("protector release (acting as a {rel} {kind})"),
         }

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

@@ -6,7 +6,7 @@ use rustc_middle::ty::{self, Ty};
 use self::foreign_access_skipping::IdempotentForeignAccess;
 use self::tree::LocationState;
 use crate::borrow_tracker::{AccessKind, GlobalState, GlobalStateInner, ProtectorKind};
-use crate::concurrency::data_race::NaReadType;
+use crate::concurrency::data_race::{NaReadType, NaWriteType};
 use crate::*;
 
 pub mod diagnostics;
@@ -109,13 +109,8 @@ impl<'tcx> Tree {
 pub struct NewPermission {
     /// Permission for the frozen part of the range.
     freeze_perm: Permission,
-    /// Whether a read access should be performed on the frozen part on a retag.
-    freeze_access: bool,
     /// Permission for the non-frozen part of the range.
     nonfreeze_perm: Permission,
-    /// Whether a read access should be performed on the non-frozen
-    /// part on a retag.
-    nonfreeze_access: bool,
     /// Permission for memory outside the range.
     outside_perm: Permission,
     /// Whether this pointer is part of the arguments of a function call.
@@ -138,36 +133,78 @@ impl<'tcx> NewPermission {
         let ty_is_freeze = pointee.is_freeze(*cx.tcx, cx.typing_env());
         let is_protected = retag_kind == RetagKind::FnEntry;
 
+        // Check if the implicit writes check has been enabled for this function using the `-Zmiri-tree-borrows-implicit-writes` flag
+        let implicit_writes = cx
+            .machine
+            .borrow_tracker
+            .as_ref()
+            .unwrap()
+            .borrow()
+            .borrow_tracker_method
+            .get_tree_borrows_params()
+            .implicit_writes;
+
         if matches!(ref_mutability, Some(Mutability::Mut) | None if !ty_is_unpin) {
             // Mutable reference / Box to pinning type: retagging is a NOP.
             // FIXME: with `UnsafePinned`, this should do proper per-byte tracking.
             return None;
         }
 
-        let freeze_perm = match ref_mutability {
-            // Shared references are frozen.
-            Some(Mutability::Not) => Permission::new_frozen(),
-            // Mutable references and Boxes are reserved.
-            _ => Permission::new_reserved_frz(),
-        };
-        let nonfreeze_perm = match ref_mutability {
-            // Shared references are "transparent".
-            Some(Mutability::Not) => Permission::new_cell(),
-            // *Protected* mutable references and boxes are reserved without regarding for interior mutability.
-            _ if is_protected => Permission::new_reserved_frz(),
-            // Unprotected mutable references and boxes start in `ReservedIm`.
-            _ => Permission::new_reserved_im(),
+        enum Part {
+            InsideFrozen,
+            InsideUnsafeCell,
+            Outside,
+        }
+        use Part::*;
+
+        let perm = |part: Part| {
+            // Whether we should consider this byte to be frozen.
+            // Outside bytes are frozen only if the entire type is frozen.
+            let frozen = match part {
+                InsideFrozen => true,
+                InsideUnsafeCell => false,
+                Outside => ty_is_freeze,
+            };
+            match ref_mutability {
+                // Shared references
+                Some(Mutability::Not) =>
+                    if frozen {
+                        Permission::new_frozen()
+                    } else {
+                        Permission::new_cell()
+                    },
+                // Mutable references
+                Some(Mutability::Mut) => {
+                    if is_protected && implicit_writes && !matches!(part, Outside) {
+                        // We cannot use `Unique` for the outside part.
+                        Permission::new_unique()
+                    } else if is_protected || frozen {
+                        // We also use this for protected `&mut UnsafeCell` as otherwise adding
+                        // `noalias` would not be sound.
+                        Permission::new_reserved_frz()
+                    } else {
+                        Permission::new_reserved_im()
+                    }
+                }
+                // Boxes
+                None =>
+                    if is_protected && implicit_writes && !matches!(part, Outside) {
+                        // Boxes are treated the same as mutable references.
+                        Permission::new_unique()
+                    } else if is_protected || frozen {
+                        // We also use this for protected `Box<UnsafeCell>` as otherwise adding
+                        // `noalias` would not be sound.
+                        Permission::new_reserved_frz()
+                    } else {
+                        Permission::new_reserved_im()
+                    },
+            }
         };
 
-        // Everything except for `Cell` gets an initial access.
-        let initial_access = |perm: &Permission| !perm.is_cell();
-
         Some(NewPermission {
-            freeze_perm,
-            freeze_access: initial_access(&freeze_perm),
-            nonfreeze_perm,
-            nonfreeze_access: initial_access(&nonfreeze_perm),
-            outside_perm: if ty_is_freeze { freeze_perm } else { nonfreeze_perm },
+            freeze_perm: perm(InsideFrozen),
+            nonfreeze_perm: perm(InsideUnsafeCell),
+            outside_perm: perm(Outside),
             protector: is_protected.then_some(if ref_mutability.is_some() {
                 // Strong protector for references
                 ProtectorKind::StrongProtector
@@ -288,13 +325,10 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         // Compute initial "inside" permissions.
         let loc_state = |frozen: bool| -> LocationState {
-            let (perm, access) = if frozen {
-                (new_perm.freeze_perm, new_perm.freeze_access)
-            } else {
-                (new_perm.nonfreeze_perm, new_perm.nonfreeze_access)
-            };
+            let perm = if frozen { new_perm.freeze_perm } else { new_perm.nonfreeze_perm };
             let sifa = perm.strongest_idempotent_foreign_access(protected);
-            if access {
+
+            if perm.associated_access().is_some() {
                 LocationState::new_accessed(perm, sifa)
             } else {
                 LocationState::new_non_accessed(perm, sifa)
@@ -303,11 +337,10 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let inside_perms = if !precise_interior_mut {
             // For `!Freeze` types, just pretend the entire thing is an `UnsafeCell`.
             let ty_is_freeze = place.layout.ty.is_freeze(*this.tcx, this.typing_env());
-            let state = loc_state(ty_is_freeze);
-            DedupRangeMap::new(ptr_size, state)
+            DedupRangeMap::new(ptr_size, loc_state(ty_is_freeze))
         } else {
             // The initial state will be overwritten by the visitor below.
-            let mut perms_map: DedupRangeMap<LocationState> = DedupRangeMap::new(
+            let mut perms_map = DedupRangeMap::new(
                 ptr_size,
                 LocationState::new_accessed(
                     Permission::new_disabled(),
@@ -327,9 +360,18 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let alloc_extra = this.get_alloc_extra(alloc_id)?;
         let mut tree_borrows = alloc_extra.borrow_tracker_tb().borrow_mut();
 
-        for (perm_range, perm) in inside_perms.iter_all() {
-            if perm.accessed() {
-                // Some reborrows incur a read access to the parent.
+        for (perm_range, loc_state) in inside_perms.iter_all() {
+            if let Some(access) = loc_state.permission().associated_access() {
+                // Some reborrows incur a read/write access to the parent.
+                // As a write also implies a read, a single write is performed instead of a read and a write.
+
+                // writing to an immutable allocation (static variables) is UB, check this here
+                if access == AccessKind::Write
+                    && this.get_alloc_mutability(alloc_id).unwrap().is_not()
+                {
+                    throw_ub!(WriteToReadOnly(alloc_id))
+                }
+
                 // Adjust range to be relative to allocation start (rather than to `place`).
                 let range_in_alloc = AllocRange {
                     start: Size::from_bytes(perm_range.start) + base_offset,
@@ -339,8 +381,8 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 tree_borrows.perform_access(
                     parent_prov,
                     range_in_alloc,
-                    AccessKind::Read,
-                    diagnostics::AccessCause::Reborrow,
+                    access,
+                    diagnostics::AccessCause::Reborrow(access),
                     this.machine.borrow_tracker.as_ref().unwrap(),
                     alloc_id,
                     this.machine.current_user_relevant_span(),
@@ -349,17 +391,29 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 // Also inform the data race model (but only if any bytes are actually affected).
                 if range_in_alloc.size.bytes() > 0 {
                     if let Some(data_race) = alloc_extra.data_race.as_vclocks_ref() {
-                        data_race.read_non_atomic(
-                            alloc_id,
-                            range_in_alloc,
-                            NaReadType::Retag,
-                            Some(place.layout.ty),
-                            &this.machine,
-                        )?
+                        match access {
+                            AccessKind::Read =>
+                                data_race.read_non_atomic(
+                                    alloc_id,
+                                    range_in_alloc,
+                                    NaReadType::Retag,
+                                    Some(place.layout.ty),
+                                    &this.machine,
+                                )?,
+                            AccessKind::Write =>
+                                data_race.write_non_atomic(
+                                    alloc_id,
+                                    range_in_alloc,
+                                    NaWriteType::Retag,
+                                    Some(place.layout.ty),
+                                    &this.machine,
+                                )?,
+                        };
                     }
                 }
             }
         }
+
         // Record the parent-child pair in the tree.
         tree_borrows.new_child(
             base_offset,
@@ -370,7 +424,6 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             protected,
             this.machine.current_user_relevant_span(),
         )?;
-        drop(tree_borrows);
 
         interp_ok(Some(new_prov))
     }
@@ -550,9 +603,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             // argument doesn't matter
             // (`ty_is_freeze || true` in `new_reserved` will always be `true`).
             freeze_perm: Permission::new_reserved_frz(),
-            freeze_access: true,
             nonfreeze_perm: Permission::new_reserved_frz(),
-            nonfreeze_access: true,
             outside_perm: Permission::new_reserved_frz(),
             protector: Some(ProtectorKind::StrongProtector),
         };

src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs

@@ -91,7 +91,7 @@ impl PartialOrd for PermissionPriv {
 impl PermissionPriv {
     /// Check if `self` can be the initial state of a pointer.
     fn is_initial(&self) -> bool {
-        matches!(self, ReservedFrz { conflicted: false } | Frozen | ReservedIM | Cell)
+        matches!(self, ReservedFrz { conflicted: false } | Frozen | ReservedIM | Cell | Unique)
     }
 
     /// Reject `ReservedIM` that cannot exist in the presence of a protector.
@@ -265,14 +265,17 @@ impl Permission {
         self.inner == Cell
     }
 
-    /// Default initial permission of the root of a new tree at inbounds positions.
-    /// Must *only* be used for the root, this is not in general an "initial" permission!
+    /// Check if `self` is a Permission of type `Unique`
+    pub fn is_unique(&self) -> bool {
+        self.inner == Unique
+    }
+
+    /// Create a new Permission of type `Unique`
     pub fn new_unique() -> Self {
         Self { inner: Unique }
     }
 
-    /// Default initial permission of a reborrowed mutable reference that is either
-    /// protected or not interior mutable.
+    /// Create a new Permission of type `ReservedFrz` with conflictedReserved set to false
     pub fn new_reserved_frz() -> Self {
         Self { inner: ReservedFrz { conflicted: false } }
     }
@@ -304,8 +307,8 @@ impl Permission {
         self.inner.compatible_with_protector()
     }
 
-    /// What kind of access to perform before releasing the protector.
-    pub fn protector_end_access(&self) -> Option<AccessKind> {
+    /// What kind of access to perform before releasing the protector or on a reborrow.
+    pub fn associated_access(&self) -> Option<AccessKind> {
         match self.inner {
             // Do not do perform access if it is a `Cell`, as this
             // can cause data races when using thread-safe data types.

src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs

@@ -63,6 +63,7 @@ impl LocationState {
     /// `sifa` is the (strongest) idempotent foreign access, see `foreign_access_skipping.rs`
     pub fn new_non_accessed(permission: Permission, sifa: IdempotentForeignAccess) -> Self {
         assert!(permission.is_initial() || permission.is_disabled());
+        assert!(!permission.is_unique());
         Self { permission, accessed: false, idempotent_foreign_access: sifa }
     }
 
@@ -73,6 +74,12 @@ impl LocationState {
         Self { permission, accessed: true, idempotent_foreign_access: sifa }
     }
 
+    /// Checks whether the current location state is ever reachable in a real execution.
+    pub fn possible(&self) -> bool {
+        // `Unique` can only be reached on actually accessed locations.
+        self.accessed || !self.permission.is_unique()
+    }
+
     /// Check if the location has been accessed, i.e. if it has
     /// ever been accessed through a child pointer.
     pub fn accessed(&self) -> bool {
@@ -150,6 +157,7 @@ impl LocationState {
         if protected && self.accessed && transition.produces_disabled() {
             return Err(TransitionError::ProtectedDisabled(old_perm));
         }
+        debug_assert!(self.possible());
         Ok(transition)
     }
 
@@ -397,6 +405,7 @@ impl<'tcx> Tree {
             ProvenanceExtra::Wildcard => None,
         };
         assert!(outside_perm.is_initial());
+        assert!(!outside_perm.is_unique());
 
         let default_strongest_idempotent =
             outside_perm.strongest_idempotent_foreign_access(protected);
@@ -690,7 +699,7 @@ impl<'tcx> Tree {
         for (loc_range, loc) in self.locations.iter_mut_all() {
             // Only visit accessed permissions
             if let Some(p) = loc.perms.get(source_idx)
-                && let Some(access_kind) = p.permission.protector_end_access()
+                && let Some(access_kind) = p.permission.associated_access()
                 && p.accessed
             {
                 let diagnostics = DiagnosticInfo {