From cb1b881556f1c9cb1341fd2393059fd610f92ec2 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 20 Nov 2022 18:07:51 +0100 Subject: [PATCH 1/3] add a weak form of protection that justifies Box noalias --- src/stacked_borrows/diagnostics.rs | 26 +++--- src/stacked_borrows/mod.rs | 87 +++++++++++++------ .../fail/stacked_borrows/aliasing_mut1.stderr | 4 +- .../fail/stacked_borrows/aliasing_mut2.stderr | 4 +- .../fail/stacked_borrows/aliasing_mut4.stderr | 4 +- .../stacked_borrows/box_noalias_violation.rs | 14 +++ .../box_noalias_violation.stderr | 30 +++++++ .../deallocate_against_protector1.rs | 2 +- .../deallocate_against_protector1.stderr | 4 +- .../deallocate_against_protector2.rs | 2 +- .../deallocate_against_protector2.stderr | 4 +- tests/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 +- 20 files changed, 146 insertions(+), 65 deletions(-) create mode 100644 tests/fail/stacked_borrows/box_noalias_violation.rs create mode 100644 tests/fail/stacked_borrows/box_noalias_violation.stderr diff --git a/src/stacked_borrows/diagnostics.rs b/src/stacked_borrows/diagnostics.rs index d3843b0303..323ec3d75f 100644 --- a/src/stacked_borrows/diagnostics.rs +++ b/src/stacked_borrows/diagnostics.rs @@ -6,7 +6,7 @@ use rustc_span::{Span, SpanData}; use rustc_target::abi::Size; use crate::helpers::CurrentSpan; -use crate::stacked_borrows::{err_sb_ub, AccessKind, GlobalStateInner, Permission}; +use crate::stacked_borrows::{err_sb_ub, AccessKind, GlobalStateInner, Permission, ProtectorKind}; use crate::*; use rustc_middle::mir::interpret::InterpError; @@ -288,7 +288,11 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir } Operation::Access(AccessOp { kind, range, .. }) => (*range, InvalidationCause::Access(*kind)), - _ => unreachable!("Tags can only be invalidated during a retag or access"), + _ => { + // This can be reached, but never be relevant later since the entire allocation is + // gone now. + return; + } }; self.history.invalidations.push(Invalidation { tag, range, span, cause }); } @@ -369,7 +373,7 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir /// Report a descriptive error when `new` could not be granted from `derived_from`. #[inline(never)] // This is only called on fatal code paths - pub fn grant_error(&self, perm: Permission, stack: &Stack) -> InterpError<'tcx> { + pub(super) fn grant_error(&self, perm: Permission, stack: &Stack) -> InterpError<'tcx> { let Operation::Retag(op) = &self.operation else { unreachable!("grant_error should only be called during a retag") }; @@ -389,7 +393,7 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir /// Report a descriptive error when `access` is not permitted based on `tag`. #[inline(never)] // This is only called on fatal code paths - pub fn access_error(&self, stack: &Stack) -> InterpError<'tcx> { + pub(super) fn access_error(&self, stack: &Stack) -> InterpError<'tcx> { let Operation::Access(op) = &self.operation else { unreachable!("access_error should only be called during an access") }; @@ -408,7 +412,11 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir } #[inline(never)] // This is only called on fatal code paths - pub fn protector_error(&self, item: &Item) -> InterpError<'tcx> { + pub(super) fn protector_error(&self, item: &Item, kind: ProtectorKind) -> InterpError<'tcx> { + let protected = match kind { + ProtectorKind::WeakProtector => "weakly protected", + ProtectorKind::StrongProtector => "strongly protected", + }; let call_id = self .threads .all_stacks() @@ -422,10 +430,7 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir match self.operation { Operation::Dealloc(_) => err_sb_ub( - format!( - "deallocating while item {:?} is protected by call {:?}", - item, call_id - ), + format!("deallocating while item {item:?} is {protected} by call {call_id:?}",), None, None, ), @@ -433,8 +438,7 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir | Operation::Access(AccessOp { tag, .. }) => err_sb_ub( format!( - "not granting access to tag {:?} because that would remove {:?} which is protected because it is an argument of call {:?}", - tag, item, 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/stacked_borrows/mod.rs b/src/stacked_borrows/mod.rs index f2e2df5ad0..648797211a 100644 --- a/src/stacked_borrows/mod.rs +++ b/src/stacked_borrows/mod.rs @@ -91,6 +91,26 @@ pub struct Stacks { modified_since_last_gc: bool, } +/// The flavor of the protector. +#[derive(Copy, Clone, Debug)] +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, +} + /// Extra global state, available to the memory access hooks. #[derive(Debug)] pub struct GlobalStateInner { @@ -102,12 +122,12 @@ pub struct GlobalStateInner { base_ptr_tags: FxHashMap, /// Next unused call ID (for protectors). next_call_id: CallId, - /// All currently protected tags. + /// All currently protected tags, and the status of their protection. /// An item is protected if its tag is in this set, *and* it has the "protected" bit set. /// 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. - protected_tags: FxHashSet, + protected_tags: FxHashMap, /// The pointer ids to trace tracked_pointer_tags: FxHashSet, /// The call ids to trace @@ -189,7 +209,7 @@ impl GlobalStateInner { next_ptr_tag: SbTag(NonZeroU64::new(1).unwrap()), base_ptr_tags: FxHashMap::default(), next_call_id: NonZeroU64::new(1).unwrap(), - protected_tags: FxHashSet::default(), + protected_tags: FxHashMap::default(), tracked_pointer_tags, tracked_call_ids, retag_fields, @@ -314,6 +334,7 @@ impl<'tcx> Stack { item: &Item, global: &GlobalStateInner, dcx: &mut DiagnosticCx<'_, '_, '_, '_, 'tcx>, + deallocation: bool, ) -> InterpResult<'tcx> { if !global.tracked_pointer_tags.is_empty() { dcx.check_tracked_tag_popped(item, global); @@ -336,8 +357,11 @@ 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 global.protected_tags.contains(&item.tag()) { - return Err(dcx.protector_error(item).into()); + if let Some(&protector_kind) = global.protected_tags.get(&item.tag()) { + let allowed = deallocation && matches!(protector_kind, ProtectorKind::WeakProtector); + if !allowed { + return Err(dcx.protector_error(item, protector_kind).into()); + } } Ok(()) } @@ -350,7 +374,7 @@ impl<'tcx> Stack { &mut self, access: AccessKind, tag: ProvenanceExtra, - global: &mut GlobalStateInner, + global: &GlobalStateInner, dcx: &mut DiagnosticCx<'_, '_, '_, '_, 'tcx>, exposed_tags: &FxHashSet, ) -> InterpResult<'tcx> { @@ -377,7 +401,7 @@ impl<'tcx> Stack { 0 }; self.pop_items_after(first_incompatible_idx, |item| { - Stack::item_popped(&item, global, dcx)?; + Stack::item_popped(&item, global, dcx, /* deallocation */ false)?; dcx.log_invalidation(item.tag()); Ok(()) })?; @@ -398,7 +422,7 @@ impl<'tcx> Stack { 0 }; self.disable_uniques_starting_at(first_incompatible_idx, |item| { - Stack::item_popped(&item, global, dcx)?; + Stack::item_popped(&item, global, dcx, /* deallocation */ false)?; dcx.log_invalidation(item.tag()); Ok(()) })?; @@ -440,14 +464,15 @@ impl<'tcx> Stack { dcx: &mut DiagnosticCx<'_, '_, '_, '_, 'tcx>, exposed_tags: &FxHashSet, ) -> InterpResult<'tcx> { - // Step 1: Make sure there is a granting item. - self.find_granting(AccessKind::Write, tag, exposed_tags) + // Step 1: Make a write access. + // 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) .map_err(|_| dcx.dealloc_error())?; - // Step 2: Consider all items removed. This checks for protectors. + // Step 2: Pretend we remove the remaining items, checking if any are strongly protected. for idx in (0..self.len()).rev() { let item = self.get(idx).unwrap(); - Stack::item_popped(&item, global, dcx)?; + Stack::item_popped(&item, global, dcx, /* deallocation */ true)?; } Ok(()) @@ -698,7 +723,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' kind: RefKind, retag_cause: RetagCause, // What caused this retag, for diagnostics only new_tag: SbTag, - protect: bool, + protect: Option, ) -> InterpResult<'tcx, Option> { let this = self.eval_context_mut(); @@ -761,7 +786,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 protect { + if protect.is_some() { dcx.log_protector(); } } @@ -821,10 +846,16 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' size.bytes() ); - if protect { + if let Some(protect) = protect { // See comment in `Stack::item_popped` for why we store the tag twice. this.frame_mut().extra.stacked_borrows.as_mut().unwrap().protected_tags.push(new_tag); - this.machine.stacked_borrows.as_mut().unwrap().get_mut().protected_tags.insert(new_tag); + this.machine + .stacked_borrows + .as_mut() + .unwrap() + .get_mut() + .protected_tags + .insert(new_tag, protect); } // Update the stacks. @@ -866,7 +897,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' Permission::SharedReadWrite }; let protected = if frozen { - protect + protect.is_some() } else { // We do not protect inside UnsafeCell. // This fixes https://github.com/rust-lang/rust/issues/55005. @@ -899,7 +930,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' .as_mut() .expect("we should have Stacked Borrows data") .borrow_mut(); - let item = Item::new(new_tag, perm, protect); + let item = Item::new(new_tag, perm, protect.is_some()); let range = alloc_range(base_offset, size); let mut global = machine.stacked_borrows.as_ref().unwrap().borrow_mut(); // FIXME: can't share this with the current_span inside log_creation @@ -926,7 +957,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' val: &ImmTy<'tcx, Provenance>, kind: RefKind, retag_cause: RetagCause, // What caused this retag, for diagnostics only - protect: bool, + protect: Option, ) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> { let this = self.eval_context_mut(); // We want a place for where the ptr *points to*, so we get one. @@ -996,7 +1027,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { place: &PlaceTy<'tcx, Provenance>, ref_kind: RefKind, retag_cause: RetagCause, - protector: bool, + protector: Option, ) -> InterpResult<'tcx> { let val = self.ecx.read_immediate(&self.ecx.place_to_op(place)?)?; let val = self.ecx.retag_reference(&val, ref_kind, retag_cause, protector)?; @@ -1015,13 +1046,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } fn visit_box(&mut self, place: &PlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> { - // Boxes do not get a protector: protectors reflect that references outlive the call - // they were passed in to; that's just not the case for boxes. + // Boxes get a weak protectors, since they may be deallocated. self.retag_place( place, RefKind::Unique { two_phase: false }, self.retag_cause, - /*protector*/ false, + /*protector*/ + (self.kind == RetagKind::FnEntry).then_some(ProtectorKind::WeakProtector), ) } @@ -1046,7 +1077,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { place, ref_kind, self.retag_cause, - /*protector*/ self.kind == RetagKind::FnEntry, + /*protector*/ + (self.kind == RetagKind::FnEntry) + .then_some(ProtectorKind::StrongProtector), )?; } ty::RawPtr(tym) => { @@ -1059,7 +1092,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { place, RefKind::Raw { mutable: tym.mutbl == Mutability::Mut }, self.retag_cause, - /*protector*/ false, + /*protector*/ None, )?; } } @@ -1110,12 +1143,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // (The pointer type does not matter, so we use a raw pointer.) let ptr_layout = this.layout_of(this.tcx.mk_mut_ptr(return_place.layout.ty))?; let val = ImmTy::from_immediate(return_place.to_ref(this), ptr_layout); - // Reborrow it. + // Reborrow it. With protection! That is part of the point. let val = this.retag_reference( &val, RefKind::Unique { two_phase: false }, RetagCause::FnReturn, - /*protector*/ true, + /*protector*/ Some(ProtectorKind::StrongProtector), )?; // And use reborrowed pointer for return place. let return_place = this.ref_to_mplace(&val)?; diff --git a/tests/fail/stacked_borrows/aliasing_mut1.stderr b/tests/fail/stacked_borrows/aliasing_mut1.stderr index 5d4679b13a..268d253ad5 100644 --- a/tests/fail/stacked_borrows/aliasing_mut1.stderr +++ b/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 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 strongly 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 protected because it is an argument of call ID + | ^^ not granting access to tag because that would remove [Unique for ] which is strongly 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/tests/fail/stacked_borrows/aliasing_mut2.stderr b/tests/fail/stacked_borrows/aliasing_mut2.stderr index c8408c150e..77a542f45a 100644 --- a/tests/fail/stacked_borrows/aliasing_mut2.stderr +++ b/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 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 strongly 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 protected because it is an argument of call ID + | ^^ not granting access to tag because that would remove [SharedReadOnly for ] which is strongly 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/tests/fail/stacked_borrows/aliasing_mut4.stderr b/tests/fail/stacked_borrows/aliasing_mut4.stderr index c53fe70f6d..e592b154a7 100644 --- a/tests/fail/stacked_borrows/aliasing_mut4.stderr +++ b/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 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 strongly 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 protected because it is an argument of call ID + | ^^ not granting access to tag because that would remove [SharedReadOnly for ] which is strongly 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/tests/fail/stacked_borrows/box_noalias_violation.rs b/tests/fail/stacked_borrows/box_noalias_violation.rs new file mode 100644 index 0000000000..2067149b6c --- /dev/null +++ b/tests/fail/stacked_borrows/box_noalias_violation.rs @@ -0,0 +1,14 @@ +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 +} + +fn main() { + unsafe { + let mut v = 42; + let ptr = &mut v as *mut i32; + test(Box::from_raw(ptr), ptr); + } +} diff --git a/tests/fail/stacked_borrows/box_noalias_violation.stderr b/tests/fail/stacked_borrows/box_noalias_violation.stderr new file mode 100644 index 0000000000..3c84cbcfd5 --- /dev/null +++ b/tests/fail/stacked_borrows/box_noalias_violation.stderr @@ -0,0 +1,30 @@ +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 + --> $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 + | + = 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] + --> $DIR/box_noalias_violation.rs:LL:CC + | +LL | let ptr = &mut v as *mut i32; + | ^^^^^^ +help: is this argument + --> $DIR/box_noalias_violation.rs:LL:CC + | +LL | unsafe fn test(mut x: Box, y: *const i32) -> i32 { + | ^^^^^ + = note: BACKTRACE: + = note: inside `test` at $DIR/box_noalias_violation.rs:LL:CC +note: inside `main` at $DIR/box_noalias_violation.rs:LL:CC + --> $DIR/box_noalias_violation.rs:LL:CC + | +LL | test(Box::from_raw(ptr), ptr); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +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/fail/stacked_borrows/deallocate_against_protector1.rs b/tests/fail/stacked_borrows/deallocate_against_protector1.rs index 9b710424c5..4036dce5be 100644 --- a/tests/fail/stacked_borrows/deallocate_against_protector1.rs +++ b/tests/fail/stacked_borrows/deallocate_against_protector1.rs @@ -1,4 +1,4 @@ -//@error-pattern: /deallocating while item \[Unique for .*\] is protected/ +//@error-pattern: /deallocating while item \[Unique for .*\] is strongly protected/ fn inner(x: &mut i32, f: fn(&mut i32)) { // `f` may mutate, but it may not deallocate! diff --git a/tests/fail/stacked_borrows/deallocate_against_protector1.stderr b/tests/fail/stacked_borrows/deallocate_against_protector1.stderr index a5db4a00c6..bb3eaec1e8 100644 --- a/tests/fail/stacked_borrows/deallocate_against_protector1.stderr +++ b/tests/fail/stacked_borrows/deallocate_against_protector1.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: deallocating while item [Unique for ] is protected by call ID +error: Undefined Behavior: deallocating while item [Unique for ] is strongly 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 protected by call ID + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ deallocating while item [Unique for ] is strongly 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/tests/fail/stacked_borrows/deallocate_against_protector2.rs b/tests/fail/stacked_borrows/deallocate_against_protector2.rs index 36e133e383..fd67dccd14 100644 --- a/tests/fail/stacked_borrows/deallocate_against_protector2.rs +++ b/tests/fail/stacked_borrows/deallocate_against_protector2.rs @@ -1,4 +1,4 @@ -//@error-pattern: /deallocating while item \[SharedReadWrite for .*\] is protected/ +//@error-pattern: /deallocating while item \[SharedReadWrite for .*\] is strongly protected/ use std::marker::PhantomPinned; pub struct NotUnpin(i32, PhantomPinned); diff --git a/tests/fail/stacked_borrows/deallocate_against_protector2.stderr b/tests/fail/stacked_borrows/deallocate_against_protector2.stderr index 99c6ee6eb0..25bab1aa56 100644 --- a/tests/fail/stacked_borrows/deallocate_against_protector2.stderr +++ b/tests/fail/stacked_borrows/deallocate_against_protector2.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: deallocating while item [SharedReadWrite for ] is protected by call ID +error: Undefined Behavior: deallocating while item [SharedReadWrite for ] is strongly 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 protected by call ID + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ deallocating while item [SharedReadWrite for ] is strongly 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/tests/fail/stacked_borrows/illegal_write6.rs b/tests/fail/stacked_borrows/illegal_write6.rs index 448f149336..7983b30ea1 100644 --- a/tests/fail/stacked_borrows/illegal_write6.rs +++ b/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 protected/ + unsafe { *y = 2 }; //~ ERROR: /not granting access .* because that would remove .* which is strongly protected/ return *a; } diff --git a/tests/fail/stacked_borrows/illegal_write6.stderr b/tests/fail/stacked_borrows/illegal_write6.stderr index 331faa89f8..1a627b8a88 100644 --- a/tests/fail/stacked_borrows/illegal_write6.stderr +++ b/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 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 strongly 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 protected because it is an argument of call ID + | ^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly 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/tests/fail/stacked_borrows/invalidate_against_protector1.stderr b/tests/fail/stacked_borrows/invalidate_against_protector1.stderr index f87bd2319a..1ef36b7ac1 100644 --- a/tests/fail/stacked_borrows/invalidate_against_protector1.stderr +++ b/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 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 strongly 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 protected because it is an argument of call ID + | ^^ not granting access to tag because that would remove [Unique for ] which is strongly 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/tests/fail/stacked_borrows/invalidate_against_protector2.stderr b/tests/fail/stacked_borrows/invalidate_against_protector2.stderr index 07c51a39b8..941b936e5d 100644 --- a/tests/fail/stacked_borrows/invalidate_against_protector2.stderr +++ b/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 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 strongly 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 protected because it is an argument of call ID + | ^^^^^^ not granting access to tag because that would remove [SharedReadOnly for ] which is strongly 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/tests/fail/stacked_borrows/invalidate_against_protector3.stderr b/tests/fail/stacked_borrows/invalidate_against_protector3.stderr index afda15ea16..176a859ee8 100644 --- a/tests/fail/stacked_borrows/invalidate_against_protector3.stderr +++ b/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 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 strongly 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 protected because it is an argument of call ID + | ^^^^^^ not granting access to tag because that would remove [SharedReadOnly for ] which is strongly 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/tests/fail/stacked_borrows/newtype_pair_retagging.rs b/tests/fail/stacked_borrows/newtype_pair_retagging.rs index cc774500a3..c19bcb99cc 100644 --- a/tests/fail/stacked_borrows/newtype_pair_retagging.rs +++ b/tests/fail/stacked_borrows/newtype_pair_retagging.rs @@ -1,4 +1,4 @@ -//@error-pattern: which is protected +//@error-pattern: which is strongly protected struct Newtype<'a>(&'a mut i32, i32); fn dealloc_while_running(_n: Newtype<'_>, dealloc: impl FnOnce()) { diff --git a/tests/fail/stacked_borrows/newtype_pair_retagging.stderr b/tests/fail/stacked_borrows/newtype_pair_retagging.stderr index 60a8b2a626..70186dd3a5 100644 --- a/tests/fail/stacked_borrows/newtype_pair_retagging.stderr +++ b/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 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 strongly 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 protected because it is an argument of call ID + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly 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/tests/fail/stacked_borrows/newtype_retagging.rs b/tests/fail/stacked_borrows/newtype_retagging.rs index 1aa6e240e3..2bbe7122ec 100644 --- a/tests/fail/stacked_borrows/newtype_retagging.rs +++ b/tests/fail/stacked_borrows/newtype_retagging.rs @@ -1,4 +1,4 @@ -//@error-pattern: which is protected +//@error-pattern: which is strongly protected struct Newtype<'a>(&'a mut i32); fn dealloc_while_running(_n: Newtype<'_>, dealloc: impl FnOnce()) { diff --git a/tests/fail/stacked_borrows/newtype_retagging.stderr b/tests/fail/stacked_borrows/newtype_retagging.stderr index 06a9b86c6f..69fa27c9c0 100644 --- a/tests/fail/stacked_borrows/newtype_retagging.stderr +++ b/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 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 strongly 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 protected because it is an argument of call ID + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag because that would remove [Unique for ] which is strongly 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 c2a6c1e882060986fa88bd2adecf7e9894d5260c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 20 Nov 2022 18:11:30 +0100 Subject: [PATCH 2/3] some things don't need to be mutable --- src/stacked_borrows/mod.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/stacked_borrows/mod.rs b/src/stacked_borrows/mod.rs index 648797211a..9ea61ae562 100644 --- a/src/stacked_borrows/mod.rs +++ b/src/stacked_borrows/mod.rs @@ -488,7 +488,7 @@ impl<'tcx> Stack { &mut self, derived_from: ProvenanceExtra, new: Item, - global: &mut GlobalStateInner, + global: &GlobalStateInner, dcx: &mut DiagnosticCx<'_, '_, '_, '_, 'tcx>, exposed_tags: &FxHashSet, ) -> InterpResult<'tcx> { @@ -658,9 +658,9 @@ impl Stacks { range.size.bytes() ); let dcx = DiagnosticCxBuilder::read(&mut current_span, threads, tag, range); - let mut state = state.borrow_mut(); + let state = state.borrow(); self.for_each(range, dcx, |stack, dcx, exposed_tags| { - stack.access(AccessKind::Read, tag, &mut state, dcx, exposed_tags) + stack.access(AccessKind::Read, tag, &state, dcx, exposed_tags) }) } @@ -681,9 +681,9 @@ impl Stacks { range.size.bytes() ); let dcx = DiagnosticCxBuilder::write(&mut current_span, threads, tag, range); - let mut state = state.borrow_mut(); + let state = state.borrow(); self.for_each(range, dcx, |stack, dcx, exposed_tags| { - stack.access(AccessKind::Write, tag, &mut state, dcx, exposed_tags) + stack.access(AccessKind::Write, tag, &state, dcx, exposed_tags) }) } @@ -904,7 +904,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' false }; let item = Item::new(new_tag, perm, protected); - let mut global = this.machine.stacked_borrows.as_ref().unwrap().borrow_mut(); + let global = this.machine.stacked_borrows.as_ref().unwrap().borrow(); let dcx = DiagnosticCxBuilder::retag( &mut current_span, // FIXME avoid this `clone` &this.machine.threads, @@ -914,7 +914,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, &mut global, dcx, exposed_tags) + stack.grant(orig_tag, item, &global, dcx, exposed_tags) }) })?; return Ok(Some(alloc_id)); @@ -932,7 +932,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' .borrow_mut(); let item = Item::new(new_tag, perm, protect.is_some()); let range = alloc_range(base_offset, size); - let mut global = machine.stacked_borrows.as_ref().unwrap().borrow_mut(); + let global = machine.stacked_borrows.as_ref().unwrap().borrow(); // FIXME: can't share this with the current_span inside log_creation let current_span = &mut machine.current_span(); let dcx = DiagnosticCxBuilder::retag( @@ -944,7 +944,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, &mut global, dcx, exposed_tags) + stack.grant(orig_tag, item, &global, dcx, exposed_tags) })?; Ok(Some(alloc_id)) From e27d57b0ae3557a6aa421bb2e800b40104d3a8ec Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 20 Nov 2022 22:22:38 +0100 Subject: [PATCH 3/3] tweaks --- src/stacked_borrows/diagnostics.rs | 2 +- src/stacked_borrows/mod.rs | 28 +++++++++++++++++++--------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/stacked_borrows/diagnostics.rs b/src/stacked_borrows/diagnostics.rs index 323ec3d75f..f307bf11ed 100644 --- a/src/stacked_borrows/diagnostics.rs +++ b/src/stacked_borrows/diagnostics.rs @@ -288,7 +288,7 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir } Operation::Access(AccessOp { kind, range, .. }) => (*range, InvalidationCause::Access(*kind)), - _ => { + Operation::Dealloc(_) => { // This can be reached, but never be relevant later since the entire allocation is // gone now. return; diff --git a/src/stacked_borrows/mod.rs b/src/stacked_borrows/mod.rs index 9ea61ae562..bc52428910 100644 --- a/src/stacked_borrows/mod.rs +++ b/src/stacked_borrows/mod.rs @@ -65,7 +65,7 @@ pub struct FrameExtra { /// incremental updates of the global list of protected tags stored in the /// `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. + /// See `Stack::item_invalidated` for more explanation. /// /// This will contain one tag per reference passed to the function, so /// a size of 2 is enough for the vast majority of functions. @@ -126,7 +126,7 @@ pub struct GlobalStateInner { /// An item is protected if its tag is in this set, *and* it has the "protected" bit set. /// 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. + /// `GlobalStateInner::end_call`. See `Stack::item_invalidated` for more details. protected_tags: FxHashMap, /// The pointer ids to trace tracked_pointer_tags: FxHashSet, @@ -292,6 +292,13 @@ 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 -- @@ -330,11 +337,11 @@ impl<'tcx> Stack { /// Within `provoking_access, the `AllocRange` refers the entire operation, and /// the `Size` refers to the specific location in the `AllocRange` that we are /// currently checking. - fn item_popped( + fn item_invalidated( item: &Item, global: &GlobalStateInner, dcx: &mut DiagnosticCx<'_, '_, '_, '_, 'tcx>, - deallocation: bool, + cause: ItemInvalidationCause, ) -> InterpResult<'tcx> { if !global.tracked_pointer_tags.is_empty() { dcx.check_tracked_tag_popped(item, global); @@ -358,7 +365,10 @@ impl<'tcx> Stack { // 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()) { - let allowed = deallocation && matches!(protector_kind, ProtectorKind::WeakProtector); + // 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()); } @@ -401,7 +411,7 @@ impl<'tcx> Stack { 0 }; self.pop_items_after(first_incompatible_idx, |item| { - Stack::item_popped(&item, global, dcx, /* deallocation */ false)?; + Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Conflict)?; dcx.log_invalidation(item.tag()); Ok(()) })?; @@ -422,7 +432,7 @@ impl<'tcx> Stack { 0 }; self.disable_uniques_starting_at(first_incompatible_idx, |item| { - Stack::item_popped(&item, global, dcx, /* deallocation */ false)?; + Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Conflict)?; dcx.log_invalidation(item.tag()); Ok(()) })?; @@ -472,7 +482,7 @@ impl<'tcx> Stack { // Step 2: Pretend we remove the remaining items, checking if any are strongly protected. for idx in (0..self.len()).rev() { let item = self.get(idx).unwrap(); - Stack::item_popped(&item, global, dcx, /* deallocation */ true)?; + Stack::item_invalidated(&item, global, dcx, ItemInvalidationCause::Dealloc)?; } Ok(()) @@ -847,7 +857,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' ); if let Some(protect) = protect { - // See comment in `Stack::item_popped` for why we store the tag twice. + // See comment in `Stack::item_invalidated` for why we store the tag twice. this.frame_mut().extra.stacked_borrows.as_mut().unwrap().protected_tags.push(new_tag); this.machine .stacked_borrows