Merge pull request #4630 from royAmmerschuber/pull-request/wildcard-provenance

initial implementation of wildcard provenence for tree borrows

Author: RalfJung

Cargo.toml

@@ -72,7 +72,7 @@ harness = false
 default = ["stack-cache", "native-lib"]
 genmc = ["dep:genmc-sys"]
 stack-cache = []
-stack-cache-consistency-check = ["stack-cache"]
+expensive-consistency-checks = ["stack-cache"]
 tracing = ["serde_json"]
 native-lib = ["dep:libffi", "dep:libloading", "dep:capstone", "dep:ipc-channel", "dep:nix", "dep:serde"]
 

src/bin/miri.rs

@@ -509,7 +509,6 @@ fn main() {
                 Some(BorrowTrackerMethod::TreeBorrows(TreeBorrowsParams {
                     precise_interior_mut: true,
                 }));
-            miri_config.provenance_mode = ProvenanceMode::Strict;
         } else if arg == "-Zmiri-tree-borrows-no-precise-interior-mut" {
             match &mut miri_config.borrow_tracker {
                 Some(BorrowTrackerMethod::TreeBorrows(params)) => {
@@ -703,17 +702,6 @@ fn main() {
             rustc_args.push(arg);
         }
     }
-    // Tree Borrows implies strict provenance, and is not compatible with native calls.
-    if matches!(miri_config.borrow_tracker, Some(BorrowTrackerMethod::TreeBorrows { .. })) {
-        if miri_config.provenance_mode != ProvenanceMode::Strict {
-            fatal_error!(
-                "Tree Borrows does not support integer-to-pointer casts, and hence requires strict provenance"
-            );
-        }
-        if !miri_config.native_lib.is_empty() {
-            fatal_error!("Tree Borrows is not compatible with calling native functions");
-        }
-    }
 
     // Native calls and strict provenance are not compatible.
     if !miri_config.native_lib.is_empty() && miri_config.provenance_mode == ProvenanceMode::Strict {

src/borrow_tracker/mod.rs

@@ -352,6 +352,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let borrow_tracker = this.machine.borrow_tracker.as_ref().unwrap();
         // The body of this loop needs `borrow_tracker` immutably
         // so we can't move this code inside the following `end_call`.
+
         for (alloc_id, tag) in &frame
             .extra
             .borrow_tracker
@@ -378,6 +379,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             }
         }
         borrow_tracker.borrow_mut().end_call(&frame.extra);
+
         interp_ok(())
     }
 }

src/borrow_tracker/stacked_borrows/stack.rs

@@ -153,7 +153,7 @@ impl<'tcx> Stack {
     /// Panics if any of the caching mechanisms have broken,
     /// - The StackCache indices don't refer to the parallel items,
     /// - There are no Unique items outside of first_unique..last_unique
-    #[cfg(feature = "stack-cache-consistency-check")]
+    #[cfg(feature = "expensive-consistency-checks")]
     fn verify_cache_consistency(&self) {
         // Only a full cache needs to be valid. Also see the comments in find_granting_cache
         // and set_unknown_bottom.
@@ -197,7 +197,7 @@ impl<'tcx> Stack {
         tag: ProvenanceExtra,
         exposed_tags: &FxHashSet<BorTag>,
     ) -> Result<Option<usize>, ()> {
-        #[cfg(feature = "stack-cache-consistency-check")]
+        #[cfg(feature = "expensive-consistency-checks")]
         self.verify_cache_consistency();
 
         let ProvenanceExtra::Concrete(tag) = tag else {
@@ -334,7 +334,7 @@ impl<'tcx> Stack {
         // This primes the cache for the next access, which is almost always the just-added tag.
         self.cache.add(new_idx, new);
 
-        #[cfg(feature = "stack-cache-consistency-check")]
+        #[cfg(feature = "expensive-consistency-checks")]
         self.verify_cache_consistency();
     }
 
@@ -417,7 +417,7 @@ impl<'tcx> Stack {
             self.unique_range.end = self.unique_range.end.min(disable_start);
         }
 
-        #[cfg(feature = "stack-cache-consistency-check")]
+        #[cfg(feature = "expensive-consistency-checks")]
         self.verify_cache_consistency();
 
         interp_ok(())
@@ -472,7 +472,7 @@ impl<'tcx> Stack {
             self.unique_range = 0..0;
         }
 
