From 892f706ce55d38bf8a6c80fec5d08e785d2240ac Mon Sep 17 00:00:00 2001 From: Smit Soni Date: Thu, 13 May 2021 23:40:07 -0700 Subject: [PATCH] Add a support to execute isolated op without halting In user interface, added a new flag `-Zmiri-isolation-error` which takes one of the four values -- hide, warn, warn-nobacktrace, and abort. This option can be used to configure Miri to either abort or return an error code upon executing isolated op. If not aborted, Miri prints a warning, whose verbosity can be configured using this flag. In implementation, added a new enum `IsolatedOp` to capture all the settings related to ops requiring communication with the host. Old `communicate` flag in both miri configs and machine stats is replaced with a new helper function `communicate()` which checks `isolated_op` internally. Added a new helper function `reject_in_isolation` which can be called by shims to reject ops according to the reject_with settings. Use miri specific diagnostics function `report_msg` to print backtrace in the warning. Update it to take an enum value instead of a bool, indicating the level of diagnostics. Updated shims related to current dir to use the new APIs. Added a new test for current dir ops in isolation without halting machine. --- README.md | 7 ++++ src/bin/miri.rs | 35 +++++++++++++++- src/diagnostics.rs | 41 ++++++++++++------- src/eval.rs | 37 +++++++++++++++-- src/helpers.rs | 28 +++++++++++-- src/lib.rs | 4 +- src/machine.rs | 13 ++++-- src/shims/env.rs | 34 ++++++++++++--- src/shims/posix/fs.rs | 19 +++++---- tests/run-pass/current_dir_with_isolation.rs | 20 +++++++++ .../current_dir_with_isolation.stderr | 4 ++ 11 files changed, 199 insertions(+), 43 deletions(-) create mode 100644 tests/run-pass/current_dir_with_isolation.rs create mode 100644 tests/run-pass/current_dir_with_isolation.stderr diff --git a/README.md b/README.md index 21ab49bc1f8..f6b46ce3a95 100644 --- a/README.md +++ b/README.md @@ -219,6 +219,13 @@ environment variable: * `-Zmiri-disable-isolation` disables host isolation. As a consequence, the program has access to host resources such as environment variables, file systems, and randomness. +* `-Zmiri-isolation-error=` configures Miri's response to operations + requiring host access while isolation is enabled. `abort`, `hide`, `warn`, + and `warn-nobacktrace` are the supported actions. Default action is `abort` + which halts the machine. Rest of the actions configure it to return an error + code for the op and continue executing. `warn` prints backtrace that could + be used to trace the call. `warn-nobacktrace` is less verbose without + backtrace. `hide` hides the warning. * `-Zmiri-env-exclude=` keeps the `var` environment variable isolated from the host so that it cannot be accessed by the program. Can be used multiple times to exclude several variables. On Windows, the `TERM` environment diff --git a/src/bin/miri.rs b/src/bin/miri.rs index 514e5b0aebe..677836d7e97 100644 --- a/src/bin/miri.rs +++ b/src/bin/miri.rs @@ -263,6 +263,9 @@ fn main() { let mut miri_config = miri::MiriConfig::default(); let mut rustc_args = vec![]; let mut after_dashdash = false; + + // If user has explicitly enabled/disabled isolation + let mut isolation_enabled: Option = None; for arg in env::args() { if rustc_args.is_empty() { // Very first arg: binary name. @@ -291,7 +294,37 @@ fn main() { miri_config.check_abi = false; } "-Zmiri-disable-isolation" => { - miri_config.communicate = true; + if matches!(isolation_enabled, Some(true)) { + panic!( + "-Zmiri-disable-isolation cannot be used along with -Zmiri-isolation-error" + ); + } else { + isolation_enabled = Some(false); + } + miri_config.isolated_op = miri::IsolatedOp::Allow; + } + arg if arg.starts_with("-Zmiri-isolation-error=") => { + if matches!(isolation_enabled, Some(false)) { + panic!( + "-Zmiri-isolation-error cannot be used along with -Zmiri-disable-isolation" + ); + } else { + isolation_enabled = Some(true); + } + + miri_config.isolated_op = match arg + .strip_prefix("-Zmiri-isolation-error=") + .unwrap() + { + "abort" => miri::IsolatedOp::Reject(miri::RejectOpWith::Abort), + "hide" => miri::IsolatedOp::Reject(miri::RejectOpWith::NoWarning), + "warn" => miri::IsolatedOp::Reject(miri::RejectOpWith::Warning), + "warn-nobacktrace" => + miri::IsolatedOp::Reject(miri::RejectOpWith::WarningWithoutBacktrace), + _ => panic!( + "-Zmiri-isolation-error must be `abort`, `hide`, `warn`, or `warn-nobacktrace`" + ), + }; } "-Zmiri-ignore-leaks" => { miri_config.ignore_leaks = true; diff --git a/src/diagnostics.rs b/src/diagnostics.rs index f273cf04203..2b17e83bee6 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -52,6 +52,14 @@ pub enum NonHaltingDiagnostic { CreatedCallId(CallId), CreatedAlloc(AllocId), FreedAlloc(AllocId), + RejectedIsolatedOp(String), +} + +/// Level of Miri specific diagnostics +enum DiagLevel { + Error, + Warning, + Note, } /// Emit a custom diagnostic without going through the miri-engine machinery @@ -76,7 +84,7 @@ pub fn report_error<'tcx, 'mir>( #[rustfmt::skip] let helps = match info { UnsupportedInIsolation(_) => - vec![(None, format!("pass the flag `-Zmiri-disable-isolation` to disable isolation"))], + vec![(None, format!("pass the flag `-Zmiri-disable-isolation` to disable isolation; or pass `-Zmiri-isolation-error=warn to configure Miri to return an error code from isolated operations and continue with a warning"))], ExperimentalUb { url, .. } => vec![ (None, format!("this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental")), @@ -137,7 +145,7 @@ pub fn report_error<'tcx, 'mir>( let msg = e.to_string(); report_msg( *ecx.tcx, - /*error*/ true, + DiagLevel::Error, &if let Some(title) = title { format!("{}: {}", title, msg) } else { msg.clone() }, msg, helps, @@ -174,18 +182,19 @@ pub fn report_error<'tcx, 'mir>( /// Also emits a full stacktrace of the interpreter stack. fn report_msg<'tcx>( tcx: TyCtxt<'tcx>, - error: bool, + diag_level: DiagLevel, title: &str, span_msg: String, mut helps: Vec<(Option, String)>, stacktrace: &[FrameInfo<'tcx>], ) { let span = stacktrace.first().map_or(DUMMY_SP, |fi| fi.span); - let mut err = if error { - tcx.sess.struct_span_err(span, title) - } else { - tcx.sess.diagnostic().span_note_diag(span, title) + let mut err = match diag_level { + DiagLevel::Error => tcx.sess.struct_span_err(span, title), + DiagLevel::Warning => tcx.sess.struct_span_warn(span, title), + DiagLevel::Note => tcx.sess.diagnostic().span_note_diag(span, title), }; + // Show main message. if span != DUMMY_SP { err.span_label(span, span_msg); @@ -303,15 +312,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx CreatedCallId(id) => format!("function call with id {}", id), CreatedAlloc(AllocId(id)) => format!("created allocation with id {}", id), FreedAlloc(AllocId(id)) => format!("freed allocation with id {}", id), + RejectedIsolatedOp(ref op) => + format!("`{}` was made to return an error due to isolation", op), }; - report_msg( - *this.tcx, - /*error*/ false, - "tracking was triggered", - msg, - vec![], - &stacktrace, - ); + + let (title, diag_level) = match e { + RejectedIsolatedOp(_) => + ("operation rejected by isolation", DiagLevel::Warning), + _ => ("tracking was triggered", DiagLevel::Note), + }; + + report_msg(*this.tcx, diag_level, title, msg, vec![], &stacktrace); } }); } diff --git a/src/eval.rs b/src/eval.rs index 6646783d349..f1fbeafd3cd 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -22,6 +22,35 @@ pub enum AlignmentCheck { Int, } +#[derive(Copy, Clone, Debug, PartialEq)] +pub enum RejectOpWith { + /// Isolated op is rejected with an abort of the machine. + Abort, + + /// If not Abort, miri returns an error for an isolated op. + /// Following options determine if user should be warned about such error. + /// Do not print warning about rejected isolated op. + NoWarning, + + /// Print a warning about rejected isolated op, with backtrace. + Warning, + + /// Print a warning about rejected isolated op, without backtrace. + WarningWithoutBacktrace, +} + +#[derive(Copy, Clone, Debug, PartialEq)] +pub enum IsolatedOp { + /// Reject an op requiring communication with the host. By + /// default, miri rejects the op with an abort. If not, it returns + /// an error code, and prints a warning about it. Warning levels + /// are controlled by `RejectOpWith` enum. + Reject(RejectOpWith), + + /// Execute op requiring communication with the host, i.e. disable isolation. + Allow, +} + /// Configuration needed to spawn a Miri instance. #[derive(Clone)] pub struct MiriConfig { @@ -33,8 +62,8 @@ pub struct MiriConfig { pub check_alignment: AlignmentCheck, /// Controls function [ABI](Abi) checking. pub check_abi: bool, - /// Determines if communication with the host environment is enabled. - pub communicate: bool, + /// Action for an op requiring communication with the host. + pub isolated_op: IsolatedOp, /// Determines if memory leaks should be ignored. pub ignore_leaks: bool, /// Environment variables that should always be isolated from the host. @@ -68,7 +97,7 @@ impl Default for MiriConfig { stacked_borrows: true, check_alignment: AlignmentCheck::Int, check_abi: true, - communicate: false, + isolated_op: IsolatedOp::Reject(RejectOpWith::Abort), ignore_leaks: false, excluded_env_vars: vec![], args: vec![], @@ -233,7 +262,7 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) -> } SchedulingAction::ExecuteTimeoutCallback => { assert!( - ecx.machine.communicate, + ecx.machine.communicate(), "scheduler callbacks require disabled isolation, but the code \ that created the callback did not check it" ); diff --git a/src/helpers.rs b/src/helpers.rs index 52176342057..e9bedd1a118 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -140,7 +140,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let mut data = vec![0; usize::try_from(len).unwrap()]; - if this.machine.communicate { + if this.machine.communicate() { // Fill the buffer using the host's rng. getrandom::getrandom(&mut data) .map_err(|err| err_unsup_format!("host getrandom failed: {}", err))?; @@ -391,12 +391,32 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx /// disabled. It returns an error using the `name` of the foreign function if this is not the /// case. fn check_no_isolation(&self, name: &str) -> InterpResult<'tcx> { - if !self.eval_context_ref().machine.communicate { - isolation_error(name)?; + if !self.eval_context_ref().machine.communicate() { + self.reject_in_isolation(name, RejectOpWith::Abort)?; } Ok(()) } + /// Helper function used inside the shims of foreign functions which reject the op + /// when isolation is enabled. It is used to print a warning/backtrace about the rejection. + fn reject_in_isolation(&self, op_name: &str, reject_with: RejectOpWith) -> InterpResult<'tcx> { + let this = self.eval_context_ref(); + match reject_with { + RejectOpWith::Abort => isolation_abort_error(op_name), + RejectOpWith::WarningWithoutBacktrace => { + this.tcx + .sess + .warn(&format!("`{}` was made to return an error due to isolation", op_name)); + Ok(()) + } + RejectOpWith::Warning => { + register_diagnostic(NonHaltingDiagnostic::RejectedIsolatedOp(op_name.to_string())); + Ok(()) + } + RejectOpWith::NoWarning => Ok(()), // no warning + } + } + /// Helper function used inside the shims of foreign functions to assert that the target OS /// is `target_os`. It panics showing a message with the `name` of the foreign function /// if this is not the case. @@ -651,7 +671,7 @@ where throw_ub_format!("incorrect number of arguments: got {}, expected {}", args.len(), N) } -pub fn isolation_error(name: &str) -> InterpResult<'static> { +pub fn isolation_abort_error(name: &str) -> InterpResult<'static> { throw_machine_stop!(TerminationInfo::UnsupportedInIsolation(format!( "{} not available when isolation is enabled", name, diff --git a/src/lib.rs b/src/lib.rs index 25806b472b6..8c0a19b6dfb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -59,7 +59,9 @@ pub use crate::diagnostics::{ register_diagnostic, report_error, EvalContextExt as DiagnosticsEvalContextExt, NonHaltingDiagnostic, TerminationInfo, }; -pub use crate::eval::{create_ecx, eval_main, AlignmentCheck, MiriConfig}; +pub use crate::eval::{ + create_ecx, eval_main, AlignmentCheck, IsolatedOp, MiriConfig, RejectOpWith, +}; pub use crate::helpers::EvalContextExt as HelpersEvalContextExt; pub use crate::machine::{ AllocExtra, Evaluator, FrameData, MemoryExtra, MiriEvalContext, MiriEvalContextExt, diff --git a/src/machine.rs b/src/machine.rs index 9c49575ded3..752d2134482 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -263,9 +263,10 @@ pub struct Evaluator<'mir, 'tcx> { /// TLS state. pub(crate) tls: TlsData<'tcx>, - /// If enabled, the `env_vars` field is populated with the host env vars during initialization - /// and random number generation is delegated to the host. - pub(crate) communicate: bool, + /// What should Miri do when an op requires communicating with the host, + /// such as accessing host env vars, random number generation, and + /// file system access. + pub(crate) isolated_op: IsolatedOp, /// Whether to enforce the validity invariant. pub(crate) validate: bool, @@ -314,7 +315,7 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> { argv: None, cmd_line: None, tls: TlsData::default(), - communicate: config.communicate, + isolated_op: config.isolated_op, validate: config.validate, enforce_abi: config.check_abi, file_handler: Default::default(), @@ -328,6 +329,10 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> { exported_symbols_cache: FxHashMap::default(), } } + + pub(crate) fn communicate(&self) -> bool { + self.isolated_op == IsolatedOp::Allow + } } /// A rustc InterpCx for Miri. diff --git a/src/shims/env.rs b/src/shims/env.rs index a0916975322..9a68cf7bd53 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -1,6 +1,7 @@ use std::convert::TryFrom; use std::env; use std::ffi::{OsStr, OsString}; +use std::io::{Error, ErrorKind}; use rustc_data_structures::fx::FxHashMap; use rustc_mir::interpret::Pointer; @@ -45,7 +46,7 @@ impl<'tcx> EnvVars<'tcx> { excluded_env_vars.push("TERM".to_owned()); } - if ecx.machine.communicate { + if ecx.machine.communicate() { for (name, value) in env::vars() { if !excluded_env_vars.contains(&name) { let var_ptr = match target_os { @@ -321,7 +322,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx "`getcwd` is only available for the UNIX target family" ); - this.check_no_isolation("`getcwd`")?; + if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { + this.reject_in_isolation("getcwd", reject_with)?; + let err = Error::new(ErrorKind::NotFound, "rejected due to isolation"); + this.set_last_error_from_io_error(err)?; + return Ok(Scalar::null_ptr(&*this.tcx)); + } let buf = this.read_scalar(&buf_op)?.check_init()?; let size = this.read_scalar(&size_op)?.to_machine_usize(&*this.tcx)?; @@ -336,6 +342,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } Err(e) => this.set_last_error_from_io_error(e)?, } + Ok(Scalar::null_ptr(&*this.tcx)) } @@ -348,7 +355,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let this = self.eval_context_mut(); this.assert_target_os("windows", "GetCurrentDirectoryW"); - this.check_no_isolation("`GetCurrentDirectoryW`")?; + if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { + this.reject_in_isolation("GetCurrentDirectoryW", reject_with)?; + let err = Error::new(ErrorKind::NotFound, "rejected due to isolation"); + this.set_last_error_from_io_error(err)?; + return Ok(0); + } let size = u64::from(this.read_scalar(size_op)?.to_u32()?); let buf = this.read_scalar(buf_op)?.check_init()?; @@ -370,7 +382,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx "`getcwd` is only available for the UNIX target family" ); - this.check_no_isolation("`chdir`")?; + if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { + this.reject_in_isolation("chdir", reject_with)?; + let err = Error::new(ErrorKind::NotFound, "rejected due to isolation"); + this.set_last_error_from_io_error(err)?; + + return Ok(-1); + } let path = this.read_path_from_c_str(this.read_scalar(path_op)?.check_init()?)?; @@ -393,7 +411,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let this = self.eval_context_mut(); this.assert_target_os("windows", "SetCurrentDirectoryW"); - this.check_no_isolation("`SetCurrentDirectoryW`")?; + if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { + this.reject_in_isolation("SetCurrentDirectoryW", reject_with)?; + let err = Error::new(ErrorKind::NotFound, "rejected due to isolation"); + this.set_last_error_from_io_error(err)?; + + return Ok(0); + } let path = this.read_path_from_wide_str(this.read_scalar(path_op)?.check_init()?)?; diff --git a/src/shims/posix/fs.rs b/src/shims/posix/fs.rs index 06594e5ca1d..ca7a91a0f94 100644 --- a/src/shims/posix/fs.rs +++ b/src/shims/posix/fs.rs @@ -127,7 +127,7 @@ impl FileDescriptor for io::Stdin { ) -> InterpResult<'tcx, io::Result> { if !communicate_allowed { // We want isolation mode to be deterministic, so we have to disallow all reads, even stdin. - helpers::isolation_error("`read` from stdin")?; + helpers::isolation_abort_error("`read` from stdin")?; } Ok(Read::read(self, bytes)) } @@ -662,7 +662,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let fd = this.read_scalar(fd_op)?.to_i32()?; if let Some(file_descriptor) = this.machine.file_handler.handles.remove(&fd) { - let result = file_descriptor.close(this.machine.communicate)?; + let result = file_descriptor.close(this.machine.communicate())?; this.try_unwrap_io_result(result) } else { this.handle_not_found() @@ -687,6 +687,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // We cap the number of read bytes to the largest value that we are able to fit in both the // host's and target's `isize`. This saves us from having to handle overflows later. let count = count.min(this.machine_isize_max() as u64).min(isize::MAX as u64); + let communicate = this.machine.communicate(); if let Some(file_descriptor) = this.machine.file_handler.handles.get_mut(&fd) { trace!("read: FD mapped to {:?}", file_descriptor); @@ -696,9 +697,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let mut bytes = vec![0; count as usize]; // `File::read` never returns a value larger than `count`, // so this cannot fail. - let result = file_descriptor - .read(this.machine.communicate, &mut bytes)? - .map(|c| i64::try_from(c).unwrap()); + let result = + file_descriptor.read(communicate, &mut bytes)?.map(|c| i64::try_from(c).unwrap()); match result { Ok(read_bytes) => { @@ -733,12 +733,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // We cap the number of written bytes to the largest value that we are able to fit in both the // host's and target's `isize`. This saves us from having to handle overflows later. let count = count.min(this.machine_isize_max() as u64).min(isize::MAX as u64); + let communicate = this.machine.communicate(); if let Some(file_descriptor) = this.machine.file_handler.handles.get_mut(&fd) { let bytes = this.memory.read_bytes(buf, Size::from_bytes(count))?; - let result = file_descriptor - .write(this.machine.communicate, &bytes)? - .map(|c| i64::try_from(c).unwrap()); + let result = + file_descriptor.write(communicate, &bytes)?.map(|c| i64::try_from(c).unwrap()); this.try_unwrap_io_result(result) } else { this.handle_not_found() @@ -771,9 +771,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx return Ok(-1); }; + let communicate = this.machine.communicate(); if let Some(file_descriptor) = this.machine.file_handler.handles.get_mut(&fd) { let result = file_descriptor - .seek(this.machine.communicate, seek_from)? + .seek(communicate, seek_from)? .map(|offset| i64::try_from(offset).unwrap()); this.try_unwrap_io_result(result) } else { diff --git a/tests/run-pass/current_dir_with_isolation.rs b/tests/run-pass/current_dir_with_isolation.rs new file mode 100644 index 00000000000..ea891c89983 --- /dev/null +++ b/tests/run-pass/current_dir_with_isolation.rs @@ -0,0 +1,20 @@ +// compile-flags: -Zmiri-isolation-error=warn-nobacktrace +// normalize-stderr-test "(getcwd|GetCurrentDirectoryW)" -> "$$GET" +// normalize-stderr-test "(chdir|SetCurrentDirectoryW)" -> "$$SET" + +use std::env; +use std::io::ErrorKind; + +fn main() { + // Test that current dir operations return a proper error instead + // of stopping the machine in isolation mode + assert_eq!(env::current_dir().unwrap_err().kind(), ErrorKind::NotFound); + for _i in 0..3 { + assert_eq!(env::current_dir().unwrap_err().kind(), ErrorKind::NotFound); + } + + assert_eq!(env::set_current_dir("..").unwrap_err().kind(), ErrorKind::NotFound); + for _i in 0..3 { + assert_eq!(env::set_current_dir("..").unwrap_err().kind(), ErrorKind::NotFound); + } +} diff --git a/tests/run-pass/current_dir_with_isolation.stderr b/tests/run-pass/current_dir_with_isolation.stderr new file mode 100644 index 00000000000..cc0975230de --- /dev/null +++ b/tests/run-pass/current_dir_with_isolation.stderr @@ -0,0 +1,4 @@ +warning: `$GET` was made to return an error due to isolation + +warning: `$SET` was made to return an error due to isolation +