diff --git a/src/borrow_tracker/tree_borrows/diagnostics.rs b/src/borrow_tracker/tree_borrows/diagnostics.rs index 8abc8530f7..a753de28a0 100644 --- a/src/borrow_tracker/tree_borrows/diagnostics.rs +++ b/src/borrow_tracker/tree_borrows/diagnostics.rs @@ -19,7 +19,7 @@ pub enum AccessCause { Explicit(AccessKind), Reborrow, Dealloc, - FnExit, + FnExit(AccessKind), } impl fmt::Display for AccessCause { @@ -28,7 +28,11 @@ impl fmt::Display for AccessCause { Self::Explicit(kind) => write!(f, "{kind}"), Self::Reborrow => write!(f, "reborrow"), Self::Dealloc => write!(f, "deallocation"), - Self::FnExit => write!(f, "protector release"), + // This is dead code, since the protector release access itself can never + // cause UB (while the protector is active, if some other access invalidates + // further use of the protected tag, that is immediate UB). + // Describing the cause of UB is the only time this function is called. + Self::FnExit(_) => unreachable!("protector accesses can never be the source of UB"), } } } @@ -40,7 +44,7 @@ impl AccessCause { Self::Explicit(kind) => format!("{rel} {kind}"), Self::Reborrow => format!("reborrow (acting as a {rel} read access)"), Self::Dealloc => format!("deallocation (acting as a {rel} write access)"), - Self::FnExit => format!("protector release (acting as a {rel} read access)"), + Self::FnExit(kind) => format!("protector release (acting as a {rel} {kind})"), } } } diff --git a/src/borrow_tracker/tree_borrows/mod.rs b/src/borrow_tracker/tree_borrows/mod.rs index 77e003ab8a..8607438408 100644 --- a/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/borrow_tracker/tree_borrows/mod.rs @@ -68,13 +68,11 @@ impl<'tcx> Tree { let global = machine.borrow_tracker.as_ref().unwrap(); let span = machine.current_span(); self.perform_access( - access_kind, tag, - Some(range), + Some((range, access_kind, diagnostics::AccessCause::Explicit(access_kind))), global, alloc_id, span, - diagnostics::AccessCause::Explicit(access_kind), ) } @@ -115,15 +113,8 @@ impl<'tcx> Tree { alloc_id: AllocId, // diagnostics ) -> InterpResult<'tcx> { let span = machine.current_span(); - self.perform_access( - AccessKind::Read, - tag, - None, // no specified range because it occurs on the entire allocation - global, - alloc_id, - span, - diagnostics::AccessCause::FnExit, - ) + // `None` makes it the magic on-protector-end operation + self.perform_access(tag, None, global, alloc_id, span) } } @@ -297,13 +288,11 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // All reborrows incur a (possibly zero-sized) read access to the parent tree_borrows.perform_access( - AccessKind::Read, orig_tag, - Some(range), + Some((range, AccessKind::Read, diagnostics::AccessCause::Reborrow)), this.machine.borrow_tracker.as_ref().unwrap(), alloc_id, this.machine.current_span(), - diagnostics::AccessCause::Reborrow, )?; // Record the parent-child pair in the tree. tree_borrows.new_child(orig_tag, new_tag, new_perm.initial_state, range, span)?; diff --git a/src/borrow_tracker/tree_borrows/perms.rs b/src/borrow_tracker/tree_borrows/perms.rs index fb3a4c8dad..7aa9c3e862 100644 --- a/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/borrow_tracker/tree_borrows/perms.rs @@ -186,6 +186,10 @@ impl Permission { pub fn is_disabled(&self) -> bool { self.inner == Disabled } + /// Check if `self` is the post-child-write state of a pointer (is `Active`). + pub fn is_active(&self) -> bool { + self.inner == Active + } /// 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! diff --git a/src/borrow_tracker/tree_borrows/tree.rs b/src/borrow_tracker/tree_borrows/tree.rs index ff4589657a..90bd110321 100644 --- a/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/borrow_tracker/tree_borrows/tree.rs @@ -530,13 +530,11 @@ impl<'tcx> Tree { span: Span, // diagnostics ) -> InterpResult<'tcx> { self.perform_access( - AccessKind::Write, tag, - Some(access_range), + Some((access_range, AccessKind::Write, diagnostics::AccessCause::Dealloc)), global, alloc_id, span, - diagnostics::AccessCause::Dealloc, )?; for (perms_range, perms) in self.rperms.iter_mut(access_range.start, access_range.size) { TreeVisitor { nodes: &mut self.nodes, tag_mapping: &self.tag_mapping, perms } @@ -570,12 +568,16 @@ impl<'tcx> Tree { } /// Map the per-node and per-location `LocationState::perform_access` - /// to each location of `access_range`, on every tag of the allocation. + /// to each location of the first component of `access_range_and_kind`, + /// on every tag of the allocation. /// - /// If `access_range` is `None`, this is interpreted as the special + /// If `access_range_and_kind` is `None`, this is interpreted as the special /// access that is applied on protector release: /// - the access will be applied only to initialized locations of the allocation, - /// - and it will not be visible to children. + /// - it will not be visible to children, + /// - it will be recorded as a `FnExit` diagnostic access + /// - and it will be a read except if the location is `Active`, i.e. has been written to, + /// in which case it will be a write. /// /// `LocationState::perform_access` will take care of raising transition /// errors and updating the `initialized` status of each location, @@ -585,13 +587,11 @@ impl<'tcx> Tree { /// - recording the history. pub fn perform_access( &mut self, - access_kind: AccessKind, tag: BorTag, - access_range: Option, + access_range_and_kind: Option<(AllocRange, AccessKind, diagnostics::AccessCause)>, global: &GlobalState, - alloc_id: AllocId, // diagnostics - span: Span, // diagnostics - access_cause: diagnostics::AccessCause, // diagnostics + alloc_id: AllocId, // diagnostics + span: Span, // diagnostics ) -> InterpResult<'tcx> { use std::ops::Range; // Performs the per-node work: @@ -605,6 +605,8 @@ impl<'tcx> Tree { // `perms_range` is only for diagnostics (it is the range of // the `RangeMap` on which we are currently working). let node_app = |perms_range: Range, + access_kind: AccessKind, + access_cause: diagnostics::AccessCause, args: NodeAppArgs<'_>| -> Result { let NodeAppArgs { node, mut perm, rel_pos } = args; @@ -618,14 +620,13 @@ impl<'tcx> Tree { let protected = global.borrow().protected_tags.contains_key(&node.tag); let transition = old_state.perform_access(access_kind, rel_pos, protected)?; - // Record the event as part of the history if !transition.is_noop() { node.debug_info.history.push(diagnostics::Event { transition, is_foreign: rel_pos.is_foreign(), access_cause, - access_range, + access_range: access_range_and_kind.map(|x| x.0), transition_range: perms_range, span, }); @@ -636,6 +637,7 @@ impl<'tcx> Tree { // Error handler in case `node_app` goes wrong. // Wraps the faulty transition in more context for diagnostics. let err_handler = |perms_range: Range, + access_cause: diagnostics::AccessCause, args: ErrHandlerArgs<'_, TransitionError>| -> InterpError<'tcx> { let ErrHandlerArgs { error_kind, conflicting_info, accessed_info } = args; @@ -650,7 +652,7 @@ impl<'tcx> Tree { .build() }; - if let Some(access_range) = access_range { + if let Some((access_range, access_kind, access_cause)) = access_range_and_kind { // Default branch: this is a "normal" access through a known range. // We iterate over affected locations and traverse the tree for each of them. for (perms_range, perms) in self.rperms.iter_mut(access_range.start, access_range.size) @@ -658,8 +660,8 @@ impl<'tcx> Tree { TreeVisitor { nodes: &mut self.nodes, tag_mapping: &self.tag_mapping, perms } .traverse_parents_this_children_others( tag, - |args| node_app(perms_range.clone(), args), - |args| err_handler(perms_range.clone(), args), + |args| node_app(perms_range.clone(), access_kind, access_cause, args), + |args| err_handler(perms_range.clone(), access_cause, args), )?; } } else { @@ -678,11 +680,14 @@ impl<'tcx> Tree { if let Some(p) = perms.get(idx) && p.initialized { + let access_kind = + if p.permission.is_active() { AccessKind::Write } else { AccessKind::Read }; + let access_cause = diagnostics::AccessCause::FnExit(access_kind); TreeVisitor { nodes: &mut self.nodes, tag_mapping: &self.tag_mapping, perms } .traverse_nonchildren( tag, - |args| node_app(perms_range.clone(), args), - |args| err_handler(perms_range.clone(), args), + |args| node_app(perms_range.clone(), access_kind, access_cause, args), + |args| err_handler(perms_range.clone(), access_cause, args), )?; } } diff --git a/tests/fail/tree_borrows/protector-write-lazy.rs b/tests/fail/tree_borrows/protector-write-lazy.rs new file mode 100644 index 0000000000..238f6dba9d --- /dev/null +++ b/tests/fail/tree_borrows/protector-write-lazy.rs @@ -0,0 +1,35 @@ +//@compile-flags: -Zmiri-tree-borrows +// This test tests that TB's protector end semantics correctly ensure +// that protected activated writes can be reordered. +fn the_other_function(ref_to_fst_elem: &mut i32, ptr_to_vec: *mut i32) -> *mut i32 { + // Activate the reference. Afterwards, we should be able to reorder arbitrary writes. + *ref_to_fst_elem = 0; + // Here is such an arbitrary write. + // It could be moved down after the retag, in which case the `funky_ref` would be invalidated. + // We need to ensure that the `funky_ptr` is unusable even if the write to `ref_to_fst_elem` + // happens before the retag. + *ref_to_fst_elem = 42; + // this creates a reference that is Reserved Lazy on the first element (offset 0). + // It does so by doing a proper retag on the second element (offset 1), which is fine + // since nothing else happens at that offset, but the lazy init mechanism means it's + // also reserved at offset 0, but not initialized. + let funky_ptr_lazy_on_fst_elem = + unsafe { (&mut *(ptr_to_vec.wrapping_add(1))) as *mut i32 }.wrapping_sub(1); + // If we write to `ref_to_fst_elem` here, then any further access to `funky_ptr_lazy_on_fst_elem` would + // definitely be UB. Since the compiler ought to be able to reorder the write of `42` above down to + // here, that means we want this program to also be UB. + return funky_ptr_lazy_on_fst_elem; +} + +fn main() { + let mut v = vec![0, 1]; + // get a pointer to the root of the allocation + // note that it's not important it's the actual root, what matters is that it's a parent + // of both references that will be created + let ptr_to_vec = v.as_mut_ptr(); + let ref_to_fst_elem = unsafe { &mut *ptr_to_vec }; + let funky_ptr_lazy_on_fst_elem = the_other_function(ref_to_fst_elem, ptr_to_vec); + // now we try to use the funky lazy pointer. + // It should be UB, since the write-on-protector-end should disable it. + unsafe { println!("Value of funky: {}", *funky_ptr_lazy_on_fst_elem) } //~ ERROR: /reborrow through .* is forbidden/ +} diff --git a/tests/fail/tree_borrows/protector-write-lazy.stderr b/tests/fail/tree_borrows/protector-write-lazy.stderr new file mode 100644 index 0000000000..955abd144c --- /dev/null +++ b/tests/fail/tree_borrows/protector-write-lazy.stderr @@ -0,0 +1,27 @@ +error: Undefined Behavior: reborrow through at ALLOC[0x0] is forbidden + --> $DIR/protector-write-lazy.rs:LL:CC + | +LL | unsafe { println!("Value of funky: {}", *funky_ptr_lazy_on_fst_elem) } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ reborrow through at ALLOC[0x0] is forbidden + | + = 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: the accessed tag has state Disabled which forbids this reborrow (acting as a child read access) +help: the accessed tag was created here, in the initial state Reserved + --> $DIR/protector-write-lazy.rs:LL:CC + | +LL | unsafe { (&mut *(ptr_to_vec.wrapping_add(1))) as *mut i32 }.wrapping_sub(1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: the accessed tag later transitioned to Disabled due to a protector release (acting as a foreign write access) on every location previously accessed by this tag + --> $DIR/protector-write-lazy.rs:LL:CC + | +LL | } + | ^ + = help: this transition corresponds to a loss of read and write permissions + = note: BACKTRACE (of the first span): + = note: inside `main` at $DIR/protector-write-lazy.rs:LL:CC + = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error +