diff --git a/src/borrow_tracker/tree_borrows/perms.rs b/src/borrow_tracker/tree_borrows/perms.rs index bd4573f940..064c1cc5b9 100644 --- a/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/borrow_tracker/tree_borrows/perms.rs @@ -374,9 +374,9 @@ impl Permission { self.inner.strongest_idempotent_foreign_access(prot) } - /// Returns the strongest access allowed from a child to this node without + /// Returns the strongest access allowed that is local to this node without /// causing UB (only considers possible transitions to this permission). - pub fn strongest_allowed_child_access(&self, protected: bool) -> WildcardAccessLevel { + pub fn strongest_allowed_local_access(&self, protected: bool) -> WildcardAccessLevel { match self.inner { // Everything except disabled can be accessed by read access. Disabled => WildcardAccessLevel::None, @@ -794,9 +794,9 @@ mod propagation_optimization_checks { /// Checks that `strongest_allowed_child_access` correctly /// represents which transitions are possible. #[test] - fn strongest_allowed_child_access() { + fn strongest_allowed_local_access() { for (permission, protected) in <(Permission, bool)>::exhaustive() { - let strongest_child_access = permission.strongest_allowed_child_access(protected); + let strongest_local_access = permission.strongest_allowed_local_access(protected); let is_read_valid = Permission::perform_access( AccessKind::Read, @@ -814,8 +814,8 @@ mod propagation_optimization_checks { ) .is_some(); - assert_eq!(is_read_valid, strongest_child_access >= WildcardAccessLevel::Read); - assert_eq!(is_write_valid, strongest_child_access >= WildcardAccessLevel::Write); + assert_eq!(is_read_valid, strongest_local_access >= WildcardAccessLevel::Read); + assert_eq!(is_write_valid, strongest_local_access >= WildcardAccessLevel::Write); } } } diff --git a/src/borrow_tracker/tree_borrows/tree.rs b/src/borrow_tracker/tree_borrows/tree.rs index 900e9c3729..c8e27ebf0f 100644 --- a/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/borrow_tracker/tree_borrows/tree.rs @@ -111,7 +111,7 @@ impl LocationState { // We need to update the wildcard state, if the permission // of an exposed pointer changes. if node.is_exposed { - let access_type = self.permission.strongest_allowed_child_access(protected); + let access_type = self.permission.strongest_allowed_local_access(protected); WildcardState::update_exposure(idx, access_type, nodes, wildcard_accesses); } } @@ -1034,6 +1034,9 @@ impl<'tcx> LocationTree { wildcard_state.access_relatedness(access_kind, only_foreign) }; + // Whether there is an exposed node in this tree that allows this access. + let mut has_valid_exposed = false; + // This does a traversal across the tree updating children before their parents. The // difference to `perform_normal_access` is that we take the access relatedness from // the wildcard tracking state of the node instead of from the visitor itself. @@ -1082,6 +1085,17 @@ impl<'tcx> LocationTree { return Err(no_valid_exposed_references_error(diagnostics)); }; + let mut entry = args.data.perms.entry(args.idx); + let perm = entry.or_insert(node.default_location_state()); + + // We only count exposed nodes through which an access could happen. + if node.is_exposed + && perm.permission.strongest_allowed_local_access(protected).allows(access_kind) + && max_local_tag.is_none_or(|max_local_tag| max_local_tag >= node.tag) + { + has_valid_exposed = true; + } + let Some(relatedness) = wildcard_relatedness.to_relatedness() else { // If the access type is Either, then we do not apply any transition // to this node, but we still update each of its children. @@ -1090,8 +1104,6 @@ impl<'tcx> LocationTree { return Ok(()); }; - let mut entry = args.data.perms.entry(args.idx); - let perm = entry.or_insert(node.default_location_state()); // We know the exact relatedness, so we can actually do precise checks. perm.perform_transition( args.idx, @@ -1115,6 +1127,21 @@ impl<'tcx> LocationTree { }) }, )?; + // If there is no exposed node in this tree that allows this access, then the + // access *must* be foreign. So we check if the root of this tree would allow this + // as a foreign access, and if not, then we can error. + // In practice, all wildcard trees accept foreign accesses, but the main tree does + // not, so this catches UB when none of the nodes in the main tree allows this access. + if !has_valid_exposed + && self + .wildcard_accesses + .get(root) + .unwrap() + .access_relatedness(access_kind, /* only_foreign */ true) + .is_none() + { + return Err(no_valid_exposed_references_error(diagnostics)).into(); + } interp_ok(()) } } diff --git a/src/borrow_tracker/tree_borrows/wildcard.rs b/src/borrow_tracker/tree_borrows/wildcard.rs index 3b55a9e36e..b5ae0ee4c7 100644 --- a/src/borrow_tracker/tree_borrows/wildcard.rs +++ b/src/borrow_tracker/tree_borrows/wildcard.rs @@ -20,6 +20,16 @@ pub enum WildcardAccessLevel { Read, Write, } +impl WildcardAccessLevel { + /// Weather this access kind is allowed at this level. + pub fn allows(self, kind: AccessKind) -> bool { + let required_level = match kind { + AccessKind::Read => Self::Read, + AccessKind::Write => Self::Write, + }; + required_level <= self + } +} /// Where the access happened relative to the current node. #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -430,7 +440,7 @@ impl Tree { .map(|p| p.permission()) .unwrap_or_else(|| node.default_location_state().permission()); - let access_type = perm.strongest_allowed_child_access(protected); + let access_type = perm.strongest_allowed_local_access(protected); WildcardState::update_exposure( id, access_type, @@ -480,7 +490,7 @@ impl Tree { perms.get(id).copied().unwrap_or_else(|| node.default_location_state()); perm.permission() - .strongest_allowed_child_access(protected_tags.contains_key(&node.tag)) + .strongest_allowed_local_access(protected_tags.contains_key(&node.tag)) } else { WildcardAccessLevel::None }; diff --git a/tests/fail/tree_borrows/wildcard/cross_tree_update_main_invalid_exposed2.rs b/tests/fail/tree_borrows/wildcard/cross_tree_update_main_invalid_exposed2.rs new file mode 100644 index 0000000000..ca430cad16 --- /dev/null +++ b/tests/fail/tree_borrows/wildcard/cross_tree_update_main_invalid_exposed2.rs @@ -0,0 +1,48 @@ +//@compile-flags: -Zmiri-tree-borrows -Zmiri-permissive-provenance +use std::cell::Cell; + +/// Checks how accesses from one subtree affect other subtrees. +/// This test checks that an access from a subtree performs a +/// wildcard access on all earlier trees, and that local +/// accesses are treated as access errors for tags that are +/// larger than the root of the accessed subtree. +/// This tests the case were we have multiple exposed nodes on +/// the main tree that are invalid because their tag is too large. +pub fn main() { + let mut x: u32 = 42; + + let ptr_base = &mut x as *mut u32; + let ref1 = unsafe { &mut *ptr_base }; + let int1 = ref1 as *mut u32 as usize; + let wild = int1 as *mut u32; + + // Activates ref1. + *ref1 = 4; + + let ref2 = unsafe { &mut *wild }; + + // Freezes ref1. + let ref3 = unsafe { &mut *(ptr_base as *mut Cell) }; + let _int3 = ref3 as *mut Cell as usize; + let ref4 = unsafe { &mut *(ptr_base as *mut Cell) }; + let _int4 = ref4 as *mut Cell as usize; + + // ┌──────────────┐ + // │ │ + // │ptr_base(Act) ├───────────┬──────────────────┐ * + // │ │ │ │ │ + // └──────┬───────┘ │ │ │ + // │ │ │ │ + // │ │ │ │ + // ▼ ▼ ▼ ▼ + // ┌─────────────┐ ┌────────────┐ ┌────────────┐ ┌───────────┐ + // │ │ │ │ │ │ │ │ + // │ ref1(Frz)* │ │ ref3(ReIM)*│ │ ref4(ReIM)*│ │ ref2(Res) │ + // │ │ │ │ │ │ │ │ + // └─────────────┘ └────────────┘ └────────────┘ └───────────┘ + + // Performs a wildcard access on the main root. However, as there are + // no exposed tags with write permissions and a tag smaller than ref2 + // this access fails. + *ref2 = 13; //~ ERROR: /write access through .* is forbidden/ +} diff --git a/tests/fail/tree_borrows/wildcard/cross_tree_update_main_invalid_exposed2.stderr b/tests/fail/tree_borrows/wildcard/cross_tree_update_main_invalid_exposed2.stderr new file mode 100644 index 0000000000..1f7a3d6182 --- /dev/null +++ b/tests/fail/tree_borrows/wildcard/cross_tree_update_main_invalid_exposed2.stderr @@ -0,0 +1,14 @@ +error: Undefined Behavior: write access through at ALLOC[0x0] is forbidden + --> tests/fail/tree_borrows/wildcard/cross_tree_update_main_invalid_exposed2.rs:LL:CC + | +LL | *ref2 = 13; + | ^^^^^^^^^^ Undefined Behavior occurred here + | + = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental + = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/tree-borrows.md for further information + = help: there are no exposed tags which may perform this access here + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error +