-        #[cfg(feature = "stack-cache-consistency-check")]
+        #[cfg(feature = "expensive-consistency-checks")]
         self.verify_cache_consistency();
         interp_ok(())
     }

src/borrow_tracker/tree_borrows/diagnostics.rs

@@ -291,9 +291,10 @@ pub(super) struct TbError<'node> {
     pub conflicting_info: &'node NodeDebugInfo,
     // What kind of access caused this error (read, write, reborrow, deallocation)
     pub access_cause: AccessCause,
-    /// Which tag the access that caused this error was made through, i.e.
+    /// Which tag, if any, the access that caused this error was made through, i.e.
     /// which tag was used to read/write/deallocate.
-    pub accessed_info: &'node NodeDebugInfo,
+    /// Not set on wildcard accesses.
+    pub accessed_info: Option<&'node NodeDebugInfo>,
 }
 
 impl TbError<'_> {
@@ -302,10 +303,20 @@ impl TbError<'_> {
         use TransitionError::*;
         let cause = self.access_cause;
         let accessed = self.accessed_info;
+        let accessed_str =
+            self.accessed_info.map(|v| format!("{v}")).unwrap_or_else(|| "<wildcard>".into());
         let conflicting = self.conflicting_info;
-        let accessed_is_conflicting = accessed.tag == conflicting.tag;
+        // An access is considered conflicting if it happened through a
+        // different tag than the one who caused UB.
+        // When doing a wildcard access (where `accessed` is `None`) we
+        // do not know which precise tag the accessed happened from,
+        // however we can be certain that it did not come from the
+        // conflicting tag.
+        // This is because the wildcard data structure already removes
+        // all tags through which an access would cause UB.
+        let accessed_is_conflicting = accessed.map(|a| a.tag) == Some(conflicting.tag);
         let title = format!(
-            "{cause} through {accessed} at {alloc_id:?}[{offset:#x}] is forbidden",
+            "{cause} through {accessed_str} at {alloc_id:?}[{offset:#x}] is forbidden",
             alloc_id = self.alloc_id,
             offset = self.error_offset
         );
@@ -316,7 +327,7 @@ impl TbError<'_> {
                 let mut details = Vec::new();
                 if !accessed_is_conflicting {
                     details.push(format!(
-                        "the accessed tag {accessed} is a child of the conflicting tag {conflicting}"
+                        "the accessed tag {accessed_str} is a child of the conflicting tag {conflicting}"
                     ));
                 }
                 let access = cause.print_as_access(/* is_foreign */ false);
@@ -330,7 +341,7 @@ impl TbError<'_> {
                 let access = cause.print_as_access(/* is_foreign */ true);
                 let details = vec![
                     format!(
-                        "the accessed tag {accessed} is foreign to the {conflicting_tag_name} tag {conflicting} (i.e., it is not a child)"
+                        "the accessed tag {accessed_str} is foreign to the {conflicting_tag_name} tag {conflicting} (i.e., it is not a child)"
                     ),
                     format!(
                         "this {access} would cause the {conflicting_tag_name} tag {conflicting} (currently {before_disabled}) to become Disabled"
@@ -343,16 +354,18 @@ impl TbError<'_> {
                 let conflicting_tag_name = "strongly protected";
                 let details = vec![
                     format!(
-                        "the allocation of the accessed tag {accessed} also contains the {conflicting_tag_name} tag {conflicting}"
+                        "the allocation of the accessed tag {accessed_str} also contains the {conflicting_tag_name} tag {conflicting}"
                     ),
                     format!("the {conflicting_tag_name} tag {conflicting} disallows deallocations"),
                 ];
                 (title, details, conflicting_tag_name)
             }
         };
         let mut history = HistoryData::default();
-        if !accessed_is_conflicting {
-            history.extend(self.accessed_info.history.forget(), "accessed", false);
+        if let Some(accessed_info) = self.accessed_info
+            && !accessed_is_conflicting
+        {
+            history.extend(accessed_info.history.forget(), "accessed", false);
         }
         history.extend(
             self.conflicting_info.history.extract_relevant(self.error_offset, self.error_kind),
@@ -363,6 +376,20 @@ impl TbError<'_> {
     }
 }
 
