From 1462f8035126ce9ad514e98d513532404bd3a988 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Feb 2024 21:48:58 +0100 Subject: [PATCH] miri: fix ICE with symbolic alignment check on extern static --- src/borrow_tracker/mod.rs | 2 +- src/machine.rs | 34 +++++++++++-------- src/provenance_gc.rs | 1 + src/shims/foreign_items.rs | 12 +++---- ...88-ice-symbolic-alignment-extern-static.rs | 12 +++++++ ...ce-symbolic-alignment-extern-static.stderr | 14 ++++++++ tests/pass/align_offset_symbolic.rs | 23 ++++++++++++- 7 files changed, 75 insertions(+), 23 deletions(-) create mode 100644 tests/fail/issue-miri-3288-ice-symbolic-alignment-extern-static.rs create mode 100644 tests/fail/issue-miri-3288-ice-symbolic-alignment-extern-static.stderr diff --git a/src/borrow_tracker/mod.rs b/src/borrow_tracker/mod.rs index 74ff6ed4e0..46f96a715f 100644 --- a/src/borrow_tracker/mod.rs +++ b/src/borrow_tracker/mod.rs @@ -383,7 +383,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // will never cause UB on the pointer itself. let (_, _, kind) = this.get_alloc_info(*alloc_id); if matches!(kind, AllocKind::LiveData) { - let alloc_extra = this.get_alloc_extra(*alloc_id).unwrap(); + let alloc_extra = this.get_alloc_extra(*alloc_id)?; // can still fail for `extern static` let alloc_borrow_tracker = &alloc_extra.borrow_tracker.as_ref().unwrap(); alloc_borrow_tracker.release_protector(&this.machine, borrow_tracker, *tag)?; } diff --git a/src/machine.rs b/src/machine.rs index 946887637f..40d041c8fd 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -2,7 +2,7 @@ //! `Machine` trait. use std::borrow::Cow; -use std::cell::{Cell, RefCell}; +use std::cell::RefCell; use std::collections::hash_map::Entry; use std::fmt; use std::path::Path; @@ -336,20 +336,11 @@ pub struct AllocExtra<'tcx> { /// if this allocation is leakable. The backtrace is not /// pruned yet; that should be done before printing it. pub backtrace: Option>>, - /// An offset inside this allocation that was deemed aligned even for symbolic alignment checks. - /// Invariant: the promised alignment will never be less than the native alignment of this allocation. - pub symbolic_alignment: Cell>, } impl VisitProvenance for AllocExtra<'_> { fn visit_provenance(&self, visit: &mut VisitWith<'_>) { - let AllocExtra { - borrow_tracker, - data_race, - weak_memory, - backtrace: _, - symbolic_alignment: _, - } = self; + let AllocExtra { borrow_tracker, data_race, weak_memory, backtrace: _ } = self; borrow_tracker.visit_provenance(visit); data_race.visit_provenance(visit); @@ -572,6 +563,14 @@ pub struct MiriMachine<'mir, 'tcx> { /// that is fixed per stack frame; this lets us have sometimes different results for the /// same const while ensuring consistent results within a single call. const_cache: RefCell, usize), OpTy<'tcx, Provenance>>>, + + /// For each allocation, an offset inside that allocation that was deemed aligned even for + /// symbolic alignment checks. This cannot be stored in `AllocExtra` since it needs to be + /// tracked for vtables and function allocations as well as regular allocations. + /// + /// Invariant: the promised alignment will never be less than the native alignment of the + /// allocation. + pub(crate) symbolic_alignment: RefCell>, } impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { @@ -698,6 +697,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { collect_leak_backtraces: config.collect_leak_backtraces, allocation_spans: RefCell::new(FxHashMap::default()), const_cache: RefCell::new(FxHashMap::default()), + symbolic_alignment: RefCell::new(FxHashMap::default()), } } @@ -810,6 +810,7 @@ impl VisitProvenance for MiriMachine<'_, '_> { collect_leak_backtraces: _, allocation_spans: _, const_cache: _, + symbolic_alignment: _, } = self; threads.visit_provenance(visit); @@ -893,9 +894,13 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { return None; } // Let's see which alignment we have been promised for this allocation. - let alloc_info = ecx.get_alloc_extra(alloc_id).unwrap(); // cannot fail since the allocation is live - let (promised_offset, promised_align) = - alloc_info.symbolic_alignment.get().unwrap_or((Size::ZERO, alloc_align)); + let (promised_offset, promised_align) = ecx + .machine + .symbolic_alignment + .borrow() + .get(&alloc_id) + .copied() + .unwrap_or((Size::ZERO, alloc_align)); if promised_align < align { // Definitely not enough. Some(Misalignment { has: promised_align, required: align }) @@ -1132,7 +1137,6 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { data_race: race_alloc, weak_memory: buffer_alloc, backtrace, - symbolic_alignment: Cell::new(None), }, |ptr| ecx.global_base_pointer(ptr), )?; diff --git a/src/provenance_gc.rs b/src/provenance_gc.rs index ab178f82d9..347951ce37 100644 --- a/src/provenance_gc.rs +++ b/src/provenance_gc.rs @@ -196,6 +196,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_mut(); let allocs = LiveAllocs { ecx: this, collected: allocs }; this.machine.allocation_spans.borrow_mut().retain(|id, _| allocs.is_live(*id)); + this.machine.symbolic_alignment.borrow_mut().retain(|id, _| allocs.is_live(*id)); this.machine.intptrcast.borrow_mut().remove_unreachable_allocs(&allocs); if let Some(borrow_tracker) = &this.machine.borrow_tracker { borrow_tracker.borrow_mut().remove_unreachable_allocs(&allocs); diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index a002f2aad0..25654248db 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -585,17 +585,17 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } if let Ok((alloc_id, offset, ..)) = this.ptr_try_get_alloc_id(ptr) { let (_size, alloc_align, _kind) = this.get_alloc_info(alloc_id); - // Not `get_alloc_extra_mut`, need to handle read-only allocations! - let alloc_extra = this.get_alloc_extra(alloc_id)?; // If the newly promised alignment is bigger than the native alignment of this // allocation, and bigger than the previously promised alignment, then set it. if align > alloc_align - && !alloc_extra + && !this + .machine .symbolic_alignment - .get() - .is_some_and(|(_, old_align)| align <= old_align) + .get_mut() + .get(&alloc_id) + .is_some_and(|&(_, old_align)| align <= old_align) { - alloc_extra.symbolic_alignment.set(Some((offset, align))); + this.machine.symbolic_alignment.get_mut().insert(alloc_id, (offset, align)); } } } diff --git a/tests/fail/issue-miri-3288-ice-symbolic-alignment-extern-static.rs b/tests/fail/issue-miri-3288-ice-symbolic-alignment-extern-static.rs new file mode 100644 index 0000000000..4460407498 --- /dev/null +++ b/tests/fail/issue-miri-3288-ice-symbolic-alignment-extern-static.rs @@ -0,0 +1,12 @@ +//@compile-flags: -Zmiri-symbolic-alignment-check + +extern "C" { + static _dispatch_queue_attr_concurrent: [u8; 0]; +} + +static DISPATCH_QUEUE_CONCURRENT: &'static [u8; 0] = + unsafe { &_dispatch_queue_attr_concurrent }; + +fn main() { + let _val = *DISPATCH_QUEUE_CONCURRENT; //~ERROR: is not supported +} diff --git a/tests/fail/issue-miri-3288-ice-symbolic-alignment-extern-static.stderr b/tests/fail/issue-miri-3288-ice-symbolic-alignment-extern-static.stderr new file mode 100644 index 0000000000..a4249d2e88 --- /dev/null +++ b/tests/fail/issue-miri-3288-ice-symbolic-alignment-extern-static.stderr @@ -0,0 +1,14 @@ +error: unsupported operation: `extern` static `_dispatch_queue_attr_concurrent` from crate `issue_miri_3288_ice_symbolic_alignment_extern_static` is not supported by Miri + --> $DIR/issue-miri-3288-ice-symbolic-alignment-extern-static.rs:LL:CC + | +LL | let _val = *DISPATCH_QUEUE_CONCURRENT; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ `extern` static `_dispatch_queue_attr_concurrent` from crate `issue_miri_3288_ice_symbolic_alignment_extern_static` is not supported by Miri + | + = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support + = note: BACKTRACE: + = note: inside `main` at $DIR/issue-miri-3288-ice-symbolic-alignment-extern-static.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + diff --git a/tests/pass/align_offset_symbolic.rs b/tests/pass/align_offset_symbolic.rs index 4df364bac7..e96f11b1ef 100644 --- a/tests/pass/align_offset_symbolic.rs +++ b/tests/pass/align_offset_symbolic.rs @@ -1,6 +1,8 @@ //@compile-flags: -Zmiri-symbolic-alignment-check #![feature(strict_provenance)] +use std::mem; + fn test_align_to() { const N: usize = 4; let d = Box::new([0u32; N]); @@ -68,7 +70,7 @@ fn test_u64_array() { #[repr(align(8))] struct AlignToU64(T); - const BYTE_LEN: usize = std::mem::size_of::<[u64; 4]>(); + const BYTE_LEN: usize = mem::size_of::<[u64; 4]>(); type Data = AlignToU64<[u8; BYTE_LEN]>; fn example(data: &Data) { @@ -101,10 +103,29 @@ fn huge_align() { let _ = std::ptr::invalid::(SIZE).align_offset(SIZE); } +// This shows that we cannot store the promised alignment info in `AllocExtra`, +// since vtables do not have an `AllocExtra`. +fn vtable() { + #[cfg(target_pointer_width = "64")] + type TWOPTR = u128; + #[cfg(target_pointer_width = "32")] + type TWOPTR = u64; + + let ptr: &dyn Send = &0; + let parts: (*const (), *const u8) = unsafe { mem::transmute(ptr) }; + let vtable = parts.1 ; + let offset = vtable.align_offset(mem::align_of::()); + let _vtable_aligned = vtable.wrapping_add(offset) as *const [TWOPTR; 0]; + // FIXME: we can't actually do the access since vtable pointers act like zero-sized allocations. + // Enable the next line once https://github.com/rust-lang/rust/issues/117945 is implemented. + //let _place = unsafe { &*vtable_aligned }; +} + fn main() { test_align_to(); test_from_utf8(); test_u64_array(); test_cstr(); huge_align(); + vtable(); }