diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 0e3e693e33f..230f46c569d 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -8,7 +8,7 @@ use rustc_middle::ty; use rustc_span::{source_map::DUMMY_SP, Span, SpanData, Symbol}; use crate::helpers::HexRange; -use crate::stacked_borrows::{diagnostics::TagHistory, AccessKind, SbTag}; +use crate::stacked_borrows::{diagnostics::TagHistory, AccessKind}; use crate::*; /// Details of premature program termination. @@ -61,9 +61,9 @@ impl MachineStopType for TerminationInfo {} /// Miri specific diagnostics pub enum NonHaltingDiagnostic { CreatedPointerTag(NonZeroU64), - /// This `Item` was popped from the borrow stack, either due to a grant of - /// `AccessKind` to `SbTag` or a deallocation when the second argument is `None`. - PoppedPointerTag(Item, Option<(SbTag, AccessKind)>), + /// This `Item` was popped from the borrow stack, either due to an access with the given tag or + /// a deallocation when the second argument is `None`. + PoppedPointerTag(Item, Option<(SbTagExtra, AccessKind)>), CreatedCallId(CallId), CreatedAlloc(AllocId), FreedAlloc(AllocId), diff --git a/src/intptrcast.rs b/src/intptrcast.rs index 9ec96b8f491..cfaf61f9d5c 100644 --- a/src/intptrcast.rs +++ b/src/intptrcast.rs @@ -142,9 +142,7 @@ impl<'mir, 'tcx> GlobalStateInner { // Determine the allocation this points to at cast time. let alloc_id = Self::alloc_id_from_addr(ecx, addr); Pointer::new( - alloc_id.map(|alloc_id| { - Tag::Concrete(ConcreteTag { alloc_id, sb: SbTag::Untagged }) - }), + alloc_id.map(|alloc_id| Tag::Concrete { alloc_id, sb: SbTag::Untagged }), Size::from_bytes(addr), ) } @@ -222,8 +220,8 @@ impl<'mir, 'tcx> GlobalStateInner { ) -> Option<(AllocId, Size)> { let (tag, addr) = ptr.into_parts(); // addr is absolute (Tag provenance) - let alloc_id = if let Tag::Concrete(concrete) = tag { - concrete.alloc_id + let alloc_id = if let Tag::Concrete { alloc_id, .. } = tag { + alloc_id } else { // A wildcard pointer. assert_eq!(ecx.machine.intptrcast.borrow().provenance_mode, ProvenanceMode::Permissive); diff --git a/src/lib.rs b/src/lib.rs index 432f469733c..68489c9b47b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,6 +7,7 @@ #![feature(io_error_more)] #![feature(yeet_expr)] #![feature(is_some_with)] +#![feature(nonzero_ops)] #![warn(rust_2018_idioms)] #![allow( clippy::collapsible_else_if, @@ -81,15 +82,15 @@ pub use crate::eval::{ pub use crate::helpers::{CurrentSpan, EvalContextExt as HelpersEvalContextExt}; pub use crate::intptrcast::ProvenanceMode; pub use crate::machine::{ - AllocExtra, ConcreteTag, Evaluator, FrameData, MiriEvalContext, MiriEvalContextExt, - MiriMemoryKind, Tag, NUM_CPUS, PAGE_SIZE, STACK_ADDR, STACK_SIZE, + AllocExtra, Evaluator, FrameData, MiriEvalContext, MiriEvalContextExt, MiriMemoryKind, Tag, + NUM_CPUS, PAGE_SIZE, STACK_ADDR, STACK_SIZE, }; pub use crate::mono_hash_map::MonoHashMap; pub use crate::operator::EvalContextExt as OperatorEvalContextExt; pub use crate::range_map::RangeMap; pub use crate::stacked_borrows::{ - CallId, EvalContextExt as StackedBorEvalContextExt, Item, Permission, PtrId, SbTag, Stack, - Stacks, + CallId, EvalContextExt as StackedBorEvalContextExt, Item, Permission, PtrId, SbTag, SbTagExtra, + Stack, Stacks, }; pub use crate::sync::{CondvarId, EvalContextExt as SyncEvalContextExt, MutexId, RwLockId}; pub use crate::thread::{ diff --git a/src/machine.rs b/src/machine.rs index 3704a538514..5810ac6d7ec 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -130,17 +130,14 @@ impl fmt::Display for MiriMemoryKind { /// Pointer provenance (tag). #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum Tag { - Concrete(ConcreteTag), + Concrete { + alloc_id: AllocId, + /// Stacked Borrows tag. + sb: SbTag, + }, Wildcard, } -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub struct ConcreteTag { - pub alloc_id: AllocId, - /// Stacked Borrows tag. - pub sb: SbTag, -} - #[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))] static_assert_size!(Pointer, 24); // #[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))] @@ -160,15 +157,15 @@ impl Provenance for Tag { write!(f, "0x{:x}", addr.bytes())?; match tag { - Tag::Concrete(tag) => { + Tag::Concrete { alloc_id, sb } => { // Forward `alternate` flag to `alloc_id` printing. if f.alternate() { - write!(f, "[{:#?}]", tag.alloc_id)?; + write!(f, "[{:#?}]", alloc_id)?; } else { - write!(f, "[{:?}]", tag.alloc_id)?; + write!(f, "[{:?}]", alloc_id)?; } // Print Stacked Borrows tag. - write!(f, "{:?}", tag.sb)?; + write!(f, "{:?}", sb)?; } Tag::Wildcard => { write!(f, "[Wildcard]")?; @@ -180,7 +177,7 @@ impl Provenance for Tag { fn get_alloc_id(self) -> Option { match self { - Tag::Concrete(concrete) => Some(concrete.alloc_id), + Tag::Concrete { alloc_id, .. } => Some(alloc_id), Tag::Wildcard => None, } } @@ -489,8 +486,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { type AllocExtra = AllocExtra; type PointerTag = Tag; - // `None` represents a wildcard pointer. - type TagExtra = Option; + type TagExtra = SbTagExtra; type MemoryMap = MonoHashMap, Allocation)>; @@ -683,7 +679,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { SbTag::Untagged }; Pointer::new( - Tag::Concrete(ConcreteTag { alloc_id: ptr.provenance, sb: sb_tag }), + Tag::Concrete { alloc_id: ptr.provenance, sb: sb_tag }, Size::from_bytes(absolute_addr), ) } @@ -709,7 +705,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { ptr: Pointer, ) -> InterpResult<'tcx> { match ptr.provenance { - Tag::Concrete(ConcreteTag { alloc_id, sb }) => { + Tag::Concrete { alloc_id, sb } => { intptrcast::GlobalStateInner::expose_ptr(ecx, alloc_id, sb); } Tag::Wildcard => { @@ -730,8 +726,8 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> { rel.map(|(alloc_id, size)| { let sb = match ptr.provenance { - Tag::Concrete(ConcreteTag { sb, .. }) => Some(sb), - Tag::Wildcard => None, + Tag::Concrete { sb, .. } => SbTagExtra::Concrete(sb), + Tag::Wildcard => SbTagExtra::Wildcard, }; (alloc_id, size, sb) }) diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 8411d85f0e0..b7b339ce77b 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -3,6 +3,7 @@ use log::trace; use std::cell::RefCell; +use std::cmp; use std::fmt; use std::num::NonZeroU64; @@ -60,6 +61,32 @@ impl fmt::Debug for SbTag { } } +/// The "extra" information an SB pointer has over a regular AllocId. +/// Newtype for `Option`. +#[derive(Copy, Clone)] +pub enum SbTagExtra { + Concrete(SbTag), + Wildcard, +} + +impl fmt::Debug for SbTagExtra { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + SbTagExtra::Concrete(tag) => write!(f, "{tag:?}"), + SbTagExtra::Wildcard => write!(f, ""), + } + } +} + +impl SbTagExtra { + fn and_then(self, f: impl FnOnce(SbTag) -> Option) -> Option { + match self { + SbTagExtra::Concrete(tag) => f(tag), + SbTagExtra::Wildcard => None, + } + } +} + /// Indicates which permission is granted (by this item to some pointers) #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)] pub enum Permission { @@ -108,6 +135,8 @@ pub struct Stack { /// wildcard pointers are used to access this location. What we do know is that `borrows` are at /// the top of the stack, and below it are arbitrarily many items whose `tag` is either /// `Untagged` or strictly less than `id`. + /// When the bottom is unknown, `borrows` always has a `SharedReadOnly` or `Unique` at the bottom; + /// we never have the unknown-to-known boundary in an SRW group. unknown_bottom: Option, } @@ -289,35 +318,37 @@ impl Permission { /// Core per-location operations: access, dealloc, reborrow. impl<'tcx> Stack { /// Find the item granting the given kind of access to the given tag, and return where - /// it is on the stack. - /// `Ok(None)` indicates it matched the "unknown" part of the stack, or it was a wildcard tag - /// and we have no clue what exactly it matched (but it could have matched something) + /// it is on the stack. For wildcard tags, the given index is approximate, but if *no* + /// index is given it means the match was *not* in the known part of the stack. + /// `Ok(None)` indicates it matched the "unknown" part of the stack. /// `Err` indicates it was not found. fn find_granting( &self, access: AccessKind, - tag: Option, + tag: SbTagExtra, exposed_tags: &FxHashSet, ) -> Result, ()> { - let Some(tag) = tag else { + let SbTagExtra::Concrete(tag) = tag else { // Handle the wildcard case. // Go search the stack for an exposed tag. - let maybe_in_stack = self - .borrows - .iter() - .rev() // search top-to-bottom - .find_map(|item| { - // If the item fits and *might* be this wildcard, use it. - if item.perm.grants(access) && exposed_tags.contains(&item.tag) { - Some(()) - } else { - None - } - }) - .is_some(); + if let Some(idx) = + self.borrows + .iter() + .enumerate() // we also need to know *where* in the stack + .rev() // search top-to-bottom + .find_map(|(idx, item)| { + // If the item fits and *might* be this wildcard, use it. + if item.perm.grants(access) && exposed_tags.contains(&item.tag) { + Some(idx) + } else { + None + } + }) + { + return Ok(Some(idx)); + } // If we couldn't find it in the stack, check the unknown bottom. - let found = maybe_in_stack || self.unknown_bottom.is_some(); - return if found { Ok(None) } else { Err(()) }; + return if self.unknown_bottom.is_some() { Ok(None) } else { Err(()) }; }; if let Some(idx) = @@ -351,8 +382,10 @@ impl<'tcx> Stack { match perm { Permission::SharedReadOnly => bug!("Cannot use SharedReadOnly for writing"), Permission::Disabled => bug!("Cannot use Disabled for anything"), - // On a write, everything above us is incompatible. - Permission::Unique => granting + 1, + Permission::Unique => { + // On a write, everything above us is incompatible. + granting + 1 + } Permission::SharedReadWrite => { // The SharedReadWrite *just* above us are compatible, to skip those. let mut idx = granting + 1; @@ -380,7 +413,7 @@ impl<'tcx> Stack { /// currently checking. fn check_protector( item: &Item, - provoking_access: Option<(SbTag, AllocRange, Size, AccessKind)>, // just for debug printing and error messages + provoking_access: Option<(SbTagExtra, AllocRange, Size, AccessKind)>, // just for debug printing and error messages global: &GlobalStateInner, alloc_history: &mut AllocHistory, ) -> InterpResult<'tcx> { @@ -401,12 +434,14 @@ impl<'tcx> Stack { tag, item ), None, - alloc_history.get_logs_relevant_to( - tag, - alloc_range, - offset, - Some(item.tag), - ), + tag.and_then(|tag| { + alloc_history.get_logs_relevant_to( + tag, + alloc_range, + offset, + Some(item.tag), + ) + }), ))? } else { Err(err_sb_ub( @@ -427,7 +462,7 @@ impl<'tcx> Stack { fn access( &mut self, access: AccessKind, - tag: Option, + tag: SbTagExtra, (alloc_id, alloc_range, offset): (AllocId, AllocRange, Size), // just for debug printing and error messages global: &mut GlobalStateInner, current_span: &mut CurrentSpan<'_, '_, 'tcx>, @@ -441,22 +476,22 @@ impl<'tcx> Stack { alloc_history.access_error(access, tag, alloc_id, alloc_range, offset, self) })?; - let Some(granting_idx) = granting_idx else { - // The access used a wildcard pointer or matched the unknown bottom. - // Nobody knows what happened, so forget everything. - trace!("access: clearing stack due to wildcard"); - self.borrows.clear(); - self.unknown_bottom = Some(global.next_ptr_id); - return Ok(()); - }; - let tag = tag.unwrap(); // only precise tags have precise locations - // Step 2: Remove incompatible items above them. Make sure we do not remove protected // items. Behavior differs for reads and writes. + // In case of wildcards/unknown matches, we remove everything that is *definitely* gone. if access == AccessKind::Write { // Remove everything above the write-compatible items, like a proper stack. This makes sure read-only and unique // pointers become invalid on write accesses (ensures F2a, and ensures U2 for write accesses). - let first_incompatible_idx = self.find_first_write_incompatible(granting_idx); + let first_incompatible_idx = if let Some(granting_idx) = granting_idx { + // The granting_idx *might* be approximate, but any lower idx would remove more + // things. Even if this is a Unique and the lower idx is an SRW (which removes + // less), there is an SRW group boundary here so strictly more would get removed. + self.find_first_write_incompatible(granting_idx) + } else { + // We are writing to something in the unknown part. + // There is a SRW group boundary between the unknown and the known, so everything is incompatible. + 0 + }; for item in self.borrows.drain(first_incompatible_idx..).rev() { trace!("access: popping item {:?}", item); Stack::check_protector( @@ -476,7 +511,13 @@ impl<'tcx> Stack { // This pattern occurs a lot in the standard library: create a raw pointer, then also create a shared // reference and use that. // We *disable* instead of removing `Unique` to avoid "connecting" two neighbouring blocks of SRWs. - let first_incompatible_idx = granting_idx + 1; + let first_incompatible_idx = if let Some(granting_idx) = granting_idx { + // The granting_idx *might* be approximate, but any lower idx would disable more things. + granting_idx + 1 + } else { + // We are reading from something in the unknown part. That means *all* `Unique` we know about are dead now. + 0 + }; for idx in (first_incompatible_idx..self.borrows.len()).rev() { let item = &mut self.borrows[idx]; @@ -494,6 +535,31 @@ impl<'tcx> Stack { } } + // If this was an approximate action, we now collapse everything into an unknown. + if granting_idx.is_none() || matches!(tag, SbTagExtra::Wildcard) { + // Compute the upper bound of the items that remain. + // (This is why we did all the work above: to reduce the items we have to consider here.) + let mut max = NonZeroU64::new(1).unwrap(); + for item in &self.borrows { + // Skip disabled items, they cannot be matched anyway. + if !matches!(item.perm, Permission::Disabled) { + if let SbTag::Tagged(tag) = item.tag { + // We are looking for a strict upper bound, so add 1 to this tag. + max = cmp::max(tag.checked_add(1).unwrap(), max); + } + } + } + if let Some(unk) = self.unknown_bottom { + max = cmp::max(unk, max); + } + // Use `max` as new strict upper bound for everything. + trace!( + "access: forgetting stack to upper bound {max} due to wildcard or unknown access" + ); + self.borrows.clear(); + self.unknown_bottom = Some(max); + } + // Done. Ok(()) } @@ -502,7 +568,7 @@ impl<'tcx> Stack { /// active protectors at all because we will remove all items. fn dealloc( &mut self, - tag: Option, + tag: SbTagExtra, (alloc_id, alloc_range, offset): (AllocId, AllocRange, Size), // just for debug printing and error messages global: &GlobalStateInner, alloc_history: &mut AllocHistory, @@ -534,7 +600,7 @@ impl<'tcx> Stack { /// `range` that we are currently checking. fn grant( &mut self, - derived_from: Option, + derived_from: SbTagExtra, new: Item, (alloc_id, alloc_range, offset): (AllocId, AllocRange, Size), // just for debug printing and error messages global: &mut GlobalStateInner, @@ -562,10 +628,12 @@ impl<'tcx> Stack { "this case only makes sense for stack-like accesses" ); - let Some(granting_idx) = granting_idx else { + let (Some(granting_idx), SbTagExtra::Concrete(_)) = (granting_idx, derived_from) else { // The parent is a wildcard pointer or matched the unknown bottom. - // Nobody knows what happened, so forget everything. - trace!("reborrow: clearing stack due to wildcard"); + // This is approximate. Nobody knows what happened, so forget everything. + // The new thing is SRW anyway, so we cannot push it "on top of the unkown part" + // (for all we know, it might join an SRW group inside the unknown). + trace!("reborrow: forgetting stack entirely due to SharedReadWrite reborrow from wildcard or unknown"); self.borrows.clear(); self.unknown_bottom = Some(global.next_ptr_id); return Ok(()); @@ -723,7 +791,7 @@ impl Stacks { pub fn memory_read<'tcx>( &self, alloc_id: AllocId, - tag: Option, + tag: SbTagExtra, range: AllocRange, state: &GlobalState, mut current_span: CurrentSpan<'_, '_, 'tcx>, @@ -752,7 +820,7 @@ impl Stacks { pub fn memory_written<'tcx>( &mut self, alloc_id: AllocId, - tag: Option, + tag: SbTagExtra, range: AllocRange, state: &GlobalState, mut current_span: CurrentSpan<'_, '_, 'tcx>, @@ -781,7 +849,7 @@ impl Stacks { pub fn memory_deallocated<'tcx>( &mut self, alloc_id: AllocId, - tag: Option, + tag: SbTagExtra, range: AllocRange, state: &GlobalState, ) -> InterpResult<'tcx> { @@ -798,6 +866,8 @@ impl Stacks { /// to grant for which references, and when to add protectors. impl<'mir, 'tcx: 'mir> EvalContextPrivExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { + /// Returns the `AllocId` the reborrow was done in, if some actual borrow stack manipulation + /// happened. fn reborrow( &mut self, place: &MPlaceTy<'tcx, Tag>, @@ -805,7 +875,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx kind: RefKind, new_tag: SbTag, protect: bool, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Option> { let this = self.eval_context_mut(); let current_span = &mut this.machine.current_span(); @@ -815,6 +885,10 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx base_offset, orig_tag| -> InterpResult<'tcx> { + let SbTagExtra::Concrete(orig_tag) = orig_tag else { + // FIXME: should we log this? + return Ok(()) + }; let extra = this.get_alloc_extra(alloc_id)?; let stacked_borrows = extra.stacked_borrows.as_ref().expect("we should have Stacked Borrows data"); @@ -832,6 +906,13 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx }; if size == Size::ZERO { + trace!( + "reborrow of size 0: {} reference {:?} derived from {:?} (pointee {})", + kind, + new_tag, + place.ptr, + place.layout.ty, + ); // Don't update any stacks for a zero-sized access; borrow stacks are per-byte and this // touches no bytes so there is no stack to put this tag in. // However, if the pointer for this operation points at a real allocation we still @@ -840,24 +921,14 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // Dangling slices are a common case here; it's valid to get their length but with raw // pointer tagging for example all calls to get_unchecked on them are invalid. if let Ok((alloc_id, base_offset, orig_tag)) = this.ptr_try_get_alloc_id(place.ptr) { - if let Some(orig_tag) = orig_tag { - log_creation(this, current_span, alloc_id, base_offset, orig_tag)?; - } + log_creation(this, current_span, alloc_id, base_offset, orig_tag)?; + return Ok(Some(alloc_id)); } - - trace!( - "reborrow of size 0: {} reference {:?} derived from {:?} (pointee {})", - kind, - new_tag, - place.ptr, - place.layout.ty, - ); - return Ok(()); + // This pointer doesn't come with an AllocId. :shrug: + return Ok(None); } let (alloc_id, base_offset, orig_tag) = this.ptr_get_alloc_id(place.ptr)?; - if let Some(orig_tag) = orig_tag { - log_creation(this, current_span, alloc_id, base_offset, orig_tag)?; - } + log_creation(this, current_span, alloc_id, base_offset, orig_tag)?; // Ensure we bail out if the pointer goes out-of-bounds (see miri#1050). let (alloc_size, _) = @@ -937,7 +1008,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx ) }) })?; - return Ok(()); + return Ok(Some(alloc_id)); } }; // Here we can avoid `borrow()` calls because we have mutable references. @@ -962,7 +1033,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx ) })?; - Ok(()) + Ok(Some(alloc_id)) } /// Retags an indidual pointer, returning the retagged version. @@ -997,16 +1068,22 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx }; // Reborrow. - this.reborrow(&place, size, kind, new_tag, protect)?; + let alloc_id = this.reborrow(&place, size, kind, new_tag, protect)?; // Adjust pointer. let new_place = place.map_provenance(|p| { - p.map(|t| { - // TODO: Fix this eventually - if let Tag::Concrete(t) = t { - Tag::Concrete(ConcreteTag { sb: new_tag, ..t }) - } else { - t + p.map(|prov| { + match alloc_id { + Some(alloc_id) => { + // If `reborrow` could figure out the AllocId of this ptr, hard-code it into the new one. + // Even if we started out with a wildcard, this newly retagged pointer is tied to that allocation. + Tag::Concrete { alloc_id, sb: new_tag } + } + None => { + // Looks like this has to stay a wildcard pointer. + assert!(matches!(prov, Tag::Wildcard)); + Tag::Wildcard + } } }) }); diff --git a/src/stacked_borrows/diagnostics.rs b/src/stacked_borrows/diagnostics.rs index 91dfe22c196..d3c706c1404 100644 --- a/src/stacked_borrows/diagnostics.rs +++ b/src/stacked_borrows/diagnostics.rs @@ -4,12 +4,11 @@ use rustc_middle::mir::interpret::{AllocId, AllocRange}; use rustc_span::{Span, SpanData}; use rustc_target::abi::Size; -use core::fmt::Debug; - use crate::helpers::{CurrentSpan, HexRange}; use crate::stacked_borrows::{err_sb_ub, AccessKind, Permission}; use crate::Item; use crate::SbTag; +use crate::SbTagExtra; use crate::Stack; use rustc_middle::mir::interpret::InterpError; @@ -199,19 +198,15 @@ impl AllocHistory { /// Report a descriptive error when `new` could not be granted from `derived_from`. pub fn grant_error<'tcx>( &self, - derived_from: Option, + derived_from: SbTagExtra, new: Item, alloc_id: AllocId, alloc_range: AllocRange, error_offset: Size, stack: &Stack, ) -> InterpError<'tcx> { - // TODO: Fix this properly - let z = &derived_from; - let f = if let Some(ref t) = z { t as &dyn Debug } else { &"" as &dyn Debug }; let action = format!( - "trying to reborrow {:?} for {:?} permission at {}[{:#x}]", - f, + "trying to reborrow {derived_from:?} for {:?} permission at {}[{:#x}]", new.perm, alloc_id, error_offset.bytes(), @@ -229,18 +224,14 @@ impl AllocHistory { pub fn access_error<'tcx>( &self, access: AccessKind, - tag: Option, + tag: SbTagExtra, alloc_id: AllocId, alloc_range: AllocRange, error_offset: Size, stack: &Stack, ) -> InterpError<'tcx> { - let z = &tag; - let f = if let Some(ref t) = z { t as &dyn Debug } else { &"" as &dyn Debug }; let action = format!( - "attempting a {} using {:?} at {}[{:#x}]", - access, - f, + "attempting a {access} using {tag:?} at {}[{:#x}]", alloc_id, error_offset.bytes(), ); @@ -260,8 +251,8 @@ fn operation_summary( format!("this error occurs as part of {} at {:?}{}", operation, alloc_id, HexRange(alloc_range)) } -fn error_cause(stack: &Stack, tag: Option) -> &'static str { - if let Some(tag) = tag { +fn error_cause(stack: &Stack, tag: SbTagExtra) -> &'static str { + if let SbTagExtra::Concrete(tag) = tag { if stack.borrows.iter().any(|item| item.tag == tag && item.perm != Permission::Disabled) { ", but that tag only grants SharedReadOnly permission for this location" } else { diff --git a/tests/fail/stacked_borrows/exposed_only_ro.stderr b/tests/fail/stacked_borrows/exposed_only_ro.stderr index 9748fe78c71..ceeca61e558 100644 --- a/tests/fail/stacked_borrows/exposed_only_ro.stderr +++ b/tests/fail/stacked_borrows/exposed_only_ro.stderr @@ -1,10 +1,10 @@ -error: Undefined Behavior: attempting a write access using "" at ALLOC[0x0], but no exposed tags are valid in the borrow stack for this location +error: Undefined Behavior: attempting a write access using at ALLOC[0x0], but no exposed tags are valid in the borrow stack for this location --> $DIR/exposed_only_ro.rs:LL:CC | LL | unsafe { *ptr = 0 }; | ^^^^^^^^ | | - | attempting a write access using "" at ALLOC[0x0], but no exposed tags are valid in the borrow stack for this location + | attempting a write access using at ALLOC[0x0], but no exposed tags are valid 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 diff --git a/tests/fail/stacked_borrows/illegal_read_despite_exposed1.rs b/tests/fail/stacked_borrows/illegal_read_despite_exposed1.rs new file mode 100644 index 00000000000..61a5e05d34c --- /dev/null +++ b/tests/fail/stacked_borrows/illegal_read_despite_exposed1.rs @@ -0,0 +1,17 @@ +// compile-flags: -Zmiri-permissive-provenance + +fn main() { + unsafe { + let root = &mut 42; + let addr = root as *mut i32 as usize; + let exposed_ptr = addr as *mut i32; + // From the exposed ptr, we get a new unique ptr. + let root2 = &mut *exposed_ptr; + let _fool = root2 as *mut _; // this would have fooled the old untagged pointer logic + // Stack: Unknown( at ALLOC[0x0], but that tag does not exist in the borrow stack for this location + --> $DIR/illegal_read_despite_exposed1.rs:LL:CC + | +LL | let _val = *root2; + | ^^^^^^ + | | + | 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 + + = note: inside `main` at $DIR/illegal_read_despite_exposed1.rs:LL:CC + +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/illegal_read_despite_exposed2.rs b/tests/fail/stacked_borrows/illegal_read_despite_exposed2.rs new file mode 100644 index 00000000000..b45b7b5d285 --- /dev/null +++ b/tests/fail/stacked_borrows/illegal_read_despite_exposed2.rs @@ -0,0 +1,18 @@ +// compile-flags: -Zmiri-permissive-provenance + +fn main() { + unsafe { + let root = &mut 42; + let addr = root as *mut i32 as usize; + let exposed_ptr = addr as *mut i32; + // From the exposed ptr, we get a new unique ptr. + let root2 = &mut *exposed_ptr; + // let _fool = root2 as *mut _; // this would [fool] us, since SRW(N+1) remains on the stack + // Stack: Unknown( at ALLOC[0x0], but that tag does not exist in the borrow stack for this location + --> $DIR/illegal_read_despite_exposed2.rs:LL:CC + | +LL | let _val = *root2; + | ^^^^^^ + | | + | 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 + + = note: inside `main` at $DIR/illegal_read_despite_exposed2.rs:LL:CC + +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/illegal_write_despite_exposed1.rs b/tests/fail/stacked_borrows/illegal_write_despite_exposed1.rs new file mode 100644 index 00000000000..b50399b9df5 --- /dev/null +++ b/tests/fail/stacked_borrows/illegal_write_despite_exposed1.rs @@ -0,0 +1,16 @@ +// compile-flags: -Zmiri-permissive-provenance + +fn main() { + unsafe { + let root = &mut 42; + let addr = root as *mut i32 as usize; + let exposed_ptr = addr as *mut i32; + // From the exposed ptr, we get a new SRO ptr. + let root2 = &*exposed_ptr; + // Stack: Unknown( at ALLOC[0x0], but that tag does not exist in the borrow stack for this location + --> $DIR/illegal_write_despite_exposed1.rs:LL:CC + | +LL | let _val = *root2; + | ^^^^^^ + | | + | 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 + + = note: inside `main` at $DIR/illegal_write_despite_exposed1.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error +