+/// Cannot access this allocation with wildcard provenance, as there are no
+/// valid exposed references for this access kind.
+pub fn no_valid_exposed_references_error<'tcx>(
+    alloc_id: AllocId,
+    offset: u64,
+    access_cause: AccessCause,
+) -> InterpErrorKind<'tcx> {
+    let title =
+        format!("{access_cause} through <wildcard> at {alloc_id:?}[{offset:#x}] is forbidden");
+    let details = vec![format!("there are no exposed tags which may perform this access here")];
+    let history = HistoryData::default();
+    err_machine_stop!(TerminationInfo::TreeBorrowsUb { title, details, history })
+}
+
 type S = &'static str;
 /// Pretty-printing details
 ///
@@ -623,10 +650,10 @@ impl DisplayRepr {
             } else {
                 // We take this node
                 let rperm = tree
-                    .rperms
+                    .locations
                     .iter_all()
-                    .map(move |(_offset, perms)| {
-                        let perm = perms.get(idx);
+                    .map(move |(_offset, loc)| {
+                        let perm = loc.perms.get(idx);
                         perm.cloned()
                     })
                     .collect::<Vec<_>>();
@@ -788,7 +815,7 @@ impl<'tcx> Tree {
         show_unnamed: bool,
     ) -> InterpResult<'tcx> {
         let mut indenter = DisplayIndent::new();
-        let ranges = self.rperms.iter_all().map(|(range, _perms)| range).collect::<Vec<_>>();
+        let ranges = self.locations.iter_all().map(|(range, _loc)| range).collect::<Vec<_>>();
         if let Some(repr) = DisplayRepr::from(self, show_unnamed) {
             repr.print(
                 &DEFAULT_FORMATTER,

src/borrow_tracker/tree_borrows/mod.rs

@@ -14,6 +14,7 @@ mod foreign_access_skipping;
 mod perms;
 mod tree;
 mod unimap;
+mod wildcard;
 
 #[cfg(test)]
 mod exhaustive;
@@ -54,16 +55,10 @@ impl<'tcx> Tree {
             interpret::Pointer::new(alloc_id, range.start),
             range.size.bytes(),
         );
-        // TODO: for now we bail out on wildcard pointers. Eventually we should
-        // handle them as much as we can.
-        let tag = match prov {
-            ProvenanceExtra::Concrete(tag) => tag,
-            ProvenanceExtra::Wildcard => return interp_ok(()),
-        };
         let global = machine.borrow_tracker.as_ref().unwrap();
         let span = machine.current_user_relevant_span();
         self.perform_access(
-            tag,
+            prov,
             Some((range, access_kind, diagnostics::AccessCause::Explicit(access_kind))),
             global,
             alloc_id,
@@ -79,19 +74,9 @@ impl<'tcx> Tree {
         size: Size,
         machine: &MiriMachine<'tcx>,
     ) -> InterpResult<'tcx> {
-        // TODO: for now we bail out on wildcard pointers. Eventually we should
-        // handle them as much as we can.
-        let tag = match prov {
-            ProvenanceExtra::Concrete(tag) => tag,
-            ProvenanceExtra::Wildcard => return interp_ok(()),
-        };
         let global = machine.borrow_tracker.as_ref().unwrap();
         let span = machine.current_user_relevant_span();
-        self.dealloc(tag, alloc_range(Size::ZERO, size), global, alloc_id, span)
-    }
-
-    pub fn expose_tag(&mut self, _tag: BorTag) {
-        // TODO
+        self.dealloc(prov, alloc_range(Size::ZERO, size), global, alloc_id, span)
     }
 
     /// A tag just lost its protector.
@@ -109,7 +94,11 @@ impl<'tcx> Tree {
     ) -> InterpResult<'tcx> {
         let span = machine.current_user_relevant_span();
         // `None` makes it the magic on-protector-end operation
-        self.perform_access(tag, None, global, alloc_id, span)
+        self.perform_access(ProvenanceExtra::Concrete(tag), None, global, alloc_id, span)?;
+
+        self.update_exposure_for_protector_release(tag);
+
+        interp_ok(())
     }
 }
 
@@ -239,21 +228,22 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 assert_eq!(ptr_size, Size::ZERO); // we did the deref check above, size has to be 0 here
                 // This pointer doesn't come with an AllocId, so there's no
                 // memory to do retagging in.
+                let new_prov = place.ptr().provenance;
                 trace!(
-                    "reborrow of size 0: reference {:?} derived from {:?} (pointee {})",
-                    new_tag,
+                    "reborrow of size 0: reusing {:?} (pointee {})",
                     place.ptr(),
                     place.layout.ty,
                 );
                 log_creation(this, None)?;
                 // Keep original provenance.
-                return interp_ok(place.ptr().provenance);
+                return interp_ok(new_prov);
             }
         };
+
         log_creation(this, Some((alloc_id, base_offset, parent_prov)))?;
 
         let orig_tag = match parent_prov {
-            ProvenanceExtra::Wildcard => return interp_ok(place.ptr().provenance), // TODO: handle wildcard pointers
+            ProvenanceExtra::Wildcard => return interp_ok(place.ptr().provenance), // TODO: handle retagging wildcard pointers
             ProvenanceExtra::Concrete(tag) => tag,
         };
 
@@ -356,7 +346,7 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 };
 
                 tree_borrows.perform_access(
-                    orig_tag,
+                    parent_prov,
                     Some((range_in_alloc, AccessKind::Read, diagnostics::AccessCause::Reborrow)),
                     this.machine.borrow_tracker.as_ref().unwrap(),
                     alloc_id,
@@ -589,7 +579,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 // uncovers a non-supported `extern static`.
                 let alloc_extra = this.get_alloc_extra(alloc_id)?;
                 trace!("Tree Borrows tag {tag:?} exposed in {alloc_id:?}");
-                alloc_extra.borrow_tracker_tb().borrow_mut().expose_tag(tag);
+
+                let global = this.machine.borrow_tracker.as_ref().unwrap();
+                let protected_tags = &global.borrow().protected_tags;
+                let protected = protected_tags.contains_key(&tag);
+                alloc_extra.borrow_tracker_tb().borrow_mut().expose_tag(tag, protected);
             }
             AllocKind::Function | AllocKind::VTable | AllocKind::TypeId | AllocKind::Dead => {
                 // No tree borrows on these allocations.

src/borrow_tracker/tree_borrows/perms.rs

@@ -53,6 +53,7 @@ enum PermissionPriv {
 }
 use self::PermissionPriv::*;
 use super::foreign_access_skipping::IdempotentForeignAccess;
+use super::wildcard::WildcardAccessLevel;
 
 impl PartialOrd for PermissionPriv {
     /// PermissionPriv is ordered by the reflexive transitive closure of
@@ -372,6 +373,23 @@ impl Permission {
     pub fn strongest_idempotent_foreign_access(&self, prot: bool) -> IdempotentForeignAccess {
         self.inner.strongest_idempotent_foreign_access(prot)
     }
+
+    /// Returns the strongest access allowed from a child to this node without
+    /// causing UB (only considers possible transitions to this permission).
+    pub fn strongest_allowed_child_access(&self, protected: bool) -> WildcardAccessLevel {
+        match self.inner {
+            // Everything except disabled can be accessed by read access.
+            Disabled => WildcardAccessLevel::None,
+            // Frozen references cannot be written to by a child.
+            Frozen => WildcardAccessLevel::Read,
+            // If the `conflicted` flag is set, then there was a foreign read
+            // during the function call that is still ongoing (still `protected`),
+            // this is UB (`noalias` violation).
+            ReservedFrz { conflicted: true } if protected => WildcardAccessLevel::Read,
+            // Everything else allows writes.
+            _ => WildcardAccessLevel::Write,
+        }
+    }
 }
 
 impl PermTransition {
@@ -772,4 +790,32 @@ mod propagation_optimization_checks {
             );
         }
     }
+
+    /// Checks that  `strongest_allowed_child_access` correctly
+    /// represents which transitions are possible.
+    #[test]
+    fn strongest_allowed_child_access() {
+        for (permission, protected) in <(Permission, bool)>::exhaustive() {
+            let strongest_child_access = permission.strongest_allowed_child_access(protected);
+
+            let is_read_valid = Permission::perform_access(
+                AccessKind::Read,
+                AccessRelatedness::LocalAccess,
+                permission,
+                protected,
+            )
+            .is_some();
+
+            let is_write_valid = Permission::perform_access(
+                AccessKind::Write,
+                AccessRelatedness::LocalAccess,
+                permission,
+                protected,
+            )
+            .is_some();
+
+            assert_eq!(is_read_valid, strongest_child_access >= WildcardAccessLevel::Read);
+            assert_eq!(is_write_valid, strongest_child_access >= WildcardAccessLevel::Write);
+        }
+    }
 }