From 4c9b6f46fe330edad53a2a067899bb01722de1f9 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 27 Dec 2022 23:06:51 +0100 Subject: [PATCH 1/3] justify return-position noalias: make Box clear stack on some retags --- compiler/rustc_middle/src/mir/mod.rs | 1 + compiler/rustc_middle/src/mir/syntax.rs | 4 +- compiler/rustc_mir_transform/src/add_retag.rs | 2 +- ...main.SimplifyCfg-elaborate-drops.after.mir | 6 +-- src/tools/miri/src/borrow_tracker/mod.rs | 2 +- .../stacked_borrows/diagnostics.rs | 4 +- .../src/borrow_tracker/stacked_borrows/mod.rs | 43 +++++++++++++++---- .../borrow_tracker/stacked_borrows/stack.rs | 17 +++++++- .../stacked_borrows/box_noalias_violation.rs | 6 ++- .../box_noalias_violation.stderr | 23 +++++----- .../deallocate_against_protector1.rs | 5 ++- .../deallocate_against_protector1.stderr | 12 ++---- .../deallocate_against_protector2.rs | 5 ++- .../deallocate_against_protector2.stderr | 12 ++---- .../fail/stacked_borrows/return-noalias.rs | 11 +++++ .../stacked_borrows/return-noalias.stderr | 24 +++++++++++ .../pass/stacked-borrows/stack-printing.rs | 2 +- .../stacked-borrows/stack-printing.stdout | 4 +- 18 files changed, 129 insertions(+), 54 deletions(-) create mode 100644 src/tools/miri/tests/fail/stacked_borrows/return-noalias.rs create mode 100644 src/tools/miri/tests/fail/stacked_borrows/return-noalias.stderr diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index a89e6566d56af..9911c84b11fcd 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -1444,6 +1444,7 @@ impl Debug for Statement<'_> { "Retag({}{:?})", match kind { RetagKind::FnEntry => "[fn entry] ", + RetagKind::FnReturn => "[fn return] ", RetagKind::TwoPhase => "[2phase] ", RetagKind::Raw => "[raw] ", RetagKind::Default => "", diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs index 6b4489026d3d3..fdca86d6472d0 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/compiler/rustc_middle/src/mir/syntax.rs @@ -323,7 +323,7 @@ pub enum StatementKind<'tcx> { /// For code that is not specific to stacked borrows, you should consider retags to read and /// modify the place in an opaque way. /// - /// Only `RetagKind::Default` and `RetagKind::FnEntry` are permitted. + /// Only `RetagKind::Default`, `RetagKind::FnEntry`, `RetagKind::FnReturn` are permitted. Retag(RetagKind, Box>), /// Encodes a user's type ascription. These need to be preserved @@ -412,6 +412,8 @@ impl std::fmt::Display for NonDivergingIntrinsic<'_> { pub enum RetagKind { /// The initial retag of arguments when entering a function. FnEntry, + /// The retag of a value that was just returned from another function. + FnReturn, /// Retag preparing for a two-phase borrow. TwoPhase, /// Retagging raw pointers. diff --git a/compiler/rustc_mir_transform/src/add_retag.rs b/compiler/rustc_mir_transform/src/add_retag.rs index 3d22035f0785e..bb1df4da6fc2d 100644 --- a/compiler/rustc_mir_transform/src/add_retag.rs +++ b/compiler/rustc_mir_transform/src/add_retag.rs @@ -112,7 +112,7 @@ impl<'tcx> MirPass<'tcx> for AddRetag { 0, Statement { source_info, - kind: StatementKind::Retag(RetagKind::Default, Box::new(dest_place)), + kind: StatementKind::Retag(RetagKind::FnReturn, Box::new(dest_place)), }, ); } diff --git a/src/test/mir-opt/retag.main.SimplifyCfg-elaborate-drops.after.mir b/src/test/mir-opt/retag.main.SimplifyCfg-elaborate-drops.after.mir index b853e45054172..a1186e730f23e 100644 --- a/src/test/mir-opt/retag.main.SimplifyCfg-elaborate-drops.after.mir +++ b/src/test/mir-opt/retag.main.SimplifyCfg-elaborate-drops.after.mir @@ -76,7 +76,7 @@ fn main() -> () { } bb1: { - Retag(_3); // scope 1 at $DIR/retag.rs:+3:17: +3:36 + Retag([fn return] _3); // scope 1 at $DIR/retag.rs:+3:17: +3:36 StorageDead(_6); // scope 1 at $DIR/retag.rs:+3:35: +3:36 StorageDead(_4); // scope 1 at $DIR/retag.rs:+3:35: +3:36 StorageDead(_7); // scope 1 at $DIR/retag.rs:+3:36: +3:37 @@ -122,7 +122,7 @@ fn main() -> () { } bb3: { - Retag(_15); // scope 6 at $DIR/retag.rs:+15:14: +15:19 + Retag([fn return] _15); // scope 6 at $DIR/retag.rs:+15:14: +15:19 StorageDead(_17); // scope 6 at $DIR/retag.rs:+15:18: +15:19 StorageDead(_16); // scope 6 at $DIR/retag.rs:+15:18: +15:19 StorageDead(_18); // scope 6 at $DIR/retag.rs:+15:19: +15:20 @@ -148,7 +148,7 @@ fn main() -> () { } bb4: { - Retag(_19); // scope 7 at $DIR/retag.rs:+18:5: +18:24 + Retag([fn return] _19); // scope 7 at $DIR/retag.rs:+18:5: +18:24 StorageDead(_22); // scope 7 at $DIR/retag.rs:+18:23: +18:24 StorageDead(_20); // scope 7 at $DIR/retag.rs:+18:23: +18:24 StorageDead(_23); // scope 7 at $DIR/retag.rs:+18:24: +18:25 diff --git a/src/tools/miri/src/borrow_tracker/mod.rs b/src/tools/miri/src/borrow_tracker/mod.rs index 9f6cbe7f3c72f..fe00dd6a094eb 100644 --- a/src/tools/miri/src/borrow_tracker/mod.rs +++ b/src/tools/miri/src/borrow_tracker/mod.rs @@ -87,7 +87,7 @@ pub struct GlobalStateInner { /// Table storing the "base" tag for each allocation. /// The base tag is the one used for the initial pointer. /// We need this in a separate table to handle cyclic statics. - pub base_ptr_tags: FxHashMap, + base_ptr_tags: FxHashMap, /// Next unused call ID (for protectors). pub next_call_id: CallId, /// All currently protected tags. diff --git a/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs b/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs index 5f132bf11a92a..71935bcfe5e2d 100644 --- a/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs +++ b/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs @@ -193,7 +193,7 @@ struct RetagOp { #[derive(Debug, Clone, Copy, PartialEq)] pub enum RetagCause { Normal, - FnReturn, + FnReturnPlace, FnEntry, TwoPhase, } @@ -496,7 +496,7 @@ impl RetagCause { match self { RetagCause::Normal => "retag", RetagCause::FnEntry => "FnEntry retag", - RetagCause::FnReturn => "FnReturn retag", + RetagCause::FnReturnPlace => "return-place retag", RetagCause::TwoPhase => "two-phase retag", } .to_string() diff --git a/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs b/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs index bcdf2e751790e..acb8e6373f2fd 100644 --- a/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs @@ -48,6 +48,8 @@ pub struct Stacks { enum NewPermission { Uniform { perm: Permission, + /// If true, indicates that the stack should be cleared of all other tags. + clear: bool, access: Option, protector: Option, }, @@ -77,6 +79,7 @@ impl NewPermission { assert!(protector.is_none()); // RetagKind can't be both FnEntry and TwoPhase. NewPermission::Uniform { perm: Permission::SharedReadWrite, + clear: false, access: None, protector: None, } @@ -84,12 +87,14 @@ impl NewPermission { // A regular full mutable reference. NewPermission::Uniform { perm: Permission::Unique, + clear: false, access: Some(AccessKind::Write), protector, } } else { NewPermission::Uniform { perm: Permission::SharedReadWrite, + clear: false, // FIXME: We emit `dereferenceable` for `!Unpin` mutable references, so we // should do fake accesses here. But then we run into // , so for now @@ -104,6 +109,7 @@ impl NewPermission { // Mutable raw pointer. No access, not protected. NewPermission::Uniform { perm: Permission::SharedReadWrite, + clear: false, access: None, protector: None, } @@ -371,11 +377,13 @@ impl<'tcx> Stack { /// Derive a new pointer from one with the given tag. /// + /// `clear` indicates whether everything else should be removed from the stack. /// `access` indicates which kind of memory access this retag itself should correspond to. fn grant( &mut self, derived_from: ProvenanceExtra, new: Item, + clear: bool, access: Option, global: &GlobalStateInner, dcx: &mut DiagnosticCx<'_, '_, '_, 'tcx>, @@ -392,6 +400,15 @@ impl<'tcx> Stack { // This ensures F2b for `Unique`, by removing offending `SharedReadOnly`. self.access(access, derived_from, global, dcx, exposed_tags)?; + if clear { + // Remove all items, also checking their protectors. + for idx in (0..self.len()).rev() { + let item = self.get(idx).unwrap(); + Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Conflict)?; + } + self.clear(); + } + // We insert "as far up as possible": We know only compatible items are remaining // on top of `derived_from`, and we want the new item at the top so that we // get the strongest possible guarantees. @@ -400,6 +417,7 @@ impl<'tcx> Stack { } else { // The tricky case: creating a new SRW permission without actually being an access. assert!(new.perm() == Permission::SharedReadWrite); + assert!(!clear); // First we figure out which item grants our parent (`derived_from`) this kind of access. // We use that to determine where to put the new item. @@ -723,7 +741,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' // Update the stacks, according to the new permission information we are given. match new_perm { - NewPermission::Uniform { perm, access, protector } => { + NewPermission::Uniform { perm, clear, access, protector } => { assert!(perm != Permission::SharedReadOnly); // Here we can avoid `borrow()` calls because we have mutable references. // Note that this asserts that the allocation is mutable -- but since we are creating a @@ -741,7 +759,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' alloc_range(base_offset, size), ); stacked_borrows.for_each(range, dcx, |stack, dcx, exposed_tags| { - stack.grant(orig_tag, item, access, &global, dcx, exposed_tags) + stack.grant(orig_tag, item, clear, access, &global, dcx, exposed_tags) })?; drop(global); if let Some(access) = access { @@ -784,7 +802,8 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' alloc_range(base_offset, size), ); stacked_borrows.for_each(range, dcx, |stack, dcx, exposed_tags| { - stack.grant(orig_tag, item, access, &global, dcx, exposed_tags) + let clear = false; + stack.grant(orig_tag, item, clear, access, &global, dcx, exposed_tags) })?; drop(global); if let Some(access) = access { @@ -862,7 +881,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let new_perm = NewPermission::from_ref_ty(val.layout.ty, kind, this); let retag_cause = match kind { RetagKind::TwoPhase { .. } => RetagCause::TwoPhase, - RetagKind::FnEntry => unreachable!(), + RetagKind::FnEntry | RetagKind::FnReturn => unreachable!(), RetagKind::Raw | RetagKind::Default => RetagCause::Normal, }; this.sb_retag_reference(val, new_perm, retag_cause) @@ -878,7 +897,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let retag_cause = match kind { RetagKind::Raw | RetagKind::TwoPhase { .. } => unreachable!(), // these can only happen in `retag_ptr_value` RetagKind::FnEntry => RetagCause::FnEntry, - RetagKind::Default => RetagCause::Normal, + RetagKind::Default | RetagKind::FnReturn => RetagCause::Normal, }; let mut visitor = RetagVisitor { ecx: this, kind, retag_cause, retag_fields }; return visitor.visit_value(place); @@ -915,12 +934,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } fn visit_box(&mut self, place: &PlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> { - // Boxes get a weak protectors, since they may be deallocated. + // We cannot protect boxes since they might get invalidated if passed to an `id` + // function (due to the return-position `noalias`). However, doing a `clear` on + // `FnEntry` also has the effect that using any other pointer to access this memory + // is *immediate* UB. + // FIXME: should we `clear` on *all* `Box` retags? Currently (2022-12-27) this leads + // to UB somewhere in thread-local dtor handling. let new_perm = NewPermission::Uniform { perm: Permission::Unique, + clear: matches!(self.kind, RetagKind::FnEntry | RetagKind::FnReturn), access: Some(AccessKind::Write), - protector: (self.kind == RetagKind::FnEntry) - .then_some(ProtectorKind::WeakProtector), + protector: None, }; self.retag_ptr_inplace(place, new_perm, self.retag_cause) } @@ -995,10 +1019,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // Reborrow it. With protection! That is part of the point. let new_perm = NewPermission::Uniform { perm: Permission::Unique, + clear: false, access: Some(AccessKind::Write), protector: Some(ProtectorKind::StrongProtector), }; - let val = this.sb_retag_reference(&val, new_perm, RetagCause::FnReturn)?; + let val = this.sb_retag_reference(&val, new_perm, RetagCause::FnReturnPlace)?; // And use reborrowed pointer for return place. let return_place = this.ref_to_mplace(&val)?; this.frame_mut().return_place = return_place.into(); diff --git a/src/tools/miri/src/borrow_tracker/stacked_borrows/stack.rs b/src/tools/miri/src/borrow_tracker/stacked_borrows/stack.rs index 1d5cfec3500ae..2848a90065c33 100644 --- a/src/tools/miri/src/borrow_tracker/stacked_borrows/stack.rs +++ b/src/tools/miri/src/borrow_tracker/stacked_borrows/stack.rs @@ -252,8 +252,8 @@ impl<'tcx> Stack { // and this check actually ensures we do not access an invalid cache. // When a stack is created and when items are removed from the top of the borrow stack, we // need some valid value to populate the cache. In both cases, we try to use the bottom - // item. But when the stack is cleared in `set_unknown_bottom` there is nothing we could - // place in the cache that is correct. But due to the way we populate the cache in + // item. But when the stack is cleared in `clear` or `set_unknown_bottom` there is nothing + // we could place in the cache that is correct. But due to the way we populate the cache in // `StackCache::add`, we know that when the borrow stack has grown larger than the cache, // every slot in the cache is valid. if self.borrows.len() <= CACHE_LEN { @@ -368,6 +368,19 @@ impl<'tcx> Stack { } } + // Empty the stack. + pub fn clear(&mut self) { + // We clear the borrow stack but the lookup cache doesn't support clearing per se. Instead, + // there is a check explained in `find_granting_cache` which protects against accessing the + // cache when it has been cleared and not yet refilled. + self.borrows.clear(); + self.unknown_bottom = None; + #[cfg(feature = "stack-cache")] + { + self.unique_range = 0..0; + } + } + /// Find all `Unique` elements in this borrow stack above `granting_idx`, pass a copy of them /// to the `visitor`, then set their `Permission` to `Disabled`. pub fn disable_uniques_starting_at( diff --git a/src/tools/miri/tests/fail/stacked_borrows/box_noalias_violation.rs b/src/tools/miri/tests/fail/stacked_borrows/box_noalias_violation.rs index 2067149b6c87c..208f0d4a15233 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/box_noalias_violation.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/box_noalias_violation.rs @@ -2,13 +2,15 @@ unsafe fn test(mut x: Box, y: *const i32) -> i32 { // We will call this in a way that x and y alias. *x = 5; std::mem::forget(x); - *y //~ERROR: weakly protected + *y //~ERROR: does not exist in the borrow stack } fn main() { unsafe { let mut v = 42; let ptr = &mut v as *mut i32; - test(Box::from_raw(ptr), ptr); + let b = Box::from_raw(ptr); + let ptr = &*b as *const i32; + test(b, ptr); } } diff --git a/src/tools/miri/tests/fail/stacked_borrows/box_noalias_violation.stderr b/src/tools/miri/tests/fail/stacked_borrows/box_noalias_violation.stderr index 59377aeb971a8..7cf934e60c497 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/box_noalias_violation.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/box_noalias_violation.stderr @@ -1,28 +1,31 @@ -error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is weakly protected because it is an argument of call ID +error: Undefined Behavior: attempting a read access using at ALLOC[0x0], but that tag does not exist in the borrow stack for this location --> $DIR/box_noalias_violation.rs:LL:CC | LL | *y - | ^^ not granting access to tag because that would remove [Unique for ] which is weakly protected because it is an argument of call ID + | ^^ + | | + | attempting a read access using at ALLOC[0x0], but that tag does not exist in the borrow stack for this location + | this error occurs as part of an access at ALLOC[0x0..0x4] | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information -help: was created by a SharedReadWrite retag at offsets [0x0..0x4] +help: was created by a SharedReadOnly retag at offsets [0x0..0x4] --> $DIR/box_noalias_violation.rs:LL:CC | -LL | let ptr = &mut v as *mut i32; - | ^^^^^^ -help: is this argument +LL | let ptr = &*b as *const i32; + | ^^^ +help: was later invalidated at offsets [0x0..0x4] by a Unique retag --> $DIR/box_noalias_violation.rs:LL:CC | -LL | unsafe fn test(mut x: Box, y: *const i32) -> i32 { - | ^^^^^ +LL | test(b, ptr); + | ^ = note: BACKTRACE (of the first span): = note: inside `test` at $DIR/box_noalias_violation.rs:LL:CC note: inside `main` --> $DIR/box_noalias_violation.rs:LL:CC | -LL | test(Box::from_raw(ptr), ptr); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | test(b, ptr); + | ^^^^^^^^^^^^ note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.rs b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.rs index 4036dce5bebab..6dc8bfcf80c4a 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.rs @@ -1,4 +1,5 @@ //@error-pattern: /deallocating while item \[Unique for .*\] is strongly protected/ +use std::alloc::{dealloc, Layout}; fn inner(x: &mut i32, f: fn(&mut i32)) { // `f` may mutate, but it may not deallocate! @@ -7,7 +8,7 @@ fn inner(x: &mut i32, f: fn(&mut i32)) { fn main() { inner(Box::leak(Box::new(0)), |x| { - let raw = x as *mut _; - drop(unsafe { Box::from_raw(raw) }); + let raw = x as *mut i32 as *mut u8; + drop(unsafe { dealloc(raw, Layout::new::()) }); }); } diff --git a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.stderr b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.stderr index 516964b9a4e6f..dd56f766a2858 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.stderr @@ -8,15 +8,11 @@ LL | unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) } = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information = note: BACKTRACE: = note: inside `std::alloc::dealloc` at RUSTLIB/alloc/src/alloc.rs:LL:CC - = note: inside `::deallocate` at RUSTLIB/alloc/src/alloc.rs:LL:CC - = note: inside `alloc::alloc::box_free::` at RUSTLIB/alloc/src/alloc.rs:LL:CC - = note: inside `std::ptr::drop_in_place::> - shim(Some(std::boxed::Box))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC - = note: inside `std::mem::drop::>` at RUSTLIB/core/src/mem/mod.rs:LL:CC note: inside closure --> $DIR/deallocate_against_protector1.rs:LL:CC | -LL | drop(unsafe { Box::from_raw(raw) }); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | drop(unsafe { dealloc(raw, Layout::new::()) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = note: inside `<[closure@$DIR/deallocate_against_protector1.rs:LL:CC] as std::ops::FnOnce<(&mut i32,)>>::call_once - shim` at RUSTLIB/core/src/ops/function.rs:LL:CC note: inside `inner` --> $DIR/deallocate_against_protector1.rs:LL:CC @@ -27,8 +23,8 @@ note: inside `main` --> $DIR/deallocate_against_protector1.rs:LL:CC | LL | / inner(Box::leak(Box::new(0)), |x| { -LL | | let raw = x as *mut _; -LL | | drop(unsafe { Box::from_raw(raw) }); +LL | | let raw = x as *mut i32 as *mut u8; +LL | | drop(unsafe { dealloc(raw, Layout::new::()) }); LL | | }); | |______^ diff --git a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.rs b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.rs index fd67dccd14df6..b61a191186d89 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.rs @@ -1,4 +1,5 @@ //@error-pattern: /deallocating while item \[SharedReadWrite for .*\] is strongly protected/ +use std::alloc::{dealloc, Layout}; use std::marker::PhantomPinned; pub struct NotUnpin(i32, PhantomPinned); @@ -10,7 +11,7 @@ fn inner(x: &mut NotUnpin, f: fn(&mut NotUnpin)) { fn main() { inner(Box::leak(Box::new(NotUnpin(0, PhantomPinned))), |x| { - let raw = x as *mut _; - drop(unsafe { Box::from_raw(raw) }); + let raw = x as *mut NotUnpin as *mut u8; + drop(unsafe { dealloc(raw, Layout::new::()) }); }); } diff --git a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.stderr b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.stderr index 47cfa0de7258c..082d5b92327f9 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.stderr @@ -8,15 +8,11 @@ LL | unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) } = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information = note: BACKTRACE: = note: inside `std::alloc::dealloc` at RUSTLIB/alloc/src/alloc.rs:LL:CC - = note: inside `::deallocate` at RUSTLIB/alloc/src/alloc.rs:LL:CC - = note: inside `alloc::alloc::box_free::` at RUSTLIB/alloc/src/alloc.rs:LL:CC - = note: inside `std::ptr::drop_in_place::> - shim(Some(std::boxed::Box))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC - = note: inside `std::mem::drop::>` at RUSTLIB/core/src/mem/mod.rs:LL:CC note: inside closure --> $DIR/deallocate_against_protector2.rs:LL:CC | -LL | drop(unsafe { Box::from_raw(raw) }); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | drop(unsafe { dealloc(raw, Layout::new::()) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = note: inside `<[closure@$DIR/deallocate_against_protector2.rs:LL:CC] as std::ops::FnOnce<(&mut NotUnpin,)>>::call_once - shim` at RUSTLIB/core/src/ops/function.rs:LL:CC note: inside `inner` --> $DIR/deallocate_against_protector2.rs:LL:CC @@ -27,8 +23,8 @@ note: inside `main` --> $DIR/deallocate_against_protector2.rs:LL:CC | LL | / inner(Box::leak(Box::new(NotUnpin(0, PhantomPinned))), |x| { -LL | | let raw = x as *mut _; -LL | | drop(unsafe { Box::from_raw(raw) }); +LL | | let raw = x as *mut NotUnpin as *mut u8; +LL | | drop(unsafe { dealloc(raw, Layout::new::()) }); LL | | }); | |______^ diff --git a/src/tools/miri/tests/fail/stacked_borrows/return-noalias.rs b/src/tools/miri/tests/fail/stacked_borrows/return-noalias.rs new file mode 100644 index 0000000000000..3d6d90b36d385 --- /dev/null +++ b/src/tools/miri/tests/fail/stacked_borrows/return-noalias.rs @@ -0,0 +1,11 @@ +fn id(x: Box) -> Box { x } + +fn main() { + let mut m = 0; + let b = unsafe { Box::from_raw(&mut m) }; + let mut b2 = id(b); + // Since `id` returns a `Box`, this should invalidate all other pointers to this memory. + *b2 = 5; + std::mem::forget(b2); + println!("{}", m); //~ERROR: tag does not exist in the borrow stack +} diff --git a/src/tools/miri/tests/fail/stacked_borrows/return-noalias.stderr b/src/tools/miri/tests/fail/stacked_borrows/return-noalias.stderr new file mode 100644 index 0000000000000..f12ee2791a4bb --- /dev/null +++ b/src/tools/miri/tests/fail/stacked_borrows/return-noalias.stderr @@ -0,0 +1,24 @@ +error: Undefined Behavior: trying to retag from for SharedReadOnly permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location + --> $DIR/return-noalias.rs:LL:CC + | +LL | println!("{}", m); + | ^ + | | + | trying to retag from for SharedReadOnly permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location + | this error occurs as part of retag at ALLOC[0x0..0x4] + | + = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental + = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information +help: was created here, as the base tag for ALLOC + --> $DIR/return-noalias.rs:LL:CC + | +LL | let b = unsafe { Box::from_raw(&mut m) }; + | ^^^^^^ + = note: BACKTRACE (of the first span): + = note: inside `main` at $DIR/return-noalias.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 previous error + diff --git a/src/tools/miri/tests/pass/stacked-borrows/stack-printing.rs b/src/tools/miri/tests/pass/stacked-borrows/stack-printing.rs index 3ca937ae13db8..35ec88a484644 100644 --- a/src/tools/miri/tests/pass/stacked-borrows/stack-printing.rs +++ b/src/tools/miri/tests/pass/stacked-borrows/stack-printing.rs @@ -29,7 +29,7 @@ fn main() { unsafe { *ptr = 42 }; print_borrow_stacks(alloc_id); - let _b = unsafe { ManuallyDrop::new(Box::from_raw(ptr)) }; + let _b = unsafe { &mut *ptr }; print_borrow_stacks(alloc_id); let _ptr = unsafe { &*ptr }; diff --git a/src/tools/miri/tests/pass/stacked-borrows/stack-printing.stdout b/src/tools/miri/tests/pass/stacked-borrows/stack-printing.stdout index 296339e738455..479a946a181e7 100644 --- a/src/tools/miri/tests/pass/stacked-borrows/stack-printing.stdout +++ b/src/tools/miri/tests/pass/stacked-borrows/stack-printing.stdout @@ -1,6 +1,6 @@ 0..1: [ SharedReadWrite ] 0..1: [ SharedReadWrite ] 0..1: [ SharedReadWrite ] -0..1: [ SharedReadWrite Unique Unique Unique Unique Unique Unique Unique ] -0..1: [ SharedReadWrite Disabled Disabled Disabled Disabled Disabled Disabled Disabled SharedReadOnly ] +0..1: [ SharedReadWrite Unique Unique ] +0..1: [ SharedReadWrite Disabled Disabled SharedReadOnly ] 0..1: [ unknown-bottom(..) ] From a00de3de7478c5b0f2ea17f5df93ba06daebf62f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 28 Dec 2022 11:45:19 +0100 Subject: [PATCH 2/3] now we do not need weak protectors any more --- src/tools/miri/src/borrow_tracker/mod.rs | 24 +------ .../stacked_borrows/diagnostics.rs | 13 ++-- .../src/borrow_tracker/stacked_borrows/mod.rs | 70 +++++++------------ .../fail/stacked_borrows/aliasing_mut1.stderr | 4 +- .../fail/stacked_borrows/aliasing_mut2.stderr | 4 +- .../fail/stacked_borrows/aliasing_mut4.stderr | 4 +- .../deallocate_against_protector1.rs | 2 +- .../deallocate_against_protector1.stderr | 4 +- .../deallocate_against_protector2.rs | 2 +- .../deallocate_against_protector2.stderr | 4 +- .../drop_in_place_protector.rs | 2 +- .../drop_in_place_protector.stderr | 4 +- .../fail/stacked_borrows/illegal_write6.rs | 2 +- .../stacked_borrows/illegal_write6.stderr | 4 +- .../invalidate_against_protector1.stderr | 4 +- .../invalidate_against_protector2.stderr | 4 +- .../invalidate_against_protector3.stderr | 4 +- .../stacked_borrows/newtype_pair_retagging.rs | 2 +- .../newtype_pair_retagging.stderr | 4 +- .../fail/stacked_borrows/newtype_retagging.rs | 2 +- .../stacked_borrows/newtype_retagging.stderr | 4 +- 21 files changed, 62 insertions(+), 105 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/mod.rs b/src/tools/miri/src/borrow_tracker/mod.rs index fe00dd6a094eb..f9fc199c60043 100644 --- a/src/tools/miri/src/borrow_tracker/mod.rs +++ b/src/tools/miri/src/borrow_tracker/mod.rs @@ -95,7 +95,7 @@ pub struct GlobalStateInner { /// We add tags to this when they are created with a protector in `reborrow`, and /// we remove tags from this when the call which is protecting them returns, in /// `GlobalStateInner::end_call`. See `Stack::item_popped` for more details. - pub protected_tags: FxHashMap, + pub protected_tags: FxHashSet, /// The pointer ids to trace pub tracked_pointer_tags: FxHashSet, /// The call ids to trace @@ -142,26 +142,6 @@ pub enum RetagFields { OnlyScalar, } -/// The flavor of the protector. -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub enum ProtectorKind { - /// Protected against aliasing violations from other pointers. - /// - /// Items protected like this cause UB when they are invalidated, *but* the pointer itself may - /// still be used to issue a deallocation. - /// - /// This is required for LLVM IR pointers that are `noalias` but *not* `dereferenceable`. - WeakProtector, - - /// Protected against any kind of invalidation. - /// - /// Items protected like this cause UB when they are invalidated or the memory is deallocated. - /// This is strictly stronger protection than `WeakProtector`. - /// - /// This is required for LLVM IR pointers that are `dereferenceable` (and also allows `noalias`). - StrongProtector, -} - /// Utilities for initialization and ID generation impl GlobalStateInner { pub fn new( @@ -175,7 +155,7 @@ impl GlobalStateInner { next_ptr_tag: BorTag::one(), base_ptr_tags: FxHashMap::default(), next_call_id: NonZeroU64::new(1).unwrap(), - protected_tags: FxHashMap::default(), + protected_tags: FxHashSet::default(), tracked_pointer_tags, tracked_call_ids, retag_fields, diff --git a/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs b/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs index 71935bcfe5e2d..ec9dc5cac5cf1 100644 --- a/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs +++ b/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs @@ -6,8 +6,7 @@ use rustc_span::{Span, SpanData}; use rustc_target::abi::Size; use crate::borrow_tracker::{ - stacked_borrows::{err_sb_ub, Permission}, - AccessKind, GlobalStateInner, ProtectorKind, + stacked_borrows::{err_sb_ub, Permission}, AccessKind, GlobalStateInner, }; use crate::*; @@ -398,11 +397,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { } #[inline(never)] // This is only called on fatal code paths - pub(super) fn protector_error(&self, item: &Item, kind: ProtectorKind) -> InterpError<'tcx> { - let protected = match kind { - ProtectorKind::WeakProtector => "weakly protected", - ProtectorKind::StrongProtector => "strongly protected", - }; + pub(super) fn protector_error(&self, item: &Item) -> InterpError<'tcx> { let call_id = self .machine .threads @@ -417,7 +412,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { match self.operation { Operation::Dealloc(_) => err_sb_ub( - format!("deallocating while item {item:?} is {protected} by call {call_id:?}",), + format!("deallocating while item {item:?} is protected by call {call_id:?}",), None, None, ), @@ -425,7 +420,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { | Operation::Access(AccessOp { tag, .. }) => err_sb_ub( format!( - "not granting access to tag {tag:?} because that would remove {item:?} which is {protected} because it is an argument of call {call_id:?}", + "not granting access to tag {tag:?} because that would remove {item:?} which is protected because it is an argument of call {call_id:?}", ), None, tag.and_then(|tag| self.get_logs_relevant_to(tag, Some(item.tag()))), diff --git a/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs b/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs index acb8e6373f2fd..e97835a9aa1bd 100644 --- a/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs @@ -20,7 +20,7 @@ use rustc_target::abi::{Abi, Size}; use crate::borrow_tracker::{ stacked_borrows::diagnostics::{AllocHistory, DiagnosticCx, DiagnosticCxBuilder, TagHistory}, - AccessKind, GlobalStateInner, ProtectorKind, RetagFields, + AccessKind, GlobalStateInner, RetagFields, }; use crate::*; @@ -51,12 +51,12 @@ enum NewPermission { /// If true, indicates that the stack should be cleared of all other tags. clear: bool, access: Option, - protector: Option, + protector: bool, }, FreezeSensitive { freeze_perm: Permission, freeze_access: Option, - freeze_protector: Option, + freeze_protector: bool, nonfreeze_perm: Permission, nonfreeze_access: Option, // nonfreeze_protector must always be None @@ -71,17 +71,15 @@ impl NewPermission { kind: RetagKind, cx: &crate::MiriInterpCx<'_, 'tcx>, ) -> Self { - let protector = (kind == RetagKind::FnEntry).then_some(ProtectorKind::StrongProtector); match ty.kind() { ty::Ref(_, pointee, Mutability::Mut) => { if kind == RetagKind::TwoPhase { // We mostly just give up on 2phase-borrows, and treat these exactly like raw pointers. - assert!(protector.is_none()); // RetagKind can't be both FnEntry and TwoPhase. NewPermission::Uniform { perm: Permission::SharedReadWrite, clear: false, access: None, - protector: None, + protector: false, } } else if pointee.is_unpin(*cx.tcx, cx.param_env()) { // A regular full mutable reference. @@ -89,7 +87,7 @@ impl NewPermission { perm: Permission::Unique, clear: false, access: Some(AccessKind::Write), - protector, + protector: kind == RetagKind::FnEntry, } } else { NewPermission::Uniform { @@ -100,25 +98,24 @@ impl NewPermission { // , so for now // we don't do that. access: None, - protector, + protector: kind == RetagKind::FnEntry, } } } ty::RawPtr(ty::TypeAndMut { mutbl: Mutability::Mut, .. }) => { - assert!(protector.is_none()); // RetagKind can't be both FnEntry and Raw. // Mutable raw pointer. No access, not protected. NewPermission::Uniform { perm: Permission::SharedReadWrite, clear: false, access: None, - protector: None, + protector: false, } } ty::Ref(_, _pointee, Mutability::Not) => { NewPermission::FreezeSensitive { freeze_perm: Permission::SharedReadOnly, freeze_access: Some(AccessKind::Read), - freeze_protector: protector, + freeze_protector: kind == RetagKind::FnEntry, nonfreeze_perm: Permission::SharedReadWrite, // Inside UnsafeCell, this does *not* count as an access, as there // might actually be mutable references further up the stack that @@ -129,12 +126,11 @@ impl NewPermission { } } ty::RawPtr(ty::TypeAndMut { mutbl: Mutability::Not, .. }) => { - assert!(protector.is_none()); // RetagKind can't be both FnEntry and Raw. // `*const T`, when freshly created, are read-only in the frozen part. NewPermission::FreezeSensitive { freeze_perm: Permission::SharedReadOnly, freeze_access: Some(AccessKind::Read), - freeze_protector: None, + freeze_protector: false, nonfreeze_perm: Permission::SharedReadWrite, nonfreeze_access: None, } @@ -143,7 +139,7 @@ impl NewPermission { } } - fn protector(&self) -> Option { + fn protector(&self) -> bool { match self { NewPermission::Uniform { protector, .. } => *protector, NewPermission::FreezeSensitive { freeze_protector, .. } => *freeze_protector, @@ -186,13 +182,6 @@ impl Permission { } } -/// Determines whether an item was invalidated by a conflicting access, or by deallocation. -#[derive(Copy, Clone, Debug)] -enum ItemInvalidationCause { - Conflict, - Dealloc, -} - /// Core per-location operations: access, dealloc, reborrow. impl<'tcx> Stack { /// Find the first write-incompatible item above the given one -- @@ -228,7 +217,6 @@ impl<'tcx> Stack { item: &Item, global: &GlobalStateInner, dcx: &mut DiagnosticCx<'_, '_, '_, 'tcx>, - cause: ItemInvalidationCause, ) -> InterpResult<'tcx> { if !global.tracked_pointer_tags.is_empty() { dcx.check_tracked_tag_popped(item, global); @@ -251,14 +239,8 @@ impl<'tcx> Stack { // 2. Most frames protect only one or two tags. So this duplicative global turns a search // which ends up about linear in the number of protected tags in the program into a // constant time check (and a slow linear, because the tags in the frames aren't contiguous). - if let Some(&protector_kind) = global.protected_tags.get(&item.tag()) { - // The only way this is okay is if the protector is weak and we are deallocating with - // the right pointer. - let allowed = matches!(cause, ItemInvalidationCause::Dealloc) - && matches!(protector_kind, ProtectorKind::WeakProtector); - if !allowed { - return Err(dcx.protector_error(item, protector_kind).into()); - } + if global.protected_tags.contains(&item.tag()) { + return Err(dcx.protector_error(item).into()); } Ok(()) } @@ -298,7 +280,7 @@ impl<'tcx> Stack { 0 }; self.pop_items_after(first_incompatible_idx, |item| { - Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Conflict)?; + Stack::item_invalidated(&item, global, dcx)?; dcx.log_invalidation(item.tag()); Ok(()) })?; @@ -319,7 +301,7 @@ impl<'tcx> Stack { 0 }; self.disable_uniques_starting_at(first_incompatible_idx, |item| { - Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Conflict)?; + Stack::item_invalidated(&item, global, dcx)?; dcx.log_invalidation(item.tag()); Ok(()) })?; @@ -366,10 +348,10 @@ impl<'tcx> Stack { // As part of this we do regular protector checking, i.e. even weakly protected items cause UB when popped. self.access(AccessKind::Write, tag, global, dcx, exposed_tags)?; - // Step 2: Pretend we remove the remaining items, checking if any are strongly protected. + // Step 2: Pretend we remove the remaining items, checking if any are protected. for idx in (0..self.len()).rev() { let item = self.get(idx).unwrap(); - Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Dealloc)?; + Stack::item_invalidated(&item, global, dcx)?; } Ok(()) @@ -404,7 +386,7 @@ impl<'tcx> Stack { // Remove all items, also checking their protectors. for idx in (0..self.len()).rev() { let item = self.get(idx).unwrap(); - Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Conflict)?; + Stack::item_invalidated(&item, global, dcx)?; } self.clear(); } @@ -669,7 +651,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' ); let mut dcx = dcx.build(&mut stacked_borrows.history, base_offset); dcx.log_creation(); - if new_perm.protector().is_some() { + if new_perm.protector() { dcx.log_protector(); } }, @@ -727,7 +709,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' size.bytes() ); - if let Some(protect) = new_perm.protector() { + if 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.machine @@ -736,7 +718,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' .unwrap() .get_mut() .protected_tags - .insert(new_tag, protect); + .insert(new_tag); } // Update the stacks, according to the new permission information we are given. @@ -748,7 +730,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' // mutable pointer, that seems reasonable. let (alloc_extra, machine) = this.get_alloc_extra_mut(alloc_id)?; let stacked_borrows = alloc_extra.borrow_tracker_sb_mut().get_mut(); - let item = Item::new(new_tag, perm, protector.is_some()); + let item = Item::new(new_tag, perm, protector); let range = alloc_range(base_offset, size); let global = machine.borrow_tracker.as_ref().unwrap().borrow(); let dcx = DiagnosticCxBuilder::retag( @@ -790,9 +772,9 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' let (perm, access, protector) = if frozen { (freeze_perm, freeze_access, freeze_protector) } else { - (nonfreeze_perm, nonfreeze_access, None) + (nonfreeze_perm, nonfreeze_access, /*protector*/ false) }; - let item = Item::new(new_tag, perm, protector.is_some()); + let item = Item::new(new_tag, perm, protector); let global = this.machine.borrow_tracker.as_ref().unwrap().borrow(); let dcx = DiagnosticCxBuilder::retag( &this.machine, @@ -937,14 +919,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // We cannot protect boxes since they might get invalidated if passed to an `id` // function (due to the return-position `noalias`). However, doing a `clear` on // `FnEntry` also has the effect that using any other pointer to access this memory - // is *immediate* UB. + // is *immediate* UB, thus ensuring `noalias` scoped for this function. // FIXME: should we `clear` on *all* `Box` retags? Currently (2022-12-27) this leads // to UB somewhere in thread-local dtor handling. let new_perm = NewPermission::Uniform { perm: Permission::Unique, clear: matches!(self.kind, RetagKind::FnEntry | RetagKind::FnReturn), access: Some(AccessKind::Write), - protector: None, + protector: false, }; self.retag_ptr_inplace(place, new_perm, self.retag_cause) } @@ -1021,7 +1003,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { perm: Permission::Unique, clear: false, access: Some(AccessKind::Write), - protector: Some(ProtectorKind::StrongProtector), + protector: true, }; let val = this.sb_retag_reference(&val, new_perm, RetagCause::FnReturnPlace)?; // And use reborrowed pointer for return place. diff --git a/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut1.stderr b/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut1.stderr index 3ce39968cbb13..6378b8d9dddb2 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut1.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut1.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID +error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is protected because it is an argument of call ID --> $DIR/aliasing_mut1.rs:LL:CC | LL | pub fn safe(_x: &mut i32, _y: &mut i32) {} - | ^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + | ^^ not granting access to tag because that would remove [Unique for ] which is protected because it is an argument of call ID | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information diff --git a/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut2.stderr b/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut2.stderr index df4b6cf02561c..8dfbe78a69ed0 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut2.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut2.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: not granting access to tag because that would remove [SharedReadOnly for ] which is strongly protected because it is an argument of call ID +error: Undefined Behavior: not granting access to tag because that would remove [SharedReadOnly for ] which is protected because it is an argument of call ID --> $DIR/aliasing_mut2.rs:LL:CC | LL | pub fn safe(_x: &i32, _y: &mut i32) {} - | ^^ not granting access to tag because that would remove [SharedReadOnly for ] which is strongly protected because it is an argument of call ID + | ^^ not granting access to tag because that would remove [SharedReadOnly for ] which is protected because it is an argument of call ID | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information diff --git a/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut4.stderr b/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut4.stderr index ddf197bc63955..6f83887e3de51 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut4.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut4.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: not granting access to tag because that would remove [SharedReadOnly for ] which is strongly protected because it is an argument of call ID +error: Undefined Behavior: not granting access to tag because that would remove [SharedReadOnly for ] which is protected because it is an argument of call ID --> $DIR/aliasing_mut4.rs:LL:CC | LL | pub fn safe(_x: &i32, _y: &mut Cell) {} - | ^^ not granting access to tag because that would remove [SharedReadOnly for ] which is strongly protected because it is an argument of call ID + | ^^ not granting access to tag because that would remove [SharedReadOnly for ] which is protected because it is an argument of call ID | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information diff --git a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.rs b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.rs index 6dc8bfcf80c4a..a63460a8281c3 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.rs @@ -1,4 +1,4 @@ -//@error-pattern: /deallocating while item \[Unique for .*\] is strongly protected/ +//@error-pattern: /deallocating while item \[Unique for .*\] is protected/ use std::alloc::{dealloc, Layout}; fn inner(x: &mut i32, f: fn(&mut i32)) { diff --git a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.stderr b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.stderr index dd56f766a2858..64589fa7e79ee 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector1.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: deallocating while item [Unique for ] is strongly protected by call ID +error: Undefined Behavior: deallocating while item [Unique for ] is protected by call ID --> RUSTLIB/alloc/src/alloc.rs:LL:CC | LL | unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) } - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ deallocating while item [Unique for ] is strongly protected by call ID + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ deallocating while item [Unique for ] is protected by call ID | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information diff --git a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.rs b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.rs index b61a191186d89..dc8736890babb 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.rs @@ -1,4 +1,4 @@ -//@error-pattern: /deallocating while item \[SharedReadWrite for .*\] is strongly protected/ +//@error-pattern: /deallocating while item \[SharedReadWrite for .*\] is protected/ use std::alloc::{dealloc, Layout}; use std::marker::PhantomPinned; diff --git a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.stderr b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.stderr index 082d5b92327f9..9ef46529dd58f 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: deallocating while item [SharedReadWrite for ] is strongly protected by call ID +error: Undefined Behavior: deallocating while item [SharedReadWrite for ] is protected by call ID --> RUSTLIB/alloc/src/alloc.rs:LL:CC | LL | unsafe { __rust_dealloc(ptr, layout.size(), layout.align()) } - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ deallocating while item [SharedReadWrite for ] is strongly protected by call ID + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ deallocating while item [SharedReadWrite for ] is protected by call ID | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information diff --git a/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_protector.rs b/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_protector.rs index 8cf63ee700b85..883361d05fc4d 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_protector.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_protector.rs @@ -10,7 +10,7 @@ impl Drop for HasDrop { fn drop(&mut self) { unsafe { let _val = *P; - //~^ ERROR: /not granting access .* because that would remove .* which is strongly protected/ + //~^ ERROR: /not granting access .* because that would remove .* which is protected/ } } } diff --git a/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_protector.stderr b/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_protector.stderr index 3d0cef241c3e6..1e65f3e2db1d3 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_protector.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/drop_in_place_protector.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID +error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is protected because it is an argument of call ID --> $DIR/drop_in_place_protector.rs:LL:CC | LL | let _val = *P; - | ^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + | ^^ not granting access to tag because that would remove [Unique for ] which is protected because it is an argument of call ID | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information diff --git a/src/tools/miri/tests/fail/stacked_borrows/illegal_write6.rs b/src/tools/miri/tests/fail/stacked_borrows/illegal_write6.rs index 7983b30ea1822..448f1493367af 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/illegal_write6.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/illegal_write6.rs @@ -7,6 +7,6 @@ fn main() { fn foo(a: &mut u32, y: *mut u32) -> u32 { *a = 1; let _b = &*a; - unsafe { *y = 2 }; //~ ERROR: /not granting access .* because that would remove .* which is strongly protected/ + unsafe { *y = 2 }; //~ ERROR: /not granting access .* because that would remove .* which is protected/ return *a; } diff --git a/src/tools/miri/tests/fail/stacked_borrows/illegal_write6.stderr b/src/tools/miri/tests/fail/stacked_borrows/illegal_write6.stderr index 3d3d2a24c2852..ec22213eef45f 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/illegal_write6.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/illegal_write6.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID +error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is protected because it is an argument of call ID --> $DIR/illegal_write6.rs:LL:CC | LL | unsafe { *y = 2 }; - | ^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + | ^^^^^^ not granting access to tag because that would remove [Unique for ] which is protected because it is an argument of call ID | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information diff --git a/src/tools/miri/tests/fail/stacked_borrows/invalidate_against_protector1.stderr b/src/tools/miri/tests/fail/stacked_borrows/invalidate_against_protector1.stderr index 95fa4c51d12fd..6cc8a2c93b854 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/invalidate_against_protector1.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/invalidate_against_protector1.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID +error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is protected because it is an argument of call ID --> $DIR/invalidate_against_protector1.rs:LL:CC | LL | let _val = unsafe { *x }; - | ^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + | ^^ not granting access to tag because that would remove [Unique for ] which is protected because it is an argument of call ID | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information diff --git a/src/tools/miri/tests/fail/stacked_borrows/invalidate_against_protector2.stderr b/src/tools/miri/tests/fail/stacked_borrows/invalidate_against_protector2.stderr index 8f677bd547ce3..f09d734e0c596 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/invalidate_against_protector2.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/invalidate_against_protector2.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: not granting access to tag because that would remove [SharedReadOnly for ] which is strongly protected because it is an argument of call ID +error: Undefined Behavior: not granting access to tag because that would remove [SharedReadOnly for ] which is protected because it is an argument of call ID --> $DIR/invalidate_against_protector2.rs:LL:CC | LL | unsafe { *x = 0 }; - | ^^^^^^ not granting access to tag because that would remove [SharedReadOnly for ] which is strongly protected because it is an argument of call ID + | ^^^^^^ not granting access to tag because that would remove [SharedReadOnly for ] which is protected because it is an argument of call ID | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information diff --git a/src/tools/miri/tests/fail/stacked_borrows/invalidate_against_protector3.stderr b/src/tools/miri/tests/fail/stacked_borrows/invalidate_against_protector3.stderr index 1648ca9e58bb1..3aec60b080169 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/invalidate_against_protector3.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/invalidate_against_protector3.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: not granting access to tag because that would remove [SharedReadOnly for ] which is strongly protected because it is an argument of call ID +error: Undefined Behavior: not granting access to tag because that would remove [SharedReadOnly for ] which is protected because it is an argument of call ID --> $DIR/invalidate_against_protector3.rs:LL:CC | LL | unsafe { *x = 0 }; - | ^^^^^^ not granting access to tag because that would remove [SharedReadOnly for ] which is strongly protected because it is an argument of call ID + | ^^^^^^ not granting access to tag because that would remove [SharedReadOnly for ] which is protected because it is an argument of call ID | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information diff --git a/src/tools/miri/tests/fail/stacked_borrows/newtype_pair_retagging.rs b/src/tools/miri/tests/fail/stacked_borrows/newtype_pair_retagging.rs index c19bcb99cc1ce..cc774500a3c69 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/newtype_pair_retagging.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/newtype_pair_retagging.rs @@ -1,4 +1,4 @@ -//@error-pattern: which is strongly protected +//@error-pattern: which is protected struct Newtype<'a>(&'a mut i32, i32); fn dealloc_while_running(_n: Newtype<'_>, dealloc: impl FnOnce()) { diff --git a/src/tools/miri/tests/fail/stacked_borrows/newtype_pair_retagging.stderr b/src/tools/miri/tests/fail/stacked_borrows/newtype_pair_retagging.stderr index 0cba380ea1a30..74b485d490ff6 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/newtype_pair_retagging.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/newtype_pair_retagging.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID +error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is protected because it is an argument of call ID --> RUSTLIB/alloc/src/boxed.rs:LL:CC | LL | Box(unsafe { Unique::new_unchecked(raw) }, alloc) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is protected because it is an argument of call ID | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information diff --git a/src/tools/miri/tests/fail/stacked_borrows/newtype_retagging.rs b/src/tools/miri/tests/fail/stacked_borrows/newtype_retagging.rs index 2bbe7122ec747..1aa6e240e30f1 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/newtype_retagging.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/newtype_retagging.rs @@ -1,4 +1,4 @@ -//@error-pattern: which is strongly protected +//@error-pattern: which is protected struct Newtype<'a>(&'a mut i32); fn dealloc_while_running(_n: Newtype<'_>, dealloc: impl FnOnce()) { diff --git a/src/tools/miri/tests/fail/stacked_borrows/newtype_retagging.stderr b/src/tools/miri/tests/fail/stacked_borrows/newtype_retagging.stderr index f76b6a57eaca0..692cb672e72df 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/newtype_retagging.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/newtype_retagging.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID +error: Undefined Behavior: not granting access to tag because that would remove [Unique for ] which is protected because it is an argument of call ID --> RUSTLIB/alloc/src/boxed.rs:LL:CC | LL | Box(unsafe { Unique::new_unchecked(raw) }, alloc) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly protected because it is an argument of call ID + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is protected because it is an argument of call ID | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information From 6dacaf02902d6a43b9a2fd6537f325f29255da95 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 28 Dec 2022 12:11:21 +0100 Subject: [PATCH 3/3] tweak Box retag diagnostics --- .../src/borrow_tracker/stacked_borrows/diagnostics.rs | 9 +++------ src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs | 5 ++--- .../fail/stacked_borrows/box_noalias_violation.stderr | 2 +- .../tests/fail/stacked_borrows/return-noalias.stderr | 5 +++++ 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs b/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs index ec9dc5cac5cf1..89bbeb6053c3b 100644 --- a/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs +++ b/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs @@ -86,12 +86,7 @@ impl fmt::Display for InvalidationCause { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { InvalidationCause::Access(kind) => write!(f, "{kind}"), - InvalidationCause::Retag(perm, kind) => - if *kind == RetagCause::FnEntry { - write!(f, "{perm:?} FnEntry retag") - } else { - write!(f, "{perm:?} retag") - }, + InvalidationCause::Retag(perm, kind) => write!(f, "{perm:?} {retag}", retag = kind.summary()), } } } @@ -192,6 +187,7 @@ struct RetagOp { #[derive(Debug, Clone, Copy, PartialEq)] pub enum RetagCause { Normal, + Box, FnReturnPlace, FnEntry, TwoPhase, @@ -490,6 +486,7 @@ impl RetagCause { fn summary(&self) -> String { match self { RetagCause::Normal => "retag", + RetagCause::Box => "Box retag", RetagCause::FnEntry => "FnEntry retag", RetagCause::FnReturnPlace => "return-place retag", RetagCause::TwoPhase => "two-phase retag", diff --git a/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs b/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs index e97835a9aa1bd..9419bd16ae74c 100644 --- a/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs @@ -221,6 +221,7 @@ impl<'tcx> Stack { if !global.tracked_pointer_tags.is_empty() { dcx.check_tracked_tag_popped(item, global); } + dcx.log_invalidation(item.tag()); if !item.protected() { return Ok(()); @@ -281,7 +282,6 @@ impl<'tcx> Stack { }; self.pop_items_after(first_incompatible_idx, |item| { Stack::item_invalidated(&item, global, dcx)?; - dcx.log_invalidation(item.tag()); Ok(()) })?; } else { @@ -302,7 +302,6 @@ impl<'tcx> Stack { }; self.disable_uniques_starting_at(first_incompatible_idx, |item| { Stack::item_invalidated(&item, global, dcx)?; - dcx.log_invalidation(item.tag()); Ok(()) })?; } @@ -928,7 +927,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { access: Some(AccessKind::Write), protector: false, }; - self.retag_ptr_inplace(place, new_perm, self.retag_cause) + self.retag_ptr_inplace(place, new_perm, RetagCause::Box) } fn visit_value(&mut self, place: &PlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> { diff --git a/src/tools/miri/tests/fail/stacked_borrows/box_noalias_violation.stderr b/src/tools/miri/tests/fail/stacked_borrows/box_noalias_violation.stderr index 7cf934e60c497..f2d2bbcd98593 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/box_noalias_violation.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/box_noalias_violation.stderr @@ -14,7 +14,7 @@ help: was created by a SharedReadOnly retag at offsets [0x0..0x4] | LL | let ptr = &*b as *const i32; | ^^^ -help: was later invalidated at offsets [0x0..0x4] by a Unique retag +help: was later invalidated at offsets [0x0..0x4] by a Unique Box retag --> $DIR/box_noalias_violation.rs:LL:CC | LL | test(b, ptr); diff --git a/src/tools/miri/tests/fail/stacked_borrows/return-noalias.stderr b/src/tools/miri/tests/fail/stacked_borrows/return-noalias.stderr index f12ee2791a4bb..9d5b0813fc53b 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/return-noalias.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/return-noalias.stderr @@ -14,6 +14,11 @@ help: was created here, as the base tag for ALLOC | LL | let b = unsafe { Box::from_raw(&mut m) }; | ^^^^^^ +help: was later invalidated at offsets [0x0..0x4] by a Unique Box retag + --> $DIR/return-noalias.rs:LL:CC + | +LL | let b = unsafe { Box::from_raw(&mut m) }; + | ^^^^^^^^^^^^^^^^^^^^^ = note: BACKTRACE (of the first span): = note: inside `main` at $DIR/return-noalias.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)