From 194451345dc6b7d269a5ded6fde49883cb862d75 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 28 Oct 2020 12:23:35 +0100 Subject: [PATCH] add an option to track raw pointer tags in Stacked Borrows --- README.md | 10 +++++++--- src/bin/miri.rs | 3 +++ src/eval.rs | 14 ++++---------- src/machine.rs | 23 +++++++++++------------ src/stacked_borrows.rs | 32 ++++++++++++++++++-------------- 5 files changed, 43 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index b638c3bca1ce..0aa22a81ad5c 100644 --- a/README.md +++ b/README.md @@ -232,13 +232,17 @@ environment variable: * `-Zmiri-track-alloc-id=` shows a backtrace when the given allocation is being allocated or freed. This helps in debugging memory leaks and use after free bugs. +* `-Zmiri-track-call-id=` shows a backtrace when the given call id is + assigned to a stack frame. This helps in debugging UB related to Stacked + Borrows "protectors". * `-Zmiri-track-pointer-tag=` shows a backtrace when the given pointer tag is popped from a borrow stack (which is where the tag becomes invalid and any future use of it will error). This helps you in finding out why UB is happening and where in your code would be a good place to look for it. -* `-Zmiri-track-call-id=` shows a backtrace when the given call id is - assigned to a stack frame. This helps in debugging UB related to Stacked - Borrows "protectors". +* `-Zmiri-track-raw-pointers` makes Stacked Borrows track a pointer tag even for + raw pointers. This can make valid code fail to pass the checks (when + integer-pointer casts are involved), but also can help identify latent + aliasing issues in code that Miri accepts by default. Some native rustc `-Z` flags are also very relevant for Miri: diff --git a/src/bin/miri.rs b/src/bin/miri.rs index 5769590ad094..ef1429a35020 100644 --- a/src/bin/miri.rs +++ b/src/bin/miri.rs @@ -207,6 +207,9 @@ fn main() { "-Zmiri-ignore-leaks" => { miri_config.ignore_leaks = true; } + "-Zmiri-track-raw-pointers" => { + miri_config.track_raw = true; + } "--" => { after_dashdash = true; } diff --git a/src/eval.rs b/src/eval.rs index e36a0019cdcb..54d06feec36d 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -3,8 +3,6 @@ use std::convert::TryFrom; use std::ffi::OsStr; -use rand::rngs::StdRng; -use rand::SeedableRng; use log::info; use rustc_hir::def_id::DefId; @@ -48,6 +46,8 @@ pub struct MiriConfig { pub tracked_call_id: Option, /// The allocation id to report about. pub tracked_alloc_id: Option, + /// Whether to track raw pointers in stacked borrows. + pub track_raw: bool, } impl Default for MiriConfig { @@ -64,6 +64,7 @@ impl Default for MiriConfig { tracked_pointer_tag: None, tracked_call_id: None, tracked_alloc_id: None, + track_raw: false, } } } @@ -84,14 +85,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>( rustc_span::source_map::DUMMY_SP, param_env, Evaluator::new(config.communicate, config.validate, layout_cx), - MemoryExtra::new( - StdRng::seed_from_u64(config.seed.unwrap_or(0)), - config.stacked_borrows, - config.tracked_pointer_tag, - config.tracked_call_id, - config.tracked_alloc_id, - config.check_alignment, - ), + MemoryExtra::new(&config), ); // Complete initialization. EnvVars::init(&mut ecx, config.excluded_env_vars)?; diff --git a/src/machine.rs b/src/machine.rs index 544f4667e846..e9f9298e566c 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -10,6 +10,7 @@ use std::fmt; use log::trace; use rand::rngs::StdRng; +use rand::SeedableRng; use rustc_data_structures::fx::FxHashMap; use rustc_middle::{ @@ -132,16 +133,14 @@ pub struct MemoryExtra { } impl MemoryExtra { - pub fn new( - rng: StdRng, - stacked_borrows: bool, - tracked_pointer_tag: Option, - tracked_call_id: Option, - tracked_alloc_id: Option, - check_alignment: AlignmentCheck, - ) -> Self { - let stacked_borrows = if stacked_borrows { - Some(Rc::new(RefCell::new(stacked_borrows::GlobalState::new(tracked_pointer_tag, tracked_call_id)))) + pub fn new(config: &MiriConfig) -> Self { + let rng = StdRng::seed_from_u64(config.seed.unwrap_or(0)); + let stacked_borrows = if config.stacked_borrows { + Some(Rc::new(RefCell::new(stacked_borrows::GlobalState::new( + config.tracked_pointer_tag, + config.tracked_call_id, + config.track_raw, + )))) } else { None }; @@ -150,8 +149,8 @@ impl MemoryExtra { intptrcast: Default::default(), extern_statics: FxHashMap::default(), rng: RefCell::new(rng), - tracked_alloc_id, - check_alignment, + tracked_alloc_id: config.tracked_alloc_id, + check_alignment: config.check_alignment, } } diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index cf5b31597daa..616950eb0a0a 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -108,6 +108,8 @@ pub struct GlobalState { tracked_pointer_tag: Option, /// The call id to trace tracked_call_id: Option, + /// Whether to track raw pointers. + track_raw: bool, } /// Memory extra state gives us interior mutable access to the global state. pub type MemoryExtra = Rc>; @@ -155,7 +157,7 @@ impl fmt::Display for RefKind { /// Utilities for initialization and ID generation impl GlobalState { - pub fn new(tracked_pointer_tag: Option, tracked_call_id: Option) -> Self { + pub fn new(tracked_pointer_tag: Option, tracked_call_id: Option, track_raw: bool) -> Self { GlobalState { next_ptr_id: NonZeroU64::new(1).unwrap(), base_ptr_ids: FxHashMap::default(), @@ -163,6 +165,7 @@ impl GlobalState { active_calls: FxHashSet::default(), tracked_pointer_tag, tracked_call_id, + track_raw, } } @@ -479,9 +482,12 @@ impl Stacks { // The base pointer is not unique, so the base permission is `SharedReadWrite`. MemoryKind::Machine(MiriMemoryKind::Global | MiriMemoryKind::ExternStatic | MiriMemoryKind::Tls | MiriMemoryKind::Env) => (extra.borrow_mut().global_base_ptr(id), Permission::SharedReadWrite), - // Everything else we handle entirely untagged for now. - // FIXME: experiment with more precise tracking. - _ => (Tag::Untagged, Permission::SharedReadWrite), + // Everything else we handle like raw pointers for now. + _ => { + let mut extra = extra.borrow_mut(); + let tag = if extra.track_raw { Tag::Tagged(extra.new_ptr()) } else { Tag::Untagged }; + (tag, Permission::SharedReadWrite) + } }; (Stacks::new(size, perm, tag, extra), tag) } @@ -593,16 +599,14 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } // Compute new borrow. - let new_tag = match kind { - // Give up tracking for raw pointers. - // FIXME: Experiment with more precise tracking. Blocked on `&raw` - // because `Rc::into_raw` currently creates intermediate references, - // breaking `Rc::from_raw`. - RefKind::Raw { .. } => Tag::Untagged, - // All other pointesr are properly tracked. - _ => Tag::Tagged( - this.memory.extra.stacked_borrows.as_ref().unwrap().borrow_mut().new_ptr(), - ), + let new_tag = { + let mut mem_extra = this.memory.extra.stacked_borrows.as_ref().unwrap().borrow_mut(); + match kind { + // Give up tracking for raw pointers. + RefKind::Raw { .. } if !mem_extra.track_raw => Tag::Untagged, + // All other pointers are properly tracked. + _ => Tag::Tagged(mem_extra.new_ptr()), + } }; // Reborrow.