Skip to content

Commit

Permalink
Auto merge of rust-lang#3732 - JoJoDeveloping:tree-borrows-protector-…
Browse files Browse the repository at this point in the history
…end-write, r=RalfJung

TB: Refine protector end semantics

Tree Borrows has protector end tag semantics, namely that protectors ending cause a [special implicit read](https://perso.crans.org/vanille/treebor/diff.0.html) on all locations protected by that protector that have actually been accessed. See also rust-lang#3067.

While this is enough for ensuring protectors allow adding/reordering reads, it does not prove that one can reorder writes. For this, we need to make this stronger, by making this implicit read be a write in cases when there was a write to the location protected by that protector, i.e. if the permission is `Active`.

There is a test that shows why this behavior is necessary, see `tests/fail/tree_borrows/protector-write-lazy.rs`.
  • Loading branch information
bors committed Jul 4, 2024
2 parents 4a4a81a + 02bec40 commit a3460e2
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 36 deletions.
10 changes: 7 additions & 3 deletions src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub enum AccessCause {
Explicit(AccessKind),
Reborrow,
Dealloc,
FnExit,
FnExit(AccessKind),
}

impl fmt::Display for AccessCause {
Expand All @@ -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"),
}
}
}
Expand All @@ -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})"),
}
}
}
Expand Down
19 changes: 4 additions & 15 deletions src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
)
}

Expand Down Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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)?;
Expand Down
4 changes: 4 additions & 0 deletions src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand Down
41 changes: 23 additions & 18 deletions src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down Expand Up @@ -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,
Expand All @@ -585,13 +587,11 @@ impl<'tcx> Tree {
/// - recording the history.
pub fn perform_access(
&mut self,
access_kind: AccessKind,
tag: BorTag,
access_range: Option<AllocRange>,
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:
Expand All @@ -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<u64>,
access_kind: AccessKind,
access_cause: diagnostics::AccessCause,
args: NodeAppArgs<'_>|
-> Result<ContinueTraversal, TransitionError> {
let NodeAppArgs { node, mut perm, rel_pos } = args;
Expand All @@ -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,
});
Expand All @@ -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<u64>,
access_cause: diagnostics::AccessCause,
args: ErrHandlerArgs<'_, TransitionError>|
-> InterpError<'tcx> {
let ErrHandlerArgs { error_kind, conflicting_info, accessed_info } = args;
Expand All @@ -650,16 +652,16 @@ 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)
{
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 {
Expand All @@ -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),
)?;
}
}
Expand Down
35 changes: 35 additions & 0 deletions src/tools/miri/tests/fail/tree_borrows/protector-write-lazy.rs
Original file line number Diff line number Diff line change
@@ -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/
}
27 changes: 27 additions & 0 deletions src/tools/miri/tests/fail/tree_borrows/protector-write-lazy.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
error: Undefined Behavior: reborrow through <TAG> 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 <TAG> 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 <TAG> has state Disabled which forbids this reborrow (acting as a child read access)
help: the accessed tag <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 <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

0 comments on commit a3460e2

Please sign in to comment.