From 45a8589c91e58a0145ca34053e436c29953424aa Mon Sep 17 00:00:00 2001 From: carbotaniuman <41451839+carbotaniuman@users.noreply.github.com> Date: Wed, 15 Jun 2022 14:30:26 -0500 Subject: [PATCH] Actually commit the work --- src/stacked_borrows.rs | 219 +++++++++++------------ src/stacked_borrows/diagnostics.rs | 7 +- tests/pass/permissive_provenance_sb.rs | 69 ------- tests/pass/stacked-borrows/int-to-ptr.rs | 72 +++++++- 4 files changed, 176 insertions(+), 191 deletions(-) delete mode 100644 tests/pass/permissive_provenance_sb.rs diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 61f7b2110c..db49e361e0 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -78,21 +78,6 @@ impl fmt::Debug for Item { } } -#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)] -pub enum ItemOrUnknown { - Item(Item), - Unknown(PtrId), -} - -impl ItemOrUnknown { - fn item(self) -> Option { - match self { - ItemOrUnknown::Item(item) => Some(item), - ItemOrUnknown::Unknown(_) => None, - } - } -} - /// Extra per-location state. #[derive(Clone, Debug, PartialEq, Eq)] pub struct Stack { @@ -100,7 +85,7 @@ pub struct Stack { /// Invariants: /// * Above a `SharedReadOnly` there can only be more `SharedReadOnly`. /// * Except for `Untagged`, no tag occurs in the stack more than once. - borrows: Vec, + borrows: Vec, /// Whether the borrow stack extends for an unknown amount of items at the bottom /// of the stack, whose PtrId of it's SbTag is less than this. // TODO: reword this @@ -115,7 +100,7 @@ pub struct Stacks { /// Stores past operations on this allocation history: RefCell, /// The tags that have been exposed - exposed_tags: FxHashSet, + exposed_tags: RefCell>, } /// Extra global state, available to the memory access hooks. @@ -293,47 +278,55 @@ impl Permission { impl<'tcx> Stack { /// Find the item granting the given kind of access to the given tag, and return where /// it is on the stack. + // TODO: Doc ok with Some(index) or None if unknown_bottom used + // Err if does not match fn find_granting( &self, access: AccessKind, tag: Option, exposed_tags: &FxHashSet, - ) -> Option { - self.borrows + ) -> Result, ()> { + let res = self + .borrows .iter() .enumerate() // we also need to know *where* in the stack .rev() // search top-to-bottom // Return permission of first item that grants access. // We require a permission with the right tag, ensuring U3 and F3. .find_map(|(idx, item)| { - match item { - ItemOrUnknown::Unknown(id) => - match tag { - Some(tag) => - match tag { - SbTag::Tagged(tag_id) if tag_id < *id => Some(idx), - SbTag::Untagged => todo!(), - _ => None, - }, - None => Some(idx), - }, - ItemOrUnknown::Item(item) => + match tag { + Some(tag) if tag == item.tag && item.perm.grants(access) => Some(idx), + None if exposed_tags.contains(&item.tag) => Some(idx), + _ => None, + } + }); + + if res.is_some() { + return Ok(res); + } + + match self.unknown_bottom { + Some(id) => + match tag { + Some(tag) => match tag { - Some(tag) if tag == item.tag && item.perm.grants(access) => Some(idx), - None if exposed_tags.contains(&item.tag) => Some(idx), - _ => None, + SbTag::Tagged(tag_id) if tag_id < id => Ok(None), + SbTag::Untagged => Ok(None), + _ => Err(()), }, - } - }) + None => Ok(None), + }, + None => Err(()), + } } /// Find the first write-incompatible item above the given one -- /// i.e, find the height to which the stack will be truncated when writing to `granting`. - fn find_first_write_incompatible(&self, granting: usize) -> usize { - let perm = if let ItemOrUnknown::Item(item) = self.borrows[granting] { - item.perm + fn find_first_write_incompatible(&self, granting: Option) -> usize { + let perm = if let Some(idx) = granting { + self.borrows[idx].perm } else { - // TODO: probably the least restirctive? + // I assume this has to be it? Permission::SharedReadWrite }; @@ -341,21 +334,23 @@ impl<'tcx> Stack { 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 => + if let Some(idx) = granting { + idx + 1 + } else { + 0 + }, Permission::SharedReadWrite => { // The SharedReadWrite *just* above us are compatible, to skip those. - let mut idx = granting + 1; + let mut idx = if let Some(idx) = granting { idx + 1 } else { 0 }; + while let Some(item) = self.borrows.get(idx) { - if let ItemOrUnknown::Item(item) = item { - if item.perm == Permission::SharedReadWrite { - // Go on. - idx += 1; - } else { - // Found first incompatible! - break; - } + if item.perm == Permission::SharedReadWrite { + // Go on. + idx += 1; } else { - bug!("Unknown found in the middle of the stack") + // Found first incompatible! + break; } } idx @@ -430,10 +425,9 @@ impl<'tcx> Stack { // Two main steps: Find granting item, remove incompatible items above. // Step 1: Find granting item. - let granting_idx = - self.find_granting(access, tag, exposed_tags).ok_or_else(|| { - alloc_history.access_error(access, tag, alloc_id, alloc_range, offset, self) - })?; + let granting_idx = self.find_granting(access, tag, exposed_tags).map_err(|_| { + alloc_history.access_error(access, tag, alloc_id, alloc_range, offset, self) + })?; // Step 2: Remove incompatible items above them. Make sure we do not remove protected // items. Behavior differs for reads and writes. @@ -443,20 +437,17 @@ impl<'tcx> Stack { // 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); for item in self.borrows.drain(first_incompatible_idx..).rev() { - if let ItemOrUnknown::Item(item) = item { - trace!("access: popping item {:?}", item); - Stack::check_protector( - &item, - Some((tag, alloc_range, offset, access)), - global, - alloc_history, - )?; - alloc_history.log_invalidation(item.tag, alloc_range, current_span); - } else { - bug!("Unknown found in the middle of the stack") - } + trace!("access: popping item {:?}", item); + Stack::check_protector( + &item, + Some((tag, alloc_range, offset, access)), + global, + alloc_history, + )?; + alloc_history.log_invalidation(item.tag, alloc_range, current_span); } } else { + let start_idx = if let Some(idx) = granting_idx { idx + 1 } else { 0 }; // On a read, *disable* all `Unique` above the granting item. This ensures U2 for read accesses. // The reason this is not following the stack discipline (by removing the first Unique and // everything on top of it) is that in `let raw = &mut *x as *mut _; let _val = *x;`, the second statement @@ -465,29 +456,26 @@ 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. - for idx in ((granting_idx + 1)..self.borrows.len()).rev() { + for idx in (start_idx..self.borrows.len()).rev() { let item = &mut self.borrows[idx]; - if let ItemOrUnknown::Item(item) = item { - if item.perm == Permission::Unique { - trace!("access: disabling item {:?}", item); - Stack::check_protector( - item, - Some((tag, alloc_range, offset, access)), - global, - alloc_history, - )?; - item.perm = Permission::Disabled; - alloc_history.log_invalidation(item.tag, alloc_range, current_span); - } - } else { - bug!("Unknown found in the middle of the stack") + if item.perm == Permission::Unique { + trace!("access: disabling item {:?}", item); + Stack::check_protector( + item, + Some((tag, alloc_range, offset, access)), + global, + alloc_history, + )?; + item.perm = Permission::Disabled; + alloc_history.log_invalidation(item.tag, alloc_range, current_span); } } } } else { self.borrows.clear(); - self.borrows.push(ItemOrUnknown::Unknown(global.next_ptr_id)); + // TODO + // self.borrows.push(ItemOrUnknown::Unknown(global.next_ptr_id)); } // Done. @@ -505,7 +493,7 @@ impl<'tcx> Stack { exposed_tags: &FxHashSet, ) -> InterpResult<'tcx> { // Step 1: Find granting item. - self.find_granting(AccessKind::Write, tag, exposed_tags).ok_or_else(|| { + self.find_granting(AccessKind::Write, tag, exposed_tags).map_err(|_| { err_sb_ub(format!( "no item granting write access for deallocation to tag {:?} at {:?} found in borrow stack", tag, alloc_id, @@ -517,11 +505,7 @@ impl<'tcx> Stack { // Step 2: Remove all items. Also checks for protectors. for item in self.borrows.drain(..).rev() { - if let ItemOrUnknown::Item(item) = item { - Stack::check_protector(&item, None, global, alloc_history)?; - } else { - // TODO: I think we just ignore the protectors here? - } + Stack::check_protector(&item, None, global, alloc_history)?; } Ok(()) @@ -558,9 +542,8 @@ impl<'tcx> Stack { // Now we figure out which item grants our parent (`derived_from`) this kind of access. // We use that to determine where to put the new item. - let granting_idx = self - .find_granting(access, derived_from, exposed_tags) - .ok_or_else(|| { + let granting_idx = + self.find_granting(access, derived_from, exposed_tags).map_err(|_| { alloc_history.grant_error( derived_from, new, @@ -598,14 +581,12 @@ impl<'tcx> Stack { }; // Put the new item there. As an optimization, deduplicate if it is equal to one of its new neighbors. - if self.borrows[new_idx - 1] == ItemOrUnknown::Item(new) - || self.borrows.get(new_idx) == Some(&ItemOrUnknown::Item(new)) - { + if self.borrows[new_idx - 1] == new || self.borrows.get(new_idx) == Some(&new) { // Optimization applies, done. trace!("reborrow: avoiding adding redundant item {:?}", new); } else { trace!("reborrow: adding item {:?}", new); - self.borrows.insert(new_idx, ItemOrUnknown::Item(new)); + self.borrows.insert(new_idx, new); } Ok(()) @@ -618,12 +599,12 @@ impl<'tcx> Stacks { /// Creates new stack with initial tag. fn new(size: Size, perm: Permission, tag: SbTag) -> Self { let item = Item { perm, tag, protector: None }; - let stack = Stack { borrows: vec![ItemOrUnknown::Item(item)], unknown_bottom: None }; + let stack = Stack { borrows: vec![item], unknown_bottom: None }; Stacks { stacks: RefCell::new(RangeMap::new(size, stack)), history: RefCell::new(AllocHistory::new()), - exposed_tags: FxHashSet::default(), + exposed_tags: RefCell::new(FxHashSet::default()), } } @@ -631,12 +612,18 @@ impl<'tcx> Stacks { fn for_each( &self, range: AllocRange, - mut f: impl FnMut(Size, &mut Stack, &mut AllocHistory) -> InterpResult<'tcx>, + mut f: impl FnMut( + Size, + &mut Stack, + &mut AllocHistory, + &mut FxHashSet, + ) -> InterpResult<'tcx>, ) -> InterpResult<'tcx> { let mut stacks = self.stacks.borrow_mut(); let history = &mut *self.history.borrow_mut(); + let exposed_tags = &mut *self.exposed_tags.borrow_mut(); for (offset, stack) in stacks.iter_mut(range.start, range.size) { - f(offset, stack, history)?; + f(offset, stack, history, exposed_tags)?; } Ok(()) } @@ -645,12 +632,18 @@ impl<'tcx> Stacks { fn for_each_mut( &mut self, range: AllocRange, - mut f: impl FnMut(Size, &mut Stack, &mut AllocHistory, &mut FxHashSet) -> InterpResult<'tcx>, + mut f: impl FnMut( + Size, + &mut Stack, + &mut AllocHistory, + &mut FxHashSet, + ) -> InterpResult<'tcx>, ) -> InterpResult<'tcx> { let stacks = self.stacks.get_mut(); let history = &mut *self.history.borrow_mut(); + let exposed_tags = self.exposed_tags.get_mut(); for (offset, stack) in stacks.iter_mut(range.start, range.size) { - f(offset, stack, history)?; + f(offset, stack, history, exposed_tags)?; } Ok(()) } @@ -722,7 +715,7 @@ impl Stacks { range.size.bytes() ); - self.exposed_tags.insert(tag); + self.exposed_tags.borrow_mut().insert(tag); Ok(()) } @@ -743,7 +736,7 @@ impl Stacks { range.size.bytes() ); let mut state = state.borrow_mut(); - self.for_each(range, |offset, stack, history| { + self.for_each(range, |offset, stack, history, exposed_tags| { stack.access( AccessKind::Read, tag, @@ -751,7 +744,7 @@ impl Stacks { &mut state, &mut current_span, history, - &self.exposed_tags, + exposed_tags, ) }) } @@ -772,7 +765,7 @@ impl Stacks { range.size.bytes() ); let mut state = state.borrow_mut(); - self.for_each_mut(range, |offset, stack, history| { + self.for_each_mut(range, |offset, stack, history, exposed_tags| { stack.access( AccessKind::Write, tag, @@ -780,7 +773,7 @@ impl Stacks { &mut state, &mut current_span, history, - &self.exposed_tags, + exposed_tags, ) }) } @@ -795,8 +788,8 @@ impl Stacks { ) -> InterpResult<'tcx> { trace!("deallocation with tag {:?}: {:?}, size {}", tag, alloc_id, range.size.bytes()); let state = state.borrow(); - self.for_each_mut(range, |offset, stack, history| { - stack.dealloc(tag, (alloc_id, range, offset), &state, history, &self.exposed_tags) + self.for_each_mut(range, |offset, stack, history, exposed_tags| { + stack.dealloc(tag, (alloc_id, range, offset), &state, history, exposed_tags) })?; Ok(()) } @@ -926,7 +919,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx }; let item = Item { perm, tag: new_tag, protector }; let mut global = this.machine.stacked_borrows.as_ref().unwrap().borrow_mut(); - stacked_borrows.for_each(range, |offset, stack, history| { + stacked_borrows.for_each(range, |offset, stack, history, exposed_tags| { stack.grant( orig_tag, item, @@ -934,7 +927,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx &mut *global, current_span, history, - &stacked_borrows.exposed_tags, + exposed_tags, ) }) })?; @@ -951,7 +944,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let range = alloc_range(base_offset, size); let mut global = machine.stacked_borrows.as_ref().unwrap().borrow_mut(); let current_span = &mut machine.current_span(); // `get_alloc_extra_mut` invalidated our old `current_span` - stacked_borrows.for_each_mut(range, |offset, stack, history| { + stacked_borrows.for_each_mut(range, |offset, stack, history, exposed_tags| { stack.grant( orig_tag, item, @@ -959,7 +952,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx &mut global, current_span, history, - &stacked_borrows.exposed_tags, + exposed_tags, ) })?; diff --git a/src/stacked_borrows/diagnostics.rs b/src/stacked_borrows/diagnostics.rs index e0977c23a6..224954c761 100644 --- a/src/stacked_borrows/diagnostics.rs +++ b/src/stacked_borrows/diagnostics.rs @@ -255,12 +255,7 @@ fn operation_summary( fn error_cause(stack: &Stack, tag: Option) -> &'static str { if let Some(tag) = tag { - if stack - .borrows - .iter() - .filter_map(|x| x.item()) - .any(|item| item.tag == tag && item.perm != Permission::Disabled) - { + if stack.borrows.iter().any(|item| item.tag == tag && item.perm != Permission::Disabled) { ", but that tag only grants SharedReadOnly permission for this location" } else { ", but that tag does not exist in the borrow stack for this location" diff --git a/tests/pass/permissive_provenance_sb.rs b/tests/pass/permissive_provenance_sb.rs deleted file mode 100644 index 97a634badc..0000000000 --- a/tests/pass/permissive_provenance_sb.rs +++ /dev/null @@ -1,69 +0,0 @@ -// compile-flags: -Zmiri-permissive-provenance - -#![feature(strict_provenance)] -use std::ptr; - -/// Ensure that we do not just pick the topmost possible item on int2ptr casts. -fn example(variant: bool) { unsafe { - fn not_so_innocent(x: &mut u32) -> usize { - let x_raw4 = x as *mut u32; - x_raw4.expose_addr() - } - - let mut c = 42u32; - - let x_unique1 = &mut c; - // [..., Unique(1)] - - let x_raw2 = x_unique1 as *mut u32; - let x_raw2_addr = x_raw2.expose_addr(); - // [..., Unique(1), SharedRW(2)] - - let x_unique3 = &mut *x_raw2; - // [.., Unique(1), SharedRW(2), Unique(3)] - - assert_eq!(not_so_innocent(x_unique3), x_raw2_addr); - // [.., Unique(1), SharedRW(2), Unique(3), ..., SharedRW(4)] - - // Do an int2ptr cast. This can pick tag 2 or 4 (the two previously exposed tags). - // 4 is the "obvious" choice (topmost tag, what we used to do with untagged pointers). - // And indeed if `variant == true` it is the only possible choice. - // But if `variant == false` then 2 is the only possible choice! - let x_wildcard = ptr::from_exposed_addr_mut::(x_raw2_addr); - - if variant { - // If we picked 2, this will invalidate 3. - *x_wildcard = 10; - // Now we use 3. Only possible if above we picked 4. - *x_unique3 = 12; - } else { - // This invalidates tag 4. - *x_unique3 = 10; - // Now try to write with the "guessed" tag; it must be 2. - *x_wildcard = 12; - } -} } - -fn test() { unsafe { - let root = &mut 42; - let root_raw = root as *mut i32; - let addr1 = root_raw as usize; - let child = &mut *root_raw; - let child_raw = child as *mut i32; - let addr2 = child_raw as usize; - assert_eq!(addr1, addr2); - // First use child. - *(addr2 as *mut i32) -= 2; // picks child_raw - *child -= 2; - // Then use root. - *(addr1 as *mut i32) += 2; // picks root_raw - *root += 2; - // Value should be unchanged. - assert_eq!(*root, 42); -} } - -fn main() { - example(false); - example(true); - test(); -} diff --git a/tests/pass/stacked-borrows/int-to-ptr.rs b/tests/pass/stacked-borrows/int-to-ptr.rs index efba0da1b9..b1bd61b6d9 100644 --- a/tests/pass/stacked-borrows/int-to-ptr.rs +++ b/tests/pass/stacked-borrows/int-to-ptr.rs @@ -1,6 +1,6 @@ -fn main() { - ref_raw_int_raw(); -} +// compile-flags: -Zmiri-permissive-provenance +#![feature(strict_provenance)] +use std::ptr; // Just to make sure that casting a ref to raw, to int and back to raw // and only then using it works. This rules out ideas like "do escape-to-raw lazily"; @@ -11,3 +11,69 @@ fn ref_raw_int_raw() { let xraw = xref as *mut i32 as usize as *mut i32; assert_eq!(unsafe { *xraw }, 3); } + +/// Ensure that we do not just pick the topmost possible item on int2ptr casts. +fn example(variant: bool) { unsafe { + fn not_so_innocent(x: &mut u32) -> usize { + let x_raw4 = x as *mut u32; + x_raw4.expose_addr() + } + + let mut c = 42u32; + + let x_unique1 = &mut c; + // [..., Unique(1)] + + let x_raw2 = x_unique1 as *mut u32; + let x_raw2_addr = x_raw2.expose_addr(); + // [..., Unique(1), SharedRW(2)] + + let x_unique3 = &mut *x_raw2; + // [.., Unique(1), SharedRW(2), Unique(3)] + + assert_eq!(not_so_innocent(x_unique3), x_raw2_addr); + // [.., Unique(1), SharedRW(2), Unique(3), ..., SharedRW(4)] + + // Do an int2ptr cast. This can pick tag 2 or 4 (the two previously exposed tags). + // 4 is the "obvious" choice (topmost tag, what we used to do with untagged pointers). + // And indeed if `variant == true` it is the only possible choice. + // But if `variant == false` then 2 is the only possible choice! + let x_wildcard = ptr::from_exposed_addr_mut::(x_raw2_addr); + + if variant { + // If we picked 2, this will invalidate 3. + *x_wildcard = 10; + // Now we use 3. Only possible if above we picked 4. + *x_unique3 = 12; + } else { + // This invalidates tag 4. + *x_unique3 = 10; + // Now try to write with the "guessed" tag; it must be 2. + *x_wildcard = 12; + } +} } + +fn test() { unsafe { + let root = &mut 42; + let root_raw = root as *mut i32; + let addr1 = root_raw as usize; + let child = &mut *root_raw; + let child_raw = child as *mut i32; + let addr2 = child_raw as usize; + assert_eq!(addr1, addr2); + // First use child. + *(addr2 as *mut i32) -= 2; // picks child_raw + *child -= 2; + // Then use root. + *(addr1 as *mut i32) += 2; // picks root_raw + *root += 2; + // Value should be unchanged. + assert_eq!(*root, 42); +} } + +fn main() { + example(false); + example(true); + test(); + ref_raw_int_raw(); +}