From 3492e7ec65ad0d017724e2cda27fd324a98ef565 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Wed, 20 Sep 2023 19:48:35 +0200 Subject: [PATCH] Implement implicit read on function exit Protected tags receive a read access on initialized locations. This makes tests/fail/tree_borrows/spurious_read.rs fail as expected, but it also introduces an issue in tests/pass/vec.rs Because the new error in tests/pass/vec.rs is cryptic and to ensure that we have a proper test for this issue, a new function is added to tests/pass/tree_borrows/tree-borrows.rs --- src/borrow_tracker/mod.rs | 20 +- .../stacked_borrows/diagnostics.rs | 3 +- src/borrow_tracker/stacked_borrows/mod.rs | 8 +- .../tree_borrows/diagnostics.rs | 18 +- src/borrow_tracker/tree_borrows/mod.rs | 39 ++- src/borrow_tracker/tree_borrows/tree.rs | 242 +++++++++++------- src/machine.rs | 27 +- tests/fail/tree_borrows/spurious_read.stderr | 44 ++++ tests/pass/tree_borrows/tree-borrows.rs | 16 ++ 9 files changed, 306 insertions(+), 111 deletions(-) create mode 100644 tests/fail/tree_borrows/spurious_read.stderr diff --git a/src/borrow_tracker/mod.rs b/src/borrow_tracker/mod.rs index b6dfd9944e..3a9f1726c5 100644 --- a/src/borrow_tracker/mod.rs +++ b/src/borrow_tracker/mod.rs @@ -66,10 +66,13 @@ pub struct FrameState { /// `stacked_borrows::GlobalState` upon function return, and if we attempt to pop a protected /// tag, to identify which call is responsible for protecting the tag. /// See `Stack::item_popped` for more explanation. + /// Stacked Borrows also needs to know which allocation these tags + /// belong to so that it can perform a read through them immediately before + /// the frame gets popped. /// /// This will contain one tag per reference passed to the function, so /// a size of 2 is enough for the vast majority of functions. - pub protected_tags: SmallVec<[BorTag; 2]>, + pub protected_tags: SmallVec<[(AllocId, BorTag); 2]>, } impl VisitTags for FrameState { @@ -208,7 +211,7 @@ impl GlobalStateInner { } pub fn end_call(&mut self, frame: &machine::FrameExtra<'_>) { - for tag in &frame + for (_, tag) in &frame .borrow_tracker .as_ref() .expect("we should have borrow tracking data") @@ -450,6 +453,19 @@ impl AllocState { AllocState::TreeBorrows(tb) => tb.borrow_mut().remove_unreachable_tags(tags), } } + + /// Tree Borrows needs to be told when a tag stops being protected. + pub fn release_protector<'tcx>( + &self, + machine: &MiriMachine<'_, 'tcx>, + global: &GlobalState, + tag: BorTag, + ) -> InterpResult<'tcx> { + match self { + AllocState::StackedBorrows(_sb) => Ok(()), + AllocState::TreeBorrows(tb) => tb.borrow_mut().release_protector(machine, global, tag), + } + } } impl VisitTags for AllocState { diff --git a/src/borrow_tracker/stacked_borrows/diagnostics.rs b/src/borrow_tracker/stacked_borrows/diagnostics.rs index 9b0f13dd62..c0ae08f756 100644 --- a/src/borrow_tracker/stacked_borrows/diagnostics.rs +++ b/src/borrow_tracker/stacked_borrows/diagnostics.rs @@ -429,6 +429,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { ProtectorKind::WeakProtector => "weakly protected", ProtectorKind::StrongProtector => "strongly protected", }; + let item_tag = item.tag(); let call_id = self .machine .threads @@ -437,7 +438,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { .map(|frame| { frame.extra.borrow_tracker.as_ref().expect("we should have borrow tracking data") }) - .find(|frame| frame.protected_tags.contains(&item.tag())) + .find(|frame| frame.protected_tags.iter().any(|(_, tag)| tag == &item_tag)) .map(|frame| frame.call_id) .unwrap(); // FIXME: Surely we should find something, but a panic seems wrong here? match self.operation { diff --git a/src/borrow_tracker/stacked_borrows/mod.rs b/src/borrow_tracker/stacked_borrows/mod.rs index a54bd32cd4..d0468253ff 100644 --- a/src/borrow_tracker/stacked_borrows/mod.rs +++ b/src/borrow_tracker/stacked_borrows/mod.rs @@ -718,7 +718,13 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' if let Some(protect) = new_perm.protector() { // See comment in `Stack::item_invalidated` for why we store the tag twice. - this.frame_mut().extra.borrow_tracker.as_mut().unwrap().protected_tags.push(new_tag); + this.frame_mut() + .extra + .borrow_tracker + .as_mut() + .unwrap() + .protected_tags + .push((alloc_id, new_tag)); this.machine .borrow_tracker .as_mut() diff --git a/src/borrow_tracker/tree_borrows/diagnostics.rs b/src/borrow_tracker/tree_borrows/diagnostics.rs index fd45671ba2..7e828d192c 100644 --- a/src/borrow_tracker/tree_borrows/diagnostics.rs +++ b/src/borrow_tracker/tree_borrows/diagnostics.rs @@ -19,6 +19,7 @@ pub enum AccessCause { Explicit(AccessKind), Reborrow, Dealloc, + FnExit, } impl fmt::Display for AccessCause { @@ -27,6 +28,7 @@ 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"), } } } @@ -38,6 +40,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)"), } } } @@ -52,7 +55,8 @@ pub struct Event { /// Relative position of the tag to the one used for the access. pub is_foreign: bool, /// User-visible range of the access. - pub access_range: AllocRange, + /// `None` means that this is an implicit access to the entire allocation. + pub access_range: Option, /// The transition recorded by this event only occured on a subrange of /// `access_range`: a single access on `access_range` triggers several events, /// each with their own mutually disjoint `transition_range`. No-op transitions @@ -123,7 +127,17 @@ impl HistoryData { // NOTE: `transition_range` is explicitly absent from the error message, it has no significance // to the user. The meaningful one is `access_range`. let access = access_cause.print_as_access(is_foreign); - self.events.push((Some(span.data()), format!("{this} later transitioned to {endpoint} due to a {access} at offsets {access_range:?}", endpoint = transition.endpoint()))); + let access_range_text = match access_range { + Some(r) => format!("at offsets {r:?}"), + None => format!("over the entire allocation"), + }; + self.events.push(( + Some(span.data()), + format!( + "{this} later transitioned to {endpoint} due to a {access} {access_range_text}", + endpoint = transition.endpoint() + ), + )); self.events .push((None, format!("this transition corresponds to {}", transition.summary()))); } diff --git a/src/borrow_tracker/tree_borrows/mod.rs b/src/borrow_tracker/tree_borrows/mod.rs index 06f15ffac1..9e856c377e 100644 --- a/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/borrow_tracker/tree_borrows/mod.rs @@ -2,7 +2,9 @@ use log::trace; use rustc_target::abi::{Abi, Align, Size}; -use crate::borrow_tracker::{AccessKind, GlobalStateInner, ProtectorKind, RetagFields}; +use crate::borrow_tracker::{ + AccessKind, GlobalState, GlobalStateInner, ProtectorKind, RetagFields, +}; use rustc_middle::{ mir::{Mutability, RetagKind}, ty::{self, layout::HasParamEnv, Ty}, @@ -66,7 +68,7 @@ impl<'tcx> Tree { self.perform_access( access_kind, tag, - range, + Some(range), global, span, diagnostics::AccessCause::Explicit(access_kind), @@ -95,6 +97,29 @@ impl<'tcx> Tree { pub fn expose_tag(&mut self, _tag: BorTag) { // TODO } + + /// A tag just lost its protector. + /// + /// This emits a special kind of access that is only applied + /// to initialized locations, as a protection against other + /// tags not having been made aware of the existence of this + /// protector. + pub fn release_protector( + &mut self, + machine: &MiriMachine<'_, 'tcx>, + global: &GlobalState, + tag: BorTag, + ) -> 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, + span, + diagnostics::AccessCause::FnExit, + ) + } } /// Policy for a new borrow. @@ -242,7 +267,13 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' // We register the protection in two different places. // This makes creating a protector slower, but checking whether a tag // is protected faster. - this.frame_mut().extra.borrow_tracker.as_mut().unwrap().protected_tags.push(new_tag); + this.frame_mut() + .extra + .borrow_tracker + .as_mut() + .unwrap() + .protected_tags + .push((alloc_id, new_tag)); this.machine .borrow_tracker .as_mut() @@ -269,7 +300,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' tree_borrows.perform_access( AccessKind::Read, orig_tag, - range, + Some(range), this.machine.borrow_tracker.as_ref().unwrap(), this.machine.current_span(), diagnostics::AccessCause::Reborrow, diff --git a/src/borrow_tracker/tree_borrows/tree.rs b/src/borrow_tracker/tree_borrows/tree.rs index eb466f89a8..f045a34729 100644 --- a/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/borrow_tracker/tree_borrows/tree.rs @@ -282,6 +282,56 @@ enum ContinueTraversal { SkipChildren, } +struct TreeVisitAux { + accessed_tag: UniIndex, + f_propagate: NodeApp, + err_builder: ErrHandler, + stack: Vec<(UniIndex, AccessRelatedness)>, +} +impl TreeVisitAux +where + NodeApp: Fn(NodeAppArgs<'_>) -> Result, + ErrHandler: Fn(ErrHandlerArgs<'_, InnErr>) -> OutErr, +{ + fn pop(&mut self) -> Option<(UniIndex, AccessRelatedness)> { + self.stack.pop() + } + + /// Apply the function to the current `tag`, and push its children + /// to the stack of future tags to visit. + fn exec_and_visit( + &mut self, + this: &mut TreeVisitor<'_>, + tag: UniIndex, + exclude: Option, + rel_pos: AccessRelatedness, + ) -> Result<(), OutErr> { + // 1. apply the propagation function + let node = this.nodes.get_mut(tag).unwrap(); + let recurse = + (self.f_propagate)(NodeAppArgs { node, perm: this.perms.entry(tag), rel_pos }) + .map_err(|error_kind| { + (self.err_builder)(ErrHandlerArgs { + error_kind, + conflicting_info: &this.nodes.get(tag).unwrap().debug_info, + accessed_info: &this.nodes.get(self.accessed_tag).unwrap().debug_info, + }) + })?; + let node = this.nodes.get(tag).unwrap(); + // 2. add the children to the stack for future traversal + if matches!(recurse, ContinueTraversal::Recurse) { + let child_rel = rel_pos.for_child(); + for &child in node.children.iter() { + // some child might be excluded from here and handled separately + if Some(child) != exclude { + self.stack.push((child, child_rel)); + } + } + } + Ok(()) + } +} + impl<'tree> TreeVisitor<'tree> { // Applies `f_propagate` to every vertex of the tree top-down in the following order: first // all ancestors of `start`, then `start` itself, then children of `start`, then the rest. @@ -298,62 +348,7 @@ impl<'tree> TreeVisitor<'tree> { start: BorTag, f_propagate: impl Fn(NodeAppArgs<'_>) -> Result, err_builder: impl Fn(ErrHandlerArgs<'_, InnErr>) -> OutErr, - ) -> Result<(), OutErr> -where { - struct TreeVisitAux { - accessed_tag: UniIndex, - f_propagate: NodeApp, - err_builder: ErrHandler, - stack: Vec<(UniIndex, AccessRelatedness)>, - } - impl TreeVisitAux - where - NodeApp: Fn(NodeAppArgs<'_>) -> Result, - ErrHandler: Fn(ErrHandlerArgs<'_, InnErr>) -> OutErr, - { - fn pop(&mut self) -> Option<(UniIndex, AccessRelatedness)> { - self.stack.pop() - } - - /// Apply the function to the current `tag`, and push its children - /// to the stack of future tags to visit. - fn exec_and_visit( - &mut self, - this: &mut TreeVisitor<'_>, - tag: UniIndex, - exclude: Option, - rel_pos: AccessRelatedness, - ) -> Result<(), OutErr> { - // 1. apply the propagation function - let node = this.nodes.get_mut(tag).unwrap(); - let recurse = - (self.f_propagate)(NodeAppArgs { node, perm: this.perms.entry(tag), rel_pos }) - .map_err(|error_kind| { - (self.err_builder)(ErrHandlerArgs { - error_kind, - conflicting_info: &this.nodes.get(tag).unwrap().debug_info, - accessed_info: &this - .nodes - .get(self.accessed_tag) - .unwrap() - .debug_info, - }) - })?; - let node = this.nodes.get(tag).unwrap(); - // 2. add the children to the stack for future traversal - if matches!(recurse, ContinueTraversal::Recurse) { - let child_rel = rel_pos.for_child(); - for &child in node.children.iter() { - // some child might be excluded from here and handled separately - if Some(child) != exclude { - self.stack.push((child, child_rel)); - } - } - } - Ok(()) - } - } - + ) -> Result<(), OutErr> { let start_idx = self.tag_mapping.get(&start).unwrap(); let mut stack = TreeVisitAux { accessed_tag: start_idx, f_propagate, err_builder, stack: Vec::new() }; @@ -482,7 +477,7 @@ impl<'tcx> Tree { self.perform_access( AccessKind::Write, tag, - access_range, + Some(access_range), global, span, diagnostics::AccessCause::Dealloc, @@ -519,6 +514,8 @@ 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. + /// If `access_range` is `None`, the access will be applied to + /// all initialized locations of the allocation. /// /// `LocationState::perform_access` will take care of raising transition /// errors and updating the `initialized` status of each location, @@ -530,56 +527,103 @@ impl<'tcx> Tree { &mut self, access_kind: AccessKind, tag: BorTag, - access_range: AllocRange, + access_range: Option, global: &GlobalState, span: Span, // diagnostics access_cause: diagnostics::AccessCause, // diagnostics ) -> InterpResult<'tcx> { - 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 } - .traverse_parents_this_children_others( - tag, - |args: NodeAppArgs<'_>| -> Result { - let NodeAppArgs { node, mut perm, rel_pos } = args; + use std::ops::Range; + // Per-node application: + // - insert the permission if it does not exist + // - perform the access + // - record the transition + // to which some optimizations are added: + // - skip the traversal of the children in some cases + // - do not record noop transitions + // + // This needs some extra data: the range of the `RangeMap` + // (the one that is not visible in diagnostics, only used to record events) + // is known only at call site. + let node_app = |perms_range: Range, + args: NodeAppArgs<'_>| + -> Result { + let NodeAppArgs { node, mut perm, rel_pos } = args; + + let old_state = perm.or_insert_with(|| LocationState::new(node.default_initial_perm)); + + match old_state.skip_if_known_noop(access_kind, rel_pos) { + ContinueTraversal::SkipChildren => return Ok(ContinueTraversal::SkipChildren), + _ => {} + } - let old_state = - perm.or_insert_with(|| LocationState::new(node.default_initial_perm)); + 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, + transition_range: perms_range.clone(), + span, + }); + } + Ok(ContinueTraversal::Recurse) + }; - match old_state.skip_if_known_noop(access_kind, rel_pos) { - ContinueTraversal::SkipChildren => - return Ok(ContinueTraversal::SkipChildren), - _ => {} - } + // Error handler in case `node_app` goes wrong to wrap the faulty + // transition in more context for diagnostics. + let err_handler = |perms_range: Range, + args: ErrHandlerArgs<'_, TransitionError>| + -> InterpError<'tcx> { + let ErrHandlerArgs { error_kind, conflicting_info, accessed_info } = args; + TbError { + conflicting_info, + access_cause, + error_offset: perms_range.start, + error_kind, + accessed_info, + } + .build() + }; - 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, - transition_range: perms_range.clone(), - span, - }); - } - Ok(ContinueTraversal::Recurse) - }, - |args: ErrHandlerArgs<'_, TransitionError>| -> InterpError<'tcx> { - let ErrHandlerArgs { error_kind, conflicting_info, accessed_info } = args; - TbError { - conflicting_info, - access_cause, - error_offset: perms_range.start, - error_kind, - accessed_info, + if let Some(access_range) = access_range { + // 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) + { + 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), + )?; + } + } else { + // This is a special access through the entire allocation. + // It actually only affects `initialized` locations, so we need + // to filter on those before initiating the traversal. + for (perms_range, perms) in self.rperms.iter_mut_all() { + let idx = self.tag_mapping.get(&tag).unwrap(); + if let Some(p) = perms.get(idx) { + // missing permissions cannot be initialized + if p.initialized { + // apply the following traversal only to initialized permissions + TreeVisitor { + nodes: &mut self.nodes, + tag_mapping: &self.tag_mapping, + perms, } - .build() - }, - )?; + .traverse_parents_this_children_others( + tag, + |args| node_app(perms_range.clone(), args), + |args| err_handler(perms_range.clone(), args), + )?; + } + } + } } Ok(()) } diff --git a/src/machine.rs b/src/machine.rs index ce7f47b5b4..420ba5038c 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -1382,8 +1382,31 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { ) -> InterpResult<'tcx> { // We want this *before* the return value copy, because the return place itself is protected // until we do `end_call` here. - if let Some(borrow_tracker) = &ecx.machine.borrow_tracker { - borrow_tracker.borrow_mut().end_call(&frame.extra); + if let Some(global_borrow_tracker) = &ecx.machine.borrow_tracker { + // The body of this loop needs `global_borrow_tracker` immutably + // so we can't move this code inside the following `end_call`. + for (alloc_id, tag) in &frame + .extra + .borrow_tracker + .as_ref() + .expect("we should have borrow tracking data") + .protected_tags + { + // If this fails, *do not* return it as an error, it just + // means that the allocation no longer exists, and since this is + // only going to produce implicit reads and not actual ones, we + // don't want that to produce errors. + if let Ok(alloc_extra) = ecx.get_alloc_extra(*alloc_id) { + if let Some(alloc_borrow_tracker) = &alloc_extra.borrow_tracker { + alloc_borrow_tracker.release_protector( + &ecx.machine, + global_borrow_tracker, + *tag, + )?; + } + } + } + global_borrow_tracker.borrow_mut().end_call(&frame.extra); } Ok(()) } diff --git a/tests/fail/tree_borrows/spurious_read.stderr b/tests/fail/tree_borrows/spurious_read.stderr new file mode 100644 index 0000000000..1453095eaf --- /dev/null +++ b/tests/fail/tree_borrows/spurious_read.stderr @@ -0,0 +1,44 @@ +Thread 1 executing: start +Thread 2 executing: start +Thread 2 executing: retag x (&mut, protect) +Thread 1 executing: retag x (&mut, protect) +Thread 1 executing: retag y (&mut, protect) +Thread 2 executing: retag y (&mut, protect) +Thread 2 executing: spurious read x +Thread 1 executing: spurious read x +Thread 1 executing: ret x +Thread 2 executing: ret x +Thread 2 executing: write y +Thread 1 executing: write y +Thread 1 executing: ret y +error: Undefined Behavior: write access through is forbidden + --> $DIR/spurious_read.rs:LL:CC + | +LL | *y = 2; + | ^^^^^^ write access through 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 Reserved (aliased) which forbids this child write access +help: the accessed tag was created here, in the initial state Reserved + --> $DIR/spurious_read.rs:LL:CC + | +LL | fn as_mut(y: &mut u8, b: (usize, Arc)) -> *mut u8 { + | ^ +help: the accessed tag later transitioned to Reserved (aliased) due to a protector release (acting as a foreign read access) over the entire allocation + --> $DIR/spurious_read.rs:LL:CC + | +LL | } + | ^ + = help: this transition corresponds to a temporary loss of write permissions until function exit + = note: BACKTRACE (of the first span): + = note: inside `retagx_retagy_spuriousx_retx_writey_rety::{closure#1}::as_mut` at $DIR/spurious_read.rs:LL:CC +note: inside closure + --> $DIR/spurious_read.rs:LL:CC + | +LL | let _y = as_mut(unsafe { &mut *ptr.0 }, b.clone()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error + diff --git a/tests/pass/tree_borrows/tree-borrows.rs b/tests/pass/tree_borrows/tree-borrows.rs index 531543441c..7e7f78d861 100644 --- a/tests/pass/tree_borrows/tree-borrows.rs +++ b/tests/pass/tree_borrows/tree-borrows.rs @@ -12,6 +12,7 @@ fn main() { two_mut_protected_same_alloc(); direct_mut_to_const_raw(); local_addr_of_mut(); + returned_mut_is_usable(); // Stacked Borrows tests read_does_not_invalidate1(); @@ -93,6 +94,21 @@ fn two_mut_protected_same_alloc() { write_second(&mut data.0, &mut data.1); } +// This checks that an Active returned from a function is actually Active. +// The fact that this is not obvious is due to the addition of +// implicit reads on function exit that might freeze the return value. +fn returned_mut_is_usable() { + fn reborrow(x: &mut u8) -> &mut u8 { + let y = &mut *x; + *y = *y; + y + } + let mut data = 0; + let x = &mut data; + let y = reborrow(x); + *y = 1; +} + // ----- The tests below were taken from Stacked Borrows ---- // Make sure that reading from an `&mut` does, like reborrowing to `&`,