From 3271858deb40a7c1348274f942c704d6735ff9a7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 17 Jul 2023 16:45:18 +0200 Subject: [PATCH 1/3] make full field retagging the default --- README.md | 14 +++---- src/eval.rs | 2 +- tests/fail/both_borrows/buggy_split_at_mut.rs | 2 +- .../buggy_split_at_mut.stack.stderr | 20 +++++++--- tests/fail/generator-pinned-moved.rs | 5 ++- tests/pass/generator.rs | 38 +++++++++++++------ .../stacked-borrows/interior_mutability.rs | 1 - tests/pass/stacked-borrows/stacked-borrows.rs | 1 - .../zst-field-retagging-terminates.rs | 1 - 9 files changed, 50 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index a965f44a4e..eaf58340d7 100644 --- a/README.md +++ b/README.md @@ -407,15 +407,11 @@ to Miri failing to detect cases of undefined behavior in a program. application instead of raising an error within the context of Miri (and halting execution). Note that code might not expect these operations to ever panic, so this flag can lead to strange (mis)behavior. -* `-Zmiri-retag-fields` changes Stacked Borrows retagging to recurse into *all* fields. - This means that references in fields of structs/enums/tuples/arrays/... are retagged, - and in particular, they are protected when passed as function arguments. - (The default is to recurse only in cases where rustc would actually emit a `noalias` attribute.) -* `-Zmiri-retag-fields=` controls when Stacked Borrows retagging recurses into - fields. `all` means it always recurses (like `-Zmiri-retag-fields`), `none` means it never - recurses, `scalar` (the default) means it only recurses for types where we would also emit - `noalias` annotations in the generated LLVM IR (types passed as individual scalars or pairs of - scalars). Setting this to `none` is **unsound**. +* `-Zmiri-retag-fields[=]` controls when Stacked Borrows retagging recurses into + fields. `all` means it always recurses (the default, and equivalent to `-Zmiri-retag-fields` + without an explicit value), `none` means it never recurses, `scalar` means it only recurses for + types where we would also emit `noalias` annotations in the generated LLVM IR (types passed as + individual scalars or pairs of scalars). Setting this to `none` is **unsound**. * `-Zmiri-tag-gc=` configures how often the pointer tag garbage collector runs. The default is to search for and remove unreachable tags once every `10000` basic blocks. Setting this to `0` disables the garbage collector, which causes some programs to have explosive memory usage diff --git a/src/eval.rs b/src/eval.rs index ed3d741db1..79a7115f24 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -183,7 +183,7 @@ impl Default for MiriConfig { mute_stdout_stderr: false, preemption_rate: 0.01, // 1% report_progress: None, - retag_fields: RetagFields::OnlyScalar, + retag_fields: RetagFields::Yes, external_so_file: None, gc_interval: 10_000, num_cpus: 1, diff --git a/tests/fail/both_borrows/buggy_split_at_mut.rs b/tests/fail/both_borrows/buggy_split_at_mut.rs index 4dc6eaf7cd..80ededd210 100644 --- a/tests/fail/both_borrows/buggy_split_at_mut.rs +++ b/tests/fail/both_borrows/buggy_split_at_mut.rs @@ -14,6 +14,7 @@ mod safe { from_raw_parts_mut(ptr, len - mid), // BUG: should be "mid" instead of "len - mid" from_raw_parts_mut(ptr.offset(mid as isize), len - mid), ) + //~[stack]^^^^ ERROR: /retag .* tag does not exist in the borrow stack/ } } } @@ -21,7 +22,6 @@ mod safe { fn main() { let mut array = [1, 2, 3, 4]; let (a, b) = safe::split_at_mut(&mut array, 0); - //~[stack]^ ERROR: /retag .* tag does not exist in the borrow stack/ a[1] = 5; b[1] = 6; //~[tree]^ ERROR: /write access through .* is forbidden/ diff --git a/tests/fail/both_borrows/buggy_split_at_mut.stack.stderr b/tests/fail/both_borrows/buggy_split_at_mut.stack.stderr index c75d8cab3f..0cb6111ecb 100644 --- a/tests/fail/both_borrows/buggy_split_at_mut.stack.stderr +++ b/tests/fail/both_borrows/buggy_split_at_mut.stack.stderr @@ -1,11 +1,14 @@ error: Undefined Behavior: trying to retag from for Unique permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location --> $DIR/buggy_split_at_mut.rs:LL:CC | -LL | let (a, b) = safe::split_at_mut(&mut array, 0); - | ^ - | | - | trying to retag from for Unique 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..0x10] +LL | / ( +LL | | from_raw_parts_mut(ptr, len - mid), // BUG: should be "mid" instead of "len - mid" +LL | | from_raw_parts_mut(ptr.offset(mid as isize), len - mid), +LL | | ) + | | ^ + | | | + | |_____________trying to retag from for Unique 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..0x10] | = 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 @@ -20,7 +23,12 @@ help: was later invalidated at offsets [0x0..0x10] by a Unique retag LL | from_raw_parts_mut(ptr.offset(mid as isize), len - mid), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = note: BACKTRACE (of the first span): - = note: inside `main` at $DIR/buggy_split_at_mut.rs:LL:CC + = note: inside `safe::split_at_mut::` at $DIR/buggy_split_at_mut.rs:LL:CC +note: inside `main` + --> $DIR/buggy_split_at_mut.rs:LL:CC + | +LL | let (a, b) = safe::split_at_mut(&mut array, 0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/tests/fail/generator-pinned-moved.rs b/tests/fail/generator-pinned-moved.rs index 240ae18cc4..29dc6e56f7 100644 --- a/tests/fail/generator-pinned-moved.rs +++ b/tests/fail/generator-pinned-moved.rs @@ -1,4 +1,4 @@ -//@compile-flags: -Zmiri-disable-validation +//@compile-flags: -Zmiri-disable-validation -Zmiri-disable-stacked-borrows #![feature(generators, generator_trait)] use std::{ @@ -10,9 +10,10 @@ fn firstn() -> impl Generator { static move || { let mut num = 0; let num = &mut num; + *num += 0; yield *num; - *num += 1; //~ ERROR: dereferenced after this allocation got freed + *num += 1; //~ERROR: dereferenced after this allocation got freed } } diff --git a/tests/pass/generator.rs b/tests/pass/generator.rs index e059c7114e..2009960345 100644 --- a/tests/pass/generator.rs +++ b/tests/pass/generator.rs @@ -13,7 +13,7 @@ use std::ptr; use std::sync::atomic::{AtomicUsize, Ordering}; fn basic() { - fn finish(mut amt: usize, mut t: T) -> T::Return + fn finish(mut amt: usize, self_referential: bool, mut t: T) -> T::Return where T: Generator, { @@ -22,7 +22,10 @@ fn basic() { loop { let state = t.as_mut().resume(()); // Test if the generator is valid (according to type invariants). - let _ = unsafe { ManuallyDrop::new(ptr::read(t.as_mut().get_unchecked_mut())) }; + // For self-referential generators however this is UB! + if !self_referential { + let _ = unsafe { ManuallyDrop::new(ptr::read(t.as_mut().get_unchecked_mut())) }; + } match state { GeneratorState::Yielded(y) => { amt -= y; @@ -40,9 +43,9 @@ fn basic() { panic!() } - finish(1, || yield 1); + finish(1, false, || yield 1); - finish(3, || { + finish(3, false, || { let mut x = 0; yield 1; x += 1; @@ -52,27 +55,27 @@ fn basic() { assert_eq!(x, 2); }); - finish(7 * 8 / 2, || { + finish(7 * 8 / 2, false, || { for i in 0..8 { yield i; } }); - finish(1, || { + finish(1, false, || { if true { yield 1; } else { } }); - finish(1, || { + finish(1, false, || { if false { } else { yield 1; } }); - finish(2, || { + finish(2, false, || { if { yield 1; false @@ -83,9 +86,20 @@ fn basic() { yield 1; }); - // also test a self-referential generator + // also test self-referential generators + assert_eq!( + finish(5, true, static || { + let mut x = 5; + let y = &mut x; + *y = 5; + yield *y; + *y = 10; + x + }), + 10 + ); assert_eq!( - finish(5, || { + finish(5, true, || { let mut x = Box::new(5); let y = &mut *x; *y = 5; @@ -97,7 +111,7 @@ fn basic() { ); let b = true; - finish(1, || { + finish(1, false, || { yield 1; if b { return; @@ -109,7 +123,7 @@ fn basic() { drop(x); }); - finish(3, || { + finish(3, false, || { yield 1; #[allow(unreachable_code)] let _x: (String, !) = (String::new(), { diff --git a/tests/pass/stacked-borrows/interior_mutability.rs b/tests/pass/stacked-borrows/interior_mutability.rs index c6373a7eaf..830e9c3384 100644 --- a/tests/pass/stacked-borrows/interior_mutability.rs +++ b/tests/pass/stacked-borrows/interior_mutability.rs @@ -1,4 +1,3 @@ -//@compile-flags: -Zmiri-retag-fields use std::cell::{Cell, Ref, RefCell, RefMut, UnsafeCell}; use std::mem::{self, MaybeUninit}; diff --git a/tests/pass/stacked-borrows/stacked-borrows.rs b/tests/pass/stacked-borrows/stacked-borrows.rs index d7d7d1f97d..43ae7d6f52 100644 --- a/tests/pass/stacked-borrows/stacked-borrows.rs +++ b/tests/pass/stacked-borrows/stacked-borrows.rs @@ -1,4 +1,3 @@ -//@compile-flags: -Zmiri-retag-fields #![feature(allocator_api)] use std::ptr; diff --git a/tests/pass/stacked-borrows/zst-field-retagging-terminates.rs b/tests/pass/stacked-borrows/zst-field-retagging-terminates.rs index 6e13a9ea83..4faf6004ad 100644 --- a/tests/pass/stacked-borrows/zst-field-retagging-terminates.rs +++ b/tests/pass/stacked-borrows/zst-field-retagging-terminates.rs @@ -1,4 +1,3 @@ -//@compile-flags: -Zmiri-retag-fields // Checks that the test does not run forever (which relies on a fast path). #![allow(dropping_copy_types)] From c5c0f85df3d3f98679d565c7a05f18e6ce2f76ed Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 21 Jul 2023 16:08:30 +0200 Subject: [PATCH 2/3] SB: track whether a retag occurred nested inside a field --- .../stacked_borrows/diagnostics.rs | 41 +++++++++++------ src/borrow_tracker/stacked_borrows/mod.rs | 45 ++++++++++++------- .../buggy_split_at_mut.stack.stderr | 2 +- .../pass_invalid_shr_option.stack.stderr | 2 +- .../pass_invalid_shr_tuple.stack.stderr | 2 +- .../return_invalid_shr_option.stack.stderr | 2 +- .../return_invalid_shr_tuple.stack.stderr | 2 +- .../return_invalid_mut_option.stderr | 2 +- .../return_invalid_mut_tuple.stderr | 2 +- 9 files changed, 63 insertions(+), 37 deletions(-) diff --git a/src/borrow_tracker/stacked_borrows/diagnostics.rs b/src/borrow_tracker/stacked_borrows/diagnostics.rs index 5ec8d80fb3..33b777abd9 100644 --- a/src/borrow_tracker/stacked_borrows/diagnostics.rs +++ b/src/borrow_tracker/stacked_borrows/diagnostics.rs @@ -61,12 +61,15 @@ struct Invalidation { #[derive(Clone, Debug)] enum InvalidationCause { Access(AccessKind), - Retag(Permission, RetagCause), + Retag(Permission, RetagInfo), } impl Invalidation { fn generate_diagnostic(&self) -> (String, SpanData) { - let message = if let InvalidationCause::Retag(_, RetagCause::FnEntry) = self.cause { + let message = if matches!( + self.cause, + InvalidationCause::Retag(_, RetagInfo { cause: RetagCause::FnEntry, .. }) + ) { // For a FnEntry retag, our Span points at the caller. // See `DiagnosticCx::log_invalidation`. format!( @@ -87,8 +90,8 @@ 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) => - write!(f, "{perm:?} {retag}", retag = kind.summary()), + InvalidationCause::Retag(perm, info) => + write!(f, "{perm:?} {retag}", retag = info.summary()), } } } @@ -129,13 +132,13 @@ impl<'ecx, 'mir, 'tcx> DiagnosticCxBuilder<'ecx, 'mir, 'tcx> { pub fn retag( machine: &'ecx MiriMachine<'mir, 'tcx>, - cause: RetagCause, + info: RetagInfo, new_tag: BorTag, orig_tag: ProvenanceExtra, range: AllocRange, ) -> Self { let operation = - Operation::Retag(RetagOp { cause, new_tag, orig_tag, range, permission: None }); + Operation::Retag(RetagOp { info, new_tag, orig_tag, range, permission: None }); DiagnosticCxBuilder { machine, operation } } @@ -179,13 +182,19 @@ enum Operation { #[derive(Debug, Clone)] struct RetagOp { - cause: RetagCause, + info: RetagInfo, new_tag: BorTag, orig_tag: ProvenanceExtra, range: AllocRange, permission: Option, } +#[derive(Debug, Clone, Copy, PartialEq)] +pub struct RetagInfo { + pub cause: RetagCause, + pub in_field: bool, +} + #[derive(Debug, Clone, Copy, PartialEq)] pub enum RetagCause { Normal, @@ -258,11 +267,11 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { pub fn log_invalidation(&mut self, tag: BorTag) { let mut span = self.machine.current_span(); let (range, cause) = match &self.operation { - Operation::Retag(RetagOp { cause, range, permission, .. }) => { - if *cause == RetagCause::FnEntry { + Operation::Retag(RetagOp { info, range, permission, .. }) => { + if info.cause == RetagCause::FnEntry { span = self.machine.caller_span(); } - (*range, InvalidationCause::Retag(permission.unwrap(), *cause)) + (*range, InvalidationCause::Retag(permission.unwrap(), *info)) } Operation::Access(AccessOp { kind, range, .. }) => (*range, InvalidationCause::Access(*kind)), @@ -374,7 +383,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { ); err_sb_ub( format!("{action}{}", error_cause(stack, op.orig_tag)), - Some(operation_summary(&op.cause.summary(), self.history.id, op.range)), + Some(operation_summary(&op.info.summary(), self.history.id, op.range)), op.orig_tag.and_then(|orig_tag| self.get_logs_relevant_to(orig_tag, None)), ) } @@ -496,14 +505,18 @@ fn error_cause(stack: &Stack, prov_extra: ProvenanceExtra) -> &'static str { } } -impl RetagCause { +impl RetagInfo { fn summary(&self) -> String { - match self { + let mut s = match self.cause { RetagCause::Normal => "retag", RetagCause::FnEntry => "function-entry retag", RetagCause::InPlaceFnPassing => "in-place function argument/return passing protection", RetagCause::TwoPhase => "two-phase retag", } - .to_string() + .to_string(); + if self.in_field { + s.push_str(" (of a reference/box inside this compound value)"); + } + s } } diff --git a/src/borrow_tracker/stacked_borrows/mod.rs b/src/borrow_tracker/stacked_borrows/mod.rs index 7e89d3a0e8..5e1e0d7543 100644 --- a/src/borrow_tracker/stacked_borrows/mod.rs +++ b/src/borrow_tracker/stacked_borrows/mod.rs @@ -5,9 +5,11 @@ pub mod diagnostics; mod item; mod stack; -use log::trace; use std::cmp; use std::fmt::Write; +use std::mem; + +use log::trace; use rustc_data_structures::fx::FxHashSet; use rustc_middle::mir::{Mutability, RetagKind}; @@ -24,7 +26,7 @@ use crate::borrow_tracker::{ }; use crate::*; -use diagnostics::RetagCause; +use diagnostics::{RetagCause, RetagInfo}; pub use item::{Item, Permission}; pub use stack::Stack; @@ -623,7 +625,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' size: Size, new_perm: NewPermission, new_tag: BorTag, - retag_cause: RetagCause, // What caused this retag, for diagnostics only + retag_info: RetagInfo, // diagnostics info about this retag ) -> InterpResult<'tcx, Option> { let this = self.eval_context_mut(); @@ -670,7 +672,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' // FIXME: can this be done cleaner? let dcx = DiagnosticCxBuilder::retag( &this.machine, - retag_cause, + retag_info, new_tag, orig_tag, alloc_range(base_offset, size), @@ -761,7 +763,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' let global = machine.borrow_tracker.as_ref().unwrap().borrow(); let dcx = DiagnosticCxBuilder::retag( machine, - retag_cause, + retag_info, new_tag, orig_tag, alloc_range(base_offset, size), @@ -804,7 +806,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' let global = this.machine.borrow_tracker.as_ref().unwrap().borrow(); let dcx = DiagnosticCxBuilder::retag( &this.machine, - retag_cause, + retag_info, new_tag, orig_tag, alloc_range(base_offset, size), @@ -834,7 +836,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' &mut self, val: &ImmTy<'tcx, Provenance>, new_perm: NewPermission, - cause: RetagCause, // What caused this retag, for diagnostics only + info: RetagInfo, // diagnostics info about this retag ) -> 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. @@ -852,7 +854,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<' let new_tag = this.machine.borrow_tracker.as_mut().unwrap().get_mut().new_ptr(); // Reborrow. - let alloc_id = this.sb_reborrow(&place, size, new_perm, new_tag, cause)?; + let alloc_id = this.sb_reborrow(&place, size, new_perm, new_tag, info)?; // Adjust pointer. let new_place = place.map_provenance(|p| { @@ -886,12 +888,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { ) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> { let this = self.eval_context_mut(); let new_perm = NewPermission::from_ref_ty(val.layout.ty, kind, this); - let retag_cause = match kind { + let cause = match kind { RetagKind::TwoPhase { .. } => RetagCause::TwoPhase, RetagKind::FnEntry => unreachable!(), RetagKind::Raw | RetagKind::Default => RetagCause::Normal, }; - this.sb_retag_reference(val, new_perm, retag_cause) + this.sb_retag_reference(val, new_perm, RetagInfo { cause, in_field: false }) } fn sb_retag_place_contents( @@ -906,7 +908,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { RetagKind::FnEntry => RetagCause::FnEntry, RetagKind::Default => RetagCause::Normal, }; - let mut visitor = RetagVisitor { ecx: this, kind, retag_cause, retag_fields }; + let mut visitor = + RetagVisitor { ecx: this, kind, retag_cause, retag_fields, in_field: false }; return visitor.visit_value(place); // The actual visitor. @@ -915,6 +918,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { kind: RetagKind, retag_cause: RetagCause, retag_fields: RetagFields, + in_field: bool, } impl<'ecx, 'mir, 'tcx> RetagVisitor<'ecx, 'mir, 'tcx> { #[inline(always)] // yes this helps in our benchmarks @@ -922,10 +926,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { &mut self, place: &PlaceTy<'tcx, Provenance>, new_perm: NewPermission, - retag_cause: RetagCause, ) -> InterpResult<'tcx> { let val = self.ecx.read_immediate(&self.ecx.place_to_op(place)?)?; - let val = self.ecx.sb_retag_reference(&val, new_perm, retag_cause)?; + let val = self.ecx.sb_retag_reference( + &val, + new_perm, + RetagInfo { cause: self.retag_cause, in_field: self.in_field }, + )?; self.ecx.write_immediate(*val, place)?; Ok(()) } @@ -943,7 +950,7 @@ 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. let new_perm = NewPermission::from_box_ty(place.layout.ty, self.kind, self.ecx); - self.retag_ptr_inplace(place, new_perm, self.retag_cause) + self.retag_ptr_inplace(place, new_perm) } fn visit_value(&mut self, place: &PlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> { @@ -960,7 +967,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { ty::Ref(..) => { let new_perm = NewPermission::from_ref_ty(place.layout.ty, self.kind, self.ecx); - self.retag_ptr_inplace(place, new_perm, self.retag_cause)?; + self.retag_ptr_inplace(place, new_perm)?; } ty::RawPtr(..) => { // We do *not* want to recurse into raw pointers -- wide raw pointers have @@ -984,7 +991,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } }; if recurse { + let in_field = mem::replace(&mut self.in_field, true); // remember and restore old value self.walk_value(place)?; + self.in_field = in_field; } } } @@ -1011,7 +1020,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { access: Some(AccessKind::Write), protector: Some(ProtectorKind::StrongProtector), }; - let _new_ptr = this.sb_retag_reference(&ptr, new_perm, RetagCause::InPlaceFnPassing)?; + let _new_ptr = this.sb_retag_reference( + &ptr, + new_perm, + RetagInfo { cause: RetagCause::InPlaceFnPassing, in_field: false }, + )?; // We just throw away `new_ptr`, so nobody can access this memory while it is protected. Ok(()) diff --git a/tests/fail/both_borrows/buggy_split_at_mut.stack.stderr b/tests/fail/both_borrows/buggy_split_at_mut.stack.stderr index 0cb6111ecb..b957464f95 100644 --- a/tests/fail/both_borrows/buggy_split_at_mut.stack.stderr +++ b/tests/fail/both_borrows/buggy_split_at_mut.stack.stderr @@ -8,7 +8,7 @@ LL | | ) | | ^ | | | | |_____________trying to retag from for Unique 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..0x10] + | this error occurs as part of retag (of a reference/box inside this compound value) at ALLOC[0x0..0x10] | = 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/both_borrows/pass_invalid_shr_option.stack.stderr b/tests/fail/both_borrows/pass_invalid_shr_option.stack.stderr index e35d7918c0..96121f0659 100644 --- a/tests/fail/both_borrows/pass_invalid_shr_option.stack.stderr +++ b/tests/fail/both_borrows/pass_invalid_shr_option.stack.stderr @@ -5,7 +5,7 @@ LL | foo(some_xref); | ^^^^^^^^^ | | | 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] + | this error occurs as part of retag (of a reference/box inside this compound value) 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 diff --git a/tests/fail/both_borrows/pass_invalid_shr_tuple.stack.stderr b/tests/fail/both_borrows/pass_invalid_shr_tuple.stack.stderr index 10c566d061..0cfaf12355 100644 --- a/tests/fail/both_borrows/pass_invalid_shr_tuple.stack.stderr +++ b/tests/fail/both_borrows/pass_invalid_shr_tuple.stack.stderr @@ -5,7 +5,7 @@ LL | foo(pair_xref); | ^^^^^^^^^ | | | 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] + | this error occurs as part of retag (of a reference/box inside this compound value) 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 diff --git a/tests/fail/both_borrows/return_invalid_shr_option.stack.stderr b/tests/fail/both_borrows/return_invalid_shr_option.stack.stderr index f14e8b8532..d5b8433568 100644 --- a/tests/fail/both_borrows/return_invalid_shr_option.stack.stderr +++ b/tests/fail/both_borrows/return_invalid_shr_option.stack.stderr @@ -5,7 +5,7 @@ LL | ret | ^^^ | | | trying to retag from for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location - | this error occurs as part of retag at ALLOC[0x4..0x8] + | this error occurs as part of retag (of a reference/box inside this compound value) at ALLOC[0x4..0x8] | = 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/both_borrows/return_invalid_shr_tuple.stack.stderr b/tests/fail/both_borrows/return_invalid_shr_tuple.stack.stderr index 9ddaad4d1b..9ced0da96c 100644 --- a/tests/fail/both_borrows/return_invalid_shr_tuple.stack.stderr +++ b/tests/fail/both_borrows/return_invalid_shr_tuple.stack.stderr @@ -5,7 +5,7 @@ LL | ret | ^^^ | | | trying to retag from for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location - | this error occurs as part of retag at ALLOC[0x4..0x8] + | this error occurs as part of retag (of a reference/box inside this compound value) at ALLOC[0x4..0x8] | = 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/return_invalid_mut_option.stderr b/tests/fail/stacked_borrows/return_invalid_mut_option.stderr index ff00c54570..89b6cee7d9 100644 --- a/tests/fail/stacked_borrows/return_invalid_mut_option.stderr +++ b/tests/fail/stacked_borrows/return_invalid_mut_option.stderr @@ -5,7 +5,7 @@ LL | ret | ^^^ | | | trying to retag from for Unique permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location - | this error occurs as part of retag at ALLOC[0x4..0x8] + | this error occurs as part of retag (of a reference/box inside this compound value) at ALLOC[0x4..0x8] | = 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/return_invalid_mut_tuple.stderr b/tests/fail/stacked_borrows/return_invalid_mut_tuple.stderr index 61d041a881..388b00c714 100644 --- a/tests/fail/stacked_borrows/return_invalid_mut_tuple.stderr +++ b/tests/fail/stacked_borrows/return_invalid_mut_tuple.stderr @@ -5,7 +5,7 @@ LL | ret | ^^^ | | | trying to retag from for Unique permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location - | this error occurs as part of retag at ALLOC[0x4..0x8] + | this error occurs as part of retag (of a reference/box inside this compound value) at ALLOC[0x4..0x8] | = 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 ae3b96174cf9a0ef77e62217f8d2911356e74a2b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 21 Jul 2023 16:28:29 +0200 Subject: [PATCH 3/3] ask people to reach out if we declare too much UB --- .../stacked_borrows/diagnostics.rs | 26 ++++++++++++++----- src/borrow_tracker/stacked_borrows/mod.rs | 11 +------- src/diagnostics.rs | 5 ++-- .../buggy_split_at_mut.stack.stderr | 5 ++-- .../pass_invalid_shr_option.stack.stderr | 1 + .../pass_invalid_shr_tuple.stack.stderr | 1 + .../return_invalid_shr_option.stack.stderr | 1 + .../return_invalid_shr_tuple.stack.stderr | 1 + .../return_invalid_mut_option.stderr | 1 + .../return_invalid_mut_tuple.stderr | 1 + 10 files changed, 31 insertions(+), 22 deletions(-) diff --git a/src/borrow_tracker/stacked_borrows/diagnostics.rs b/src/borrow_tracker/stacked_borrows/diagnostics.rs index 33b777abd9..9b0f13dd62 100644 --- a/src/borrow_tracker/stacked_borrows/diagnostics.rs +++ b/src/borrow_tracker/stacked_borrows/diagnostics.rs @@ -6,11 +6,19 @@ 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::Permission, AccessKind, GlobalStateInner, ProtectorKind, }; use crate::*; +/// Error reporting +fn err_sb_ub<'tcx>( + msg: String, + help: Vec, + history: Option, +) -> InterpError<'tcx> { + err_machine_stop!(TerminationInfo::StackedBorrowsUb { msg, help, history }) +} + #[derive(Clone, Debug)] pub struct AllocHistory { id: AllocId, @@ -381,9 +389,13 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { self.history.id, self.offset.bytes(), ); + let mut helps = vec![operation_summary(&op.info.summary(), self.history.id, op.range)]; + if op.info.in_field { + helps.push(format!("errors for retagging in fields are fairly new; please reach out to us (e.g. at ) if you find this error troubling")); + } err_sb_ub( format!("{action}{}", error_cause(stack, op.orig_tag)), - Some(operation_summary(&op.info.summary(), self.history.id, op.range)), + helps, op.orig_tag.and_then(|orig_tag| self.get_logs_relevant_to(orig_tag, None)), ) } @@ -406,7 +418,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { ); err_sb_ub( format!("{action}{}", error_cause(stack, op.tag)), - Some(operation_summary("an access", self.history.id, op.range)), + vec![operation_summary("an access", self.history.id, op.range)], op.tag.and_then(|tag| self.get_logs_relevant_to(tag, None)), ) } @@ -432,7 +444,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { Operation::Dealloc(_) => err_sb_ub( format!("deallocating while item {item:?} is {protected} by call {call_id:?}",), - None, + vec![], None, ), Operation::Retag(RetagOp { orig_tag: tag, .. }) @@ -441,7 +453,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { format!( "not granting access to tag {tag:?} because that would remove {item:?} which is {protected} because it is an argument of call {call_id:?}", ), - None, + vec![], tag.and_then(|tag| self.get_logs_relevant_to(tag, Some(item.tag()))), ), } @@ -459,7 +471,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { alloc_id = self.history.id, cause = error_cause(stack, op.tag), ), - None, + vec![], op.tag.and_then(|tag| self.get_logs_relevant_to(tag, None)), ) } diff --git a/src/borrow_tracker/stacked_borrows/mod.rs b/src/borrow_tracker/stacked_borrows/mod.rs index 5e1e0d7543..1aed436e88 100644 --- a/src/borrow_tracker/stacked_borrows/mod.rs +++ b/src/borrow_tracker/stacked_borrows/mod.rs @@ -21,7 +21,7 @@ use rustc_middle::ty::{ use rustc_target::abi::{Abi, Size}; use crate::borrow_tracker::{ - stacked_borrows::diagnostics::{AllocHistory, DiagnosticCx, DiagnosticCxBuilder, TagHistory}, + stacked_borrows::diagnostics::{AllocHistory, DiagnosticCx, DiagnosticCxBuilder}, AccessKind, GlobalStateInner, ProtectorKind, RetagFields, }; use crate::*; @@ -170,15 +170,6 @@ impl NewPermission { } } -/// Error reporting -pub fn err_sb_ub<'tcx>( - msg: String, - help: Option, - history: Option, -) -> InterpError<'tcx> { - err_machine_stop!(TerminationInfo::StackedBorrowsUb { msg, help, history }) -} - // # Stacked Borrows Core Begin /// We need to make at least the following things true: diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 2a06bd871e..8d9901807e 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -22,7 +22,7 @@ pub enum TerminationInfo { UnsupportedInIsolation(String), StackedBorrowsUb { msg: String, - help: Option, + help: Vec, history: Option, }, TreeBorrowsUb { @@ -224,11 +224,10 @@ pub fn report_error<'tcx, 'mir>( (None, format!("or pass `-Zmiri-isolation-error=warn` to configure Miri to return an error code from isolated operations (if supported for that operation) and continue with a warning")), ], StackedBorrowsUb { help, history, .. } => { - let url = "https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md"; msg.extend(help.clone()); let mut helps = vec![ (None, format!("this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental")), - (None, format!("see {url} for further information")), + (None, format!("see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information")), ]; if let Some(TagHistory {created, invalidated, protected}) = history.clone() { helps.push((Some(created.1), created.0)); diff --git a/tests/fail/both_borrows/buggy_split_at_mut.stack.stderr b/tests/fail/both_borrows/buggy_split_at_mut.stack.stderr index b957464f95..daa4339225 100644 --- a/tests/fail/both_borrows/buggy_split_at_mut.stack.stderr +++ b/tests/fail/both_borrows/buggy_split_at_mut.stack.stderr @@ -7,8 +7,9 @@ LL | | from_raw_parts_mut(ptr.offset(mid as isize), len - mid), LL | | ) | | ^ | | | - | |_____________trying to retag from for Unique permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location - | this error occurs as part of retag (of a reference/box inside this compound value) at ALLOC[0x0..0x10] + | | trying to retag from for Unique permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location + | |_____________this error occurs as part of retag (of a reference/box inside this compound value) at ALLOC[0x0..0x10] + | errors for retagging in fields are fairly new; please reach out to us (e.g. at ) if you find this error troubling | = 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/both_borrows/pass_invalid_shr_option.stack.stderr b/tests/fail/both_borrows/pass_invalid_shr_option.stack.stderr index 96121f0659..26d9f38f23 100644 --- a/tests/fail/both_borrows/pass_invalid_shr_option.stack.stderr +++ b/tests/fail/both_borrows/pass_invalid_shr_option.stack.stderr @@ -6,6 +6,7 @@ LL | foo(some_xref); | | | 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 (of a reference/box inside this compound value) at ALLOC[0x0..0x4] + | errors for retagging in fields are fairly new; please reach out to us (e.g. at ) if you find this error troubling | = 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/both_borrows/pass_invalid_shr_tuple.stack.stderr b/tests/fail/both_borrows/pass_invalid_shr_tuple.stack.stderr index 0cfaf12355..5f0fbf1275 100644 --- a/tests/fail/both_borrows/pass_invalid_shr_tuple.stack.stderr +++ b/tests/fail/both_borrows/pass_invalid_shr_tuple.stack.stderr @@ -6,6 +6,7 @@ LL | foo(pair_xref); | | | 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 (of a reference/box inside this compound value) at ALLOC[0x0..0x4] + | errors for retagging in fields are fairly new; please reach out to us (e.g. at ) if you find this error troubling | = 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/both_borrows/return_invalid_shr_option.stack.stderr b/tests/fail/both_borrows/return_invalid_shr_option.stack.stderr index d5b8433568..7a9f061228 100644 --- a/tests/fail/both_borrows/return_invalid_shr_option.stack.stderr +++ b/tests/fail/both_borrows/return_invalid_shr_option.stack.stderr @@ -6,6 +6,7 @@ LL | ret | | | trying to retag from for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location | this error occurs as part of retag (of a reference/box inside this compound value) at ALLOC[0x4..0x8] + | errors for retagging in fields are fairly new; please reach out to us (e.g. at ) if you find this error troubling | = 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/both_borrows/return_invalid_shr_tuple.stack.stderr b/tests/fail/both_borrows/return_invalid_shr_tuple.stack.stderr index 9ced0da96c..6a98c9121e 100644 --- a/tests/fail/both_borrows/return_invalid_shr_tuple.stack.stderr +++ b/tests/fail/both_borrows/return_invalid_shr_tuple.stack.stderr @@ -6,6 +6,7 @@ LL | ret | | | trying to retag from for SharedReadOnly permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location | this error occurs as part of retag (of a reference/box inside this compound value) at ALLOC[0x4..0x8] + | errors for retagging in fields are fairly new; please reach out to us (e.g. at ) if you find this error troubling | = 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/return_invalid_mut_option.stderr b/tests/fail/stacked_borrows/return_invalid_mut_option.stderr index 89b6cee7d9..d357ab9639 100644 --- a/tests/fail/stacked_borrows/return_invalid_mut_option.stderr +++ b/tests/fail/stacked_borrows/return_invalid_mut_option.stderr @@ -6,6 +6,7 @@ LL | ret | | | trying to retag from for Unique permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location | this error occurs as part of retag (of a reference/box inside this compound value) at ALLOC[0x4..0x8] + | errors for retagging in fields are fairly new; please reach out to us (e.g. at ) if you find this error troubling | = 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/return_invalid_mut_tuple.stderr b/tests/fail/stacked_borrows/return_invalid_mut_tuple.stderr index 388b00c714..d346e6fa89 100644 --- a/tests/fail/stacked_borrows/return_invalid_mut_tuple.stderr +++ b/tests/fail/stacked_borrows/return_invalid_mut_tuple.stderr @@ -6,6 +6,7 @@ LL | ret | | | trying to retag from for Unique permission at ALLOC[0x4], but that tag does not exist in the borrow stack for this location | this error occurs as part of retag (of a reference/box inside this compound value) at ALLOC[0x4..0x8] + | errors for retagging in fields are fairly new; please reach out to us (e.g. at ) if you find this error troubling | = 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