From 0303c0b54212813110ba3b4814c418e108c7d70e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 2 Aug 2024 10:29:52 +0200 Subject: [PATCH] Miri: add a flag to do recursive validity checking --- README.md | 2 ++ src/bin/miri.rs | 6 ++-- src/eval.rs | 16 +++++++-- src/intrinsics/mod.rs | 2 +- src/lib.rs | 1 + src/machine.rs | 36 +++++++------------ .../validity/recursive-validity-ref-bool.rs | 8 +++++ .../recursive-validity-ref-bool.stderr | 15 ++++++++ 8 files changed, 57 insertions(+), 29 deletions(-) create mode 100644 tests/fail/validity/recursive-validity-ref-bool.rs create mode 100644 tests/fail/validity/recursive-validity-ref-bool.stderr diff --git a/README.md b/README.md index b1be596c00..ff0f162822 100644 --- a/README.md +++ b/README.md @@ -398,6 +398,8 @@ 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-recursive-validation` is a *highly experimental* flag that makes validity checking + recurse below references. * `-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 diff --git a/src/bin/miri.rs b/src/bin/miri.rs index 25b154a820..4b3c97e248 100644 --- a/src/bin/miri.rs +++ b/src/bin/miri.rs @@ -44,7 +44,7 @@ use rustc_session::config::{CrateType, ErrorOutputType, OptLevel}; use rustc_session::search_paths::PathKind; use rustc_session::{CtfeBacktrace, EarlyDiagCtxt}; -use miri::{BacktraceStyle, BorrowTrackerMethod, ProvenanceMode, RetagFields}; +use miri::{BacktraceStyle, BorrowTrackerMethod, ProvenanceMode, RetagFields, ValidationMode}; struct MiriCompilerCalls { miri_config: miri::MiriConfig, @@ -421,7 +421,9 @@ fn main() { } else if arg == "--" { after_dashdash = true; } else if arg == "-Zmiri-disable-validation" { - miri_config.validate = false; + miri_config.validation = ValidationMode::No; + } else if arg == "-Zmiri-recursive-validation" { + miri_config.validation = ValidationMode::Deep; } else if arg == "-Zmiri-disable-stacked-borrows" { miri_config.borrow_tracker = None; } else if arg == "-Zmiri-tree-borrows" { diff --git a/src/eval.rs b/src/eval.rs index 2184a4426c..53e8775170 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -68,7 +68,7 @@ pub enum IsolatedOp { Allow, } -#[derive(Copy, Clone, PartialEq, Eq)] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum BacktraceStyle { /// Prints a terser backtrace which ideally only contains relevant information. Short, @@ -78,6 +78,16 @@ pub enum BacktraceStyle { Off, } +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum ValidationMode { + /// Do not perform any kind of validation. + No, + /// Validate the interior of the value, but not things behind references. + Shallow, + /// Fully recursively validate references. + Deep, +} + /// Configuration needed to spawn a Miri instance. #[derive(Clone)] pub struct MiriConfig { @@ -85,7 +95,7 @@ pub struct MiriConfig { /// (This is still subject to isolation as well as `forwarded_env_vars`.) pub env: Vec<(OsString, OsString)>, /// Determine if validity checking is enabled. - pub validate: bool, + pub validation: ValidationMode, /// Determines if Stacked Borrows or Tree Borrows is enabled. pub borrow_tracker: Option, /// Whether `core::ptr::Unique` receives special treatment. @@ -162,7 +172,7 @@ impl Default for MiriConfig { fn default() -> MiriConfig { MiriConfig { env: vec![], - validate: true, + validation: ValidationMode::Shallow, borrow_tracker: Some(BorrowTrackerMethod::StackedBorrows), unique_is_unique: false, check_alignment: AlignmentCheck::Int, diff --git a/src/intrinsics/mod.rs b/src/intrinsics/mod.rs index 9cd776c937..d60119e75f 100644 --- a/src/intrinsics/mod.rs +++ b/src/intrinsics/mod.rs @@ -153,7 +153,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Would not be considered UB, or the other way around (`is_val_statically_known(0)`). "is_val_statically_known" => { let [arg] = check_arg_count(args)?; - this.validate_operand(arg)?; + this.validate_operand(arg, /*recursive*/ false)?; let branch: bool = this.machine.rng.get_mut().gen(); this.write_scalar(Scalar::from_bool(branch), dest)?; } diff --git a/src/lib.rs b/src/lib.rs index 24909f21eb..2b3ae6df5d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -142,6 +142,7 @@ pub use crate::diagnostics::{ }; pub use crate::eval::{ create_ecx, eval_entry, AlignmentCheck, BacktraceStyle, IsolatedOp, MiriConfig, RejectOpWith, + ValidationMode, }; pub use crate::helpers::{AccessKind, EvalContextExt as _}; pub use crate::machine::{ diff --git a/src/machine.rs b/src/machine.rs index 5f4aa9d2f5..fec345c8de 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -187,7 +187,11 @@ impl fmt::Display for MiriMemoryKind { pub type MemoryKind = interpret::MemoryKind; /// Pointer provenance. -#[derive(Clone, Copy)] +// This needs to be `Eq`+`Hash` because the `Machine` trait needs that because validity checking +// *might* be recursive and then it has to track which places have already been visited. +// These implementations are a bit questionable, and it means we may check the same place multiple +// times with different provenance, but that is in general not wrong. +#[derive(Clone, Copy, PartialEq, Eq, Hash)] pub enum Provenance { /// For pointers with concrete provenance. we exactly know which allocation they are attached to /// and what their borrow tag is. @@ -215,24 +219,6 @@ pub enum Provenance { Wildcard, } -// This needs to be `Eq`+`Hash` because the `Machine` trait needs that because validity checking -// *might* be recursive and then it has to track which places have already been visited. -// However, comparing provenance is meaningless, since `Wildcard` might be any provenance -- and of -// course we don't actually do recursive checking. -// We could change `RefTracking` to strip provenance for its `seen` set but that type is generic so that is quite annoying. -// Instead owe add the required instances but make them panic. -impl PartialEq for Provenance { - fn eq(&self, _other: &Self) -> bool { - panic!("Provenance must not be compared") - } -} -impl Eq for Provenance {} -impl std::hash::Hash for Provenance { - fn hash(&self, _state: &mut H) { - panic!("Provenance must not be hashed") - } -} - /// The "extra" information a pointer has over a regular AllocId. #[derive(Copy, Clone, PartialEq)] pub enum ProvenanceExtra { @@ -460,7 +446,7 @@ pub struct MiriMachine<'tcx> { pub(crate) isolated_op: IsolatedOp, /// Whether to enforce the validity invariant. - pub(crate) validate: bool, + pub(crate) validation: ValidationMode, /// The table of file descriptors. pub(crate) fds: shims::FdTable, @@ -659,7 +645,7 @@ impl<'tcx> MiriMachine<'tcx> { cmd_line: None, tls: TlsData::default(), isolated_op: config.isolated_op, - validate: config.validate, + validation: config.validation, fds: shims::FdTable::init(config.mute_stdout_stderr), dirs: Default::default(), layouts, @@ -801,7 +787,7 @@ impl VisitProvenance for MiriMachine<'_> { fds, tcx: _, isolated_op: _, - validate: _, + validation: _, clock: _, layouts: _, static_roots: _, @@ -943,7 +929,11 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { #[inline(always)] fn enforce_validity(ecx: &MiriInterpCx<'tcx>, _layout: TyAndLayout<'tcx>) -> bool { - ecx.machine.validate + ecx.machine.validation != ValidationMode::No + } + #[inline(always)] + fn enforce_validity_recursively(ecx: &InterpCx<'tcx, Self>, _layout: TyAndLayout<'tcx>) -> bool { + ecx.machine.validation == ValidationMode::Deep } #[inline(always)] diff --git a/tests/fail/validity/recursive-validity-ref-bool.rs b/tests/fail/validity/recursive-validity-ref-bool.rs new file mode 100644 index 0000000000..17b81f29cf --- /dev/null +++ b/tests/fail/validity/recursive-validity-ref-bool.rs @@ -0,0 +1,8 @@ +//@compile-flags: -Zmiri-recursive-validation + +fn main() { + let x = 3u8; + let xref = &x; + let xref_wrong_type: &bool = unsafe { std::mem::transmute(xref) }; //~ERROR: encountered 0x03, but expected a boolean + let _val = *xref_wrong_type; +} diff --git a/tests/fail/validity/recursive-validity-ref-bool.stderr b/tests/fail/validity/recursive-validity-ref-bool.stderr new file mode 100644 index 0000000000..2b2fa9b8a2 --- /dev/null +++ b/tests/fail/validity/recursive-validity-ref-bool.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: constructing invalid value at .: encountered 0x03, but expected a boolean + --> $DIR/recursive-validity-ref-bool.rs:LL:CC + | +LL | let xref_wrong_type: &bool = unsafe { std::mem::transmute(xref) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value at .: encountered 0x03, but expected a boolean + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at $DIR/recursive-validity-ref-bool.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 +