Consolidated settings for isolated ops

atsmtat · atsmtat · commit 68a15306af9a · 2021-06-01T20:27:27.000-07:00
In user interface, added a new option `-Zmiri-isolated-op` which takes
one of the four values -- hide, warn, warn-nobacktrace, and
allow. This option can be used to both disable isolation (allow), and
to control warning levels if op is rejected due to isolation.

In implementation, added a new enum `IsolatedOp` to capture all the
settings related to ops requiring communication with the
host. Existing `communicate` flag in both miri configs and machine
state will be discarded once its use is replaced with the new enum.

Added a new helper function to report rejected ops in isolation
according to the warning settings. Refactored miri specific
diagnostics function `report_msg` to take an enum value instead of a
bool, indicating the level of diagnostics.

Updated shims related to current dir to use the new enum.

Added a .stderr file for the new test, matching the warning thrown.
diff --git a/src/bin/miri.rs b/src/bin/miri.rs
@@ -229,19 +229,14 @@ fn main() {
                 }
                 "-Zmiri-disable-isolation" => {
                     miri_config.communicate = true;
+                    miri_config.isolated_op = miri::IsolatedOp::Allow;
                 }
                 "-Zmiri-ignore-leaks" => {
                     miri_config.ignore_leaks = true;
                 }
                 "-Zmiri-track-raw-pointers" => {
                     miri_config.track_raw = true;
                 }
-                "-Zmiri-track-dummy-op" => {
-                    miri_config.dummy_op_visibility = miri::DummyOpVisibility::Track;
-                }
-                "-Zmiri-ignore-dummy-op" => {
-                    miri_config.dummy_op_visibility = miri::DummyOpVisibility::Hide;
-                }
                 "--" => {
                     after_dashdash = true;
                 }
@@ -328,6 +323,17 @@ fn main() {
                     let measureme_out = arg.strip_prefix("-Zmiri-measureme=").unwrap();
                     miri_config.measureme_out = Some(measureme_out.to_string());
                 }
+                arg if arg.starts_with("-Zmiri-isolated-op=") => {
+                    let action = match arg.strip_prefix("-Zmiri-isolated-op=").unwrap() {
+                        "hide" => miri::IsolatedOp::Reject(miri::RejectOpWith::NoWarning),
+                        "warn" => miri::IsolatedOp::Reject(miri::RejectOpWith::Warning),
+                        "warn-nobacktrace" => miri::IsolatedOp::Reject(miri::RejectOpWith::WarningWithoutBacktrace),
+                        "allow" => miri::IsolatedOp::Allow,
+                        _ => panic!("-Zmiri-isolated-op must be `hide`, `warn`, `warn-nobacktrace`, or `allow`"),
+                        
+                    };
+                    miri_config.isolated_op = action;
+                }
                 _ => {
                     // Forward to rustc.
                     rustc_args.push(arg);
diff --git a/src/diagnostics.rs b/src/diagnostics.rs
@@ -40,7 +40,14 @@ pub enum NonHaltingDiagnostic {
     CreatedCallId(CallId),
     CreatedAlloc(AllocId),
     FreedAlloc(AllocId),
-    DummyOpInIsolation(String),
+    RejectedIsolatedOp(String),
+}
+
+/// Level of Miri specific diagnostics
+enum DiagLevel {
+    Error,
+    Warning,
+    Note,
 }
 
 /// Emit a custom diagnostic without going through the miri-engine machinery
@@ -120,7 +127,7 @@ pub fn report_error<'tcx, 'mir>(
     let msg = e.to_string();
     report_msg(
         *ecx.tcx,
-        /*error*/ true,
+        DiagLevel::Error,
         &format!("{}: {}", title, msg),
         msg,
         helps,
@@ -157,18 +164,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<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);
@@ -282,12 +290,18 @@ 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),
-                    DummyOpInIsolation(op) => format!("produced dummy error for `{}`", op),
+                    RejectedIsolatedOp(ref op) => format!("`{}` was made to return an error due to isolation", op),
                 };
+
+                let (title, diag_level) = match e {
+                    RejectedIsolatedOp(_) => ("operation rejected by isolation", DiagLevel::Warning),
+                    _ => ("tracking was triggered", DiagLevel::Note),
+                };
+
                 report_msg(
                     *this.tcx,
-                    /*error*/ false,
-                    "tracking was triggered",
+                    diag_level,
+                    title,
                     msg,
                     vec![],
                     &stacktrace,
diff --git a/src/eval.rs b/src/eval.rs
@@ -23,13 +23,26 @@ pub enum AlignmentCheck {
 }
 
 #[derive(Copy, Clone, Debug)]
-pub enum DummyOpVisibility {
-    /// Do not report a dummy op
-    Hide,
-    /// Print a warning without a backtrace, once per dummy op
-    Warn,
-    /// Print a note with a backtrace, once per line calling dummy op
-    Track,
+pub enum RejectOpWith {
+    /// 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)]
+pub enum IsolatedOp {
+    /// Reject op requiring communication with the host. Usually, miri
+    /// generates a fake error for such op, 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.
@@ -43,6 +56,8 @@ pub struct MiriConfig {
     pub check_alignment: AlignmentCheck,
     /// 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.
@@ -76,6 +91,7 @@ impl Default for MiriConfig {
             stacked_borrows: true,
             check_alignment: AlignmentCheck::Int,
             communicate: false,
+            isolated_op: IsolatedOp::Reject(RejectOpWith::WarningWithoutBacktrace),
             ignore_leaks: false,
             excluded_env_vars: vec![],
             args: vec![],
diff --git a/src/helpers.rs b/src/helpers.rs
@@ -397,23 +397,23 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         Ok(())
     }
 
-    /// Helper function used inside the shims of foreign functions which produce a dummy error
-    /// when isolation is enabled. It is used to print a warning/backtrace, informing this
-    /// decision to the user.
-    fn report_dummy_in_isolation(&self, name: &str) {
+    /// 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 report_rejected_op(&self, op_name: &str, reject_with: RejectOpWith) {
         let this = self.eval_context_ref();
-        match this.machine.dummy_op_visibility {
-            DummyOpVisibility::Warn => {
-                let msg = format!("`{}` in isolation mode produced a dummy error", name);
+        match reject_with {
+            RejectOpWith::WarningWithoutBacktrace => {
+                let msg = format!("`{}` was made to return an error due to isolation", op_name);
                 let mut warn = this.tcx.sess.struct_warn(&msg);
-                warn.note("run with -Zmiri-track-dummy-op to track with a backtrace");
-                warn.note("run with -Zmiri-ignore-dummy-op to ignore warning");
+                warn.note("run with -Zmiri-isolated-op=warn to get warning with a backtrace");
+                warn.note("run with -Zmiri-isolated-op=hide to hide warning");
+                warn.note("run with -Zmiri-isolated-op=allow to disable isolation");
                 warn.emit();
             }
-            DummyOpVisibility::Track => {
-                register_diagnostic(NonHaltingDiagnostic::DummyOpInIsolation(name.to_string()));
+            RejectOpWith::Warning => {
+                register_diagnostic(NonHaltingDiagnostic::RejectedIsolatedOp(op_name.to_string()));
             }
-            DummyOpVisibility::Hide => {} // no diagnostics
+            RejectOpWith::NoWarning => {} // no warning
         }
     }
 
diff --git a/src/lib.rs b/src/lib.rs
@@ -57,7 +57,7 @@ pub use crate::diagnostics::{
     register_diagnostic, report_error, EvalContextExt as DiagnosticsEvalContextExt,
     NonHaltingDiagnostic, TerminationInfo,
 };
-pub use crate::eval::{create_ecx, eval_main, AlignmentCheck, DummyOpVisibility, 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
@@ -267,8 +267,10 @@ pub struct Evaluator<'mir, 'tcx> {
     /// and random number generation is delegated to the host.
     pub(crate) communicate: bool,
 
-    /// Level of user visibility to dummy ops run in isolation mode.
-    pub(crate) dummy_op_visibility: DummyOpVisibility,
+    /// 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,
@@ -312,6 +314,7 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
             cmd_line: None,
             tls: TlsData::default(),
             communicate: config.communicate,
+            isolated_op: config.isolated_op,
             validate: config.validate,
             file_handler: Default::default(),
             dir_handler: Default::default(),
diff --git a/src/shims/env.rs b/src/shims/env.rs
@@ -322,25 +322,27 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             "`getcwd` is only available for the UNIX target family"
         );
 
-        if this.machine.communicate {
-            let buf = this.read_scalar(&buf_op)?.check_init()?;
-            let size = this.read_scalar(&size_op)?.to_machine_usize(&*this.tcx)?;
-            // If we cannot get the current directory, we return null
-            match env::current_dir() {
-                Ok(cwd) => {
-                    if this.write_path_to_c_str(&cwd, buf, size)?.0 {
-                        return Ok(buf);
+        match this.machine.isolated_op {
+            IsolatedOp::Allow => {
+                let buf = this.read_scalar(&buf_op)?.check_init()?;
+                let size = this.read_scalar(&size_op)?.to_machine_usize(&*this.tcx)?;
+                // If we cannot get the current directory, we return null
+                match env::current_dir() {
+                    Ok(cwd) => {
+                        if this.write_path_to_c_str(&cwd, buf, size)?.0 {
+                            return Ok(buf);
+                        }
+                        let erange = this.eval_libc("ERANGE")?;
+                        this.set_last_error(erange)?;
                     }
-                    let erange = this.eval_libc("ERANGE")?;
-                    this.set_last_error(erange)?;
+                    Err(e) => this.set_last_error_from_io_error(e)?,
                 }
-                Err(e) => this.set_last_error_from_io_error(e)?,
             }
-        } else {
-            // Return a dummy error in isolation mode, after printing a warning about it.
-            this.report_dummy_in_isolation("getcwd");
-            let err = Error::new(ErrorKind::NotFound, "dummy error in isolation mode");
-            this.set_last_error_from_io_error(err)?;
+            IsolatedOp::Reject(reject_with) => {
+                let err = Error::new(ErrorKind::NotFound, "rejected due to isolation");
+                this.set_last_error_from_io_error(err)?;
+                this.report_rejected_op("getcwd", reject_with);
+            }
         }
         Ok(Scalar::null_ptr(&*this.tcx))
     }
@@ -353,22 +355,24 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     ) -> InterpResult<'tcx, u32> {
         let this = self.eval_context_mut();
         this.assert_target_os("windows", "GetCurrentDirectoryW");
-
-        if this.machine.communicate {
-            let size = u64::from(this.read_scalar(size_op)?.to_u32()?);
-            let buf = this.read_scalar(buf_op)?.check_init()?;
-
-            // If we cannot get the current directory, we return 0
-            match env::current_dir() {
-                Ok(cwd) =>
-                    return Ok(windows_check_buffer_size(this.write_path_to_wide_str(&cwd, buf, size)?)),
-                Err(e) => this.set_last_error_from_io_error(e)?,
+        
+        match this.machine.isolated_op {
+            IsolatedOp::Allow => {
+                let size = u64::from(this.read_scalar(size_op)?.to_u32()?);
+                let buf = this.read_scalar(buf_op)?.check_init()?;
+
+                // If we cannot get the current directory, we return 0
+                match env::current_dir() {
+                    Ok(cwd) =>
+                        return Ok(windows_check_buffer_size(this.write_path_to_wide_str(&cwd, buf, size)?)),
+                    Err(e) => this.set_last_error_from_io_error(e)?,
+                }
+            }
+            IsolatedOp::Reject(reject_with) => {
+                let err = Error::new(ErrorKind::NotFound, "rejected due to isolation");
+                this.set_last_error_from_io_error(err)?;
+                this.report_rejected_op("GetCurrentDirectoryW", reject_with);
             }
-        } else {
-            // Return a dummy error in isolation mode, after printing a warning about it.
-            this.report_dummy_in_isolation("GetCurrentDirectoryW");
-            let err = Error::new(ErrorKind::NotFound, "dummy error in isolation mode");
-            this.set_last_error_from_io_error(err)?;
         }
         Ok(0)
     }
@@ -381,23 +385,25 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             "`getcwd` is only available for the UNIX target family"
         );
 
-        if this.machine.communicate {
-            let path = this.read_path_from_c_str(this.read_scalar(path_op)?.check_init()?)?;
+        match this.machine.isolated_op {
+            IsolatedOp::Allow => {
+                let path = this.read_path_from_c_str(this.read_scalar(path_op)?.check_init()?)?;
 
-            match env::set_current_dir(path) {
-                Ok(()) => Ok(0),
-                Err(e) => {
-                    this.set_last_error_from_io_error(e)?;
-                    Ok(-1)
+                match env::set_current_dir(path) {
+                    Ok(()) => Ok(0),
+                    Err(e) => {
+                        this.set_last_error_from_io_error(e)?;
+                        Ok(-1)
+                    }
                 }
             }
-        } else {
-            // Return a dummy error in isolation mode, after printing a warning about it.
-            this.report_dummy_in_isolation("chdir");
-            let err = Error::new(ErrorKind::NotFound, "dummy error in isolation mode");
-            this.set_last_error_from_io_error(err)?;
+            IsolatedOp::Reject(reject_with) => {
+                let err = Error::new(ErrorKind::NotFound, "rejected due to isolation");
+                this.set_last_error_from_io_error(err)?;
+                this.report_rejected_op("chdir", reject_with);
 
-            Ok(-1)
+                Ok(-1)
+            }
         }
     }
 
@@ -411,22 +417,25 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let this = self.eval_context_mut();
         this.assert_target_os("windows", "SetCurrentDirectoryW");
 
-        if this.machine.communicate {
-            let path = this.read_path_from_wide_str(this.read_scalar(path_op)?.check_init()?)?;
+        match this.machine.isolated_op {
+            IsolatedOp::Allow => {
+                let path = this.read_path_from_wide_str(this.read_scalar(path_op)?.check_init()?)?;
 
-            match env::set_current_dir(path) {
-                Ok(()) => Ok(1),
-                Err(e) => {
-                    this.set_last_error_from_io_error(e)?;
-                    Ok(0)
+                match env::set_current_dir(path) {
+                    Ok(()) => Ok(1),
+                    Err(e) => {
+                        this.set_last_error_from_io_error(e)?;
+                        Ok(0)
+                    }
                 }
             }
-        } else {
-            // Return a dummy error in isolation mode, after printing a warning about it.
-            this.report_dummy_in_isolation("SetCurrentDirectoryW");
-            let err = Error::new(ErrorKind::NotFound, "dummy error in isolation mode");
-            this.set_last_error_from_io_error(err)?;
-            Ok(0)
+            IsolatedOp::Reject(reject_with) => {
+                let err = Error::new(ErrorKind::NotFound, "rejected due to isolation");
+                this.set_last_error_from_io_error(err)?;
+                this.report_rejected_op("SetCurrentDirectoryW", reject_with);
+
+                Ok(0)
+            }
         }
     }
 
diff --git a/tests/run-pass/current_dir_with_isolation.stderr b/tests/run-pass/current_dir_with_isolation.stderr
