From 4896c953e18869c57c10cea3aceb81d56537e504 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 23 Nov 2023 08:14:50 +0100 Subject: [PATCH 1/2] detect and test for data races between setenv and getenv --- src/tools/miri/src/helpers.rs | 15 ++---- src/tools/miri/src/shims/env.rs | 53 ++++++++++++------- src/tools/miri/src/shims/foreign_items.rs | 3 +- .../src/shims/unix/android/foreign_items.rs | 2 +- .../fail-dep/shims/env-set_var-data-race.rs | 17 ++++++ .../shims/env-set_var-data-race.stderr | 20 +++++++ .../pass-dep/shims/env-cleanup-data-race.rs | 5 +- 7 files changed, 79 insertions(+), 36 deletions(-) create mode 100644 src/tools/miri/tests/fail-dep/shims/env-set_var-data-race.rs create mode 100644 src/tools/miri/tests/fail-dep/shims/env-set_var-data-race.stderr diff --git a/src/tools/miri/src/helpers.rs b/src/tools/miri/src/helpers.rs index 965cd534d1e..f3890fc1dcb 100644 --- a/src/tools/miri/src/helpers.rs +++ b/src/tools/miri/src/helpers.rs @@ -565,10 +565,11 @@ fn assert_target_os(&self, target_os: &str, name: &str) { /// is part of the UNIX family. It panics showing a message with the `name` of the foreign function /// if this is not the case. fn assert_target_os_is_unix(&self, name: &str) { - assert!( - target_os_is_unix(self.eval_context_ref().tcx.sess.target.os.as_ref()), - "`{name}` is only available for supported UNIX family targets", - ); + assert!(self.target_os_is_unix(), "`{name}` is only available for unix targets",); + } + + fn target_os_is_unix(&self) -> bool { + self.eval_context_ref().tcx.sess.target.families.iter().any(|f| f == "unix") } /// Get last error variable as a place, lazily allocating thread-local storage for it if @@ -1143,12 +1144,6 @@ pub fn get_local_crates(tcx: TyCtxt<'_>) -> Vec { local_crates } -/// Helper function used inside the shims of foreign functions to check that -/// `target_os` is a supported UNIX OS. -pub fn target_os_is_unix(target_os: &str) -> bool { - matches!(target_os, "linux" | "macos" | "freebsd" | "android") -} - pub(crate) fn bool_to_simd_element(b: bool, size: Size) -> Scalar { // SIMD uses all-1 as pattern for "true". In two's complement, // -1 has all its bits set to one and `from_int` will truncate or diff --git a/src/tools/miri/src/shims/env.rs b/src/tools/miri/src/shims/env.rs index 42438985907..9e19f720211 100644 --- a/src/tools/miri/src/shims/env.rs +++ b/src/tools/miri/src/shims/env.rs @@ -9,7 +9,6 @@ use rustc_middle::ty::Ty; use rustc_target::abi::Size; -use crate::helpers::target_os_is_unix; use crate::*; /// Check whether an operation that writes to a target buffer was successful. @@ -53,16 +52,15 @@ pub(crate) fn init<'mir>( ecx: &mut InterpCx<'mir, 'tcx, MiriMachine<'mir, 'tcx>>, config: &MiriConfig, ) -> InterpResult<'tcx> { - let target_os = ecx.tcx.sess.target.os.as_ref(); - + // Initialize the `env_vars` map. // Skip the loop entirely if we don't want to forward anything. if ecx.machine.communicate() || !config.forwarded_env_vars.is_empty() { for (name, value) in &config.env { let forward = ecx.machine.communicate() || config.forwarded_env_vars.iter().any(|v| **v == *name); if forward { - let var_ptr = match target_os { - target if target_os_is_unix(target) => + let var_ptr = match ecx.tcx.sess.target.os.as_ref() { + _ if ecx.target_os_is_unix() => alloc_env_var_as_c_str(name.as_ref(), value.as_ref(), ecx)?, "windows" => alloc_env_var_as_wide_str(name.as_ref(), value.as_ref(), ecx)?, unsupported => @@ -75,7 +73,17 @@ pub(crate) fn init<'mir>( } } } - ecx.update_environ() + + // Initialize the `environ` pointer when needed. + if ecx.target_os_is_unix() { + // This is memory backing an extern static, hence `ExternStatic`, not `Env`. + let layout = ecx.machine.layouts.mut_raw_ptr; + let place = ecx.allocate(layout, MiriMemoryKind::ExternStatic.into())?; + ecx.write_null(&place)?; + ecx.machine.env_vars.environ = Some(place); + ecx.update_environ()?; + } + Ok(()) } pub(crate) fn cleanup<'mir>( @@ -87,9 +95,11 @@ pub(crate) fn cleanup<'mir>( ecx.deallocate_ptr(ptr, None, MiriMemoryKind::Runtime.into())?; } // Deallocate environ var list. - let environ = ecx.machine.env_vars.environ.as_ref().unwrap(); - let old_vars_ptr = ecx.read_pointer(environ)?; - ecx.deallocate_ptr(old_vars_ptr, None, MiriMemoryKind::Runtime.into())?; + if ecx.target_os_is_unix() { + let environ = ecx.machine.env_vars.environ.as_ref().unwrap(); + let old_vars_ptr = ecx.read_pointer(environ)?; + ecx.deallocate_ptr(old_vars_ptr, None, MiriMemoryKind::Runtime.into())?; + } Ok(()) } } @@ -127,6 +137,7 @@ fn getenv( let name_ptr = this.read_pointer(name_op)?; let name = this.read_os_str_from_c_str(name_ptr)?; + this.read_environ()?; Ok(match this.machine.env_vars.map.get(name) { Some(var_ptr) => { // The offset is used to strip the "{name}=" part of the string. @@ -275,7 +286,6 @@ fn SetEnvironmentVariableW( // Delete environment variable `{name}` if let Some(var) = this.machine.env_vars.map.remove(&name) { this.deallocate_ptr(var, None, MiriMemoryKind::Runtime.into())?; - this.update_environ()?; } Ok(this.eval_windows("c", "TRUE")) } else { @@ -284,7 +294,6 @@ fn SetEnvironmentVariableW( if let Some(var) = this.machine.env_vars.map.insert(name, var_ptr) { this.deallocate_ptr(var, None, MiriMemoryKind::Runtime.into())?; } - this.update_environ()?; Ok(this.eval_windows("c", "TRUE")) } } @@ -431,15 +440,10 @@ fn SetCurrentDirectoryW( fn update_environ(&mut self) -> InterpResult<'tcx> { let this = self.eval_context_mut(); // Deallocate the old environ list, if any. - if let Some(environ) = this.machine.env_vars.environ.as_ref() { - let old_vars_ptr = this.read_pointer(environ)?; + let environ = this.machine.env_vars.environ.as_ref().unwrap().clone(); + let old_vars_ptr = this.read_pointer(&environ)?; + if !this.ptr_is_null(old_vars_ptr)? { this.deallocate_ptr(old_vars_ptr, None, MiriMemoryKind::Runtime.into())?; - } else { - // No `environ` allocated yet, let's do that. - // This is memory backing an extern static, hence `ExternStatic`, not `Env`. - let layout = this.machine.layouts.mut_raw_ptr; - let place = this.allocate(layout, MiriMemoryKind::ExternStatic.into())?; - this.machine.env_vars.environ = Some(place); } // Collect all the pointers to each variable in a vector. @@ -459,11 +463,20 @@ fn update_environ(&mut self) -> InterpResult<'tcx> { let place = this.project_field(&vars_place, idx)?; this.write_pointer(var, &place)?; } - this.write_pointer(vars_place.ptr(), &this.machine.env_vars.environ.clone().unwrap())?; + this.write_pointer(vars_place.ptr(), &environ)?; Ok(()) } + /// Reads from the `environ` static. + /// We don't actually care about the result, but we care about this potentially causing a data race. + fn read_environ(&self) -> InterpResult<'tcx> { + let this = self.eval_context_ref(); + let environ = this.machine.env_vars.environ.as_ref().unwrap(); + let _vars_ptr = this.read_pointer(environ)?; + Ok(()) + } + fn getpid(&mut self) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); this.assert_target_os_is_unix("getpid"); diff --git a/src/tools/miri/src/shims/foreign_items.rs b/src/tools/miri/src/shims/foreign_items.rs index d7aaa08dbf3..6443efd7bde 100644 --- a/src/tools/miri/src/shims/foreign_items.rs +++ b/src/tools/miri/src/shims/foreign_items.rs @@ -22,7 +22,6 @@ }; use super::backtrace::EvalContextExt as _; -use crate::helpers::target_os_is_unix; use crate::*; /// Type of dynamic symbols (for `dlsym` et al) @@ -1058,7 +1057,7 @@ fn emulate_foreign_item_inner( // Platform-specific shims _ => return match this.tcx.sess.target.os.as_ref() { - target_os if target_os_is_unix(target_os) => + _ if this.target_os_is_unix() => shims::unix::foreign_items::EvalContextExt::emulate_foreign_item_inner( this, link_name, abi, args, dest, ), diff --git a/src/tools/miri/src/shims/unix/android/foreign_items.rs b/src/tools/miri/src/shims/unix/android/foreign_items.rs index f61ebd5a3a8..5653e3f1129 100644 --- a/src/tools/miri/src/shims/unix/android/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/android/foreign_items.rs @@ -11,7 +11,7 @@ pub fn is_dyn_sym(_name: &str) -> bool { } pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { - #[allow(unused, clippy::match_single_binding)] // there isn't anything here yet + #[allow(unused, clippy::match_single_binding)] // FIXME: there isn't anything here yet fn emulate_foreign_item_inner( &mut self, link_name: Symbol, diff --git a/src/tools/miri/tests/fail-dep/shims/env-set_var-data-race.rs b/src/tools/miri/tests/fail-dep/shims/env-set_var-data-race.rs new file mode 100644 index 00000000000..2b9e7a34d65 --- /dev/null +++ b/src/tools/miri/tests/fail-dep/shims/env-set_var-data-race.rs @@ -0,0 +1,17 @@ +//@compile-flags: -Zmiri-disable-isolation -Zmiri-preemption-rate=0 +//@ignore-target-windows: No libc on Windows + +use std::env; +use std::thread; + +fn main() { + let t = thread::spawn(|| unsafe { + // Access the environment in another thread without taking the env lock. + // This represents some C code that queries the environment. + libc::getenv(b"TZ\0".as_ptr().cast()); //~ERROR: Data race detected + }); + // Meanwhile, the main thread uses the "safe" Rust env accessor. + env::set_var("MY_RUST_VAR", "Ferris"); + + t.join().unwrap(); +} diff --git a/src/tools/miri/tests/fail-dep/shims/env-set_var-data-race.stderr b/src/tools/miri/tests/fail-dep/shims/env-set_var-data-race.stderr new file mode 100644 index 00000000000..a202d7c6844 --- /dev/null +++ b/src/tools/miri/tests/fail-dep/shims/env-set_var-data-race.stderr @@ -0,0 +1,20 @@ +error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `main` and (2) non-atomic read on thread `` at ALLOC. (2) just happened here + --> $DIR/env-set_var-data-race.rs:LL:CC + | +LL | libc::getenv(b"TZ/0".as_ptr().cast()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Data race detected between (1) non-atomic write on thread `main` and (2) non-atomic read on thread `` at ALLOC. (2) just happened here + | +help: and (1) occurred earlier here + --> $DIR/env-set_var-data-race.rs:LL:CC + | +LL | env::set_var("MY_RUST_VAR", "Ferris"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = 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 (of the first span): + = note: inside closure at $DIR/env-set_var-data-race.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error + diff --git a/src/tools/miri/tests/pass-dep/shims/env-cleanup-data-race.rs b/src/tools/miri/tests/pass-dep/shims/env-cleanup-data-race.rs index d36ffe70321..5c29a1b15a7 100644 --- a/src/tools/miri/tests/pass-dep/shims/env-cleanup-data-race.rs +++ b/src/tools/miri/tests/pass-dep/shims/env-cleanup-data-race.rs @@ -2,15 +2,13 @@ //@ignore-target-windows: No libc on Windows use std::ffi::CStr; -use std::ffi::CString; use std::thread; fn main() { unsafe { thread::spawn(|| { // Access the environment in another thread without taking the env lock - let k = CString::new("MIRI_ENV_VAR_TEST".as_bytes()).unwrap(); - let s = libc::getenv(k.as_ptr()) as *const libc::c_char; + let s = libc::getenv("MIRI_ENV_VAR_TEST\0".as_ptr().cast()); if s.is_null() { panic!("null"); } @@ -19,5 +17,6 @@ fn main() { thread::yield_now(); // After the main thread exits, env vars will be cleaned up -- but because we have not *joined* // the other thread, those accesses technically race with those in the other thread. + // We don't want to emit an error here, though. } } From 4b69e525f5e090d1268cbace2846a8985dce6fae Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 23 Nov 2023 08:19:01 +0100 Subject: [PATCH 2/2] remove stub android files that don't do anything --- .../src/shims/unix/android/foreign_items.rs | 30 ------------------- src/tools/miri/src/shims/unix/android/mod.rs | 1 - .../miri/src/shims/unix/foreign_items.rs | 5 +--- src/tools/miri/src/shims/unix/mod.rs | 1 - 4 files changed, 1 insertion(+), 36 deletions(-) delete mode 100644 src/tools/miri/src/shims/unix/android/foreign_items.rs delete mode 100644 src/tools/miri/src/shims/unix/android/mod.rs diff --git a/src/tools/miri/src/shims/unix/android/foreign_items.rs b/src/tools/miri/src/shims/unix/android/foreign_items.rs deleted file mode 100644 index 5653e3f1129..00000000000 --- a/src/tools/miri/src/shims/unix/android/foreign_items.rs +++ /dev/null @@ -1,30 +0,0 @@ -use rustc_span::Symbol; -use rustc_target::spec::abi::Abi; - -use crate::*; -use shims::foreign_items::EmulateForeignItemResult; - -impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} - -pub fn is_dyn_sym(_name: &str) -> bool { - false -} - -pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { - #[allow(unused, clippy::match_single_binding)] // FIXME: there isn't anything here yet - fn emulate_foreign_item_inner( - &mut self, - link_name: Symbol, - abi: Abi, - args: &[OpTy<'tcx, Provenance>], - dest: &PlaceTy<'tcx, Provenance>, - ) -> InterpResult<'tcx, EmulateForeignItemResult> { - let this = self.eval_context_mut(); - - match link_name.as_str() { - _ => return Ok(EmulateForeignItemResult::NotSupported), - } - - Ok(EmulateForeignItemResult::NeedsJumping) - } -} diff --git a/src/tools/miri/src/shims/unix/android/mod.rs b/src/tools/miri/src/shims/unix/android/mod.rs deleted file mode 100644 index 09c6507b24f..00000000000 --- a/src/tools/miri/src/shims/unix/android/mod.rs +++ /dev/null @@ -1 +0,0 @@ -pub mod foreign_items; diff --git a/src/tools/miri/src/shims/unix/foreign_items.rs b/src/tools/miri/src/shims/unix/foreign_items.rs index 7bc26788b26..c1c3e3fa10c 100644 --- a/src/tools/miri/src/shims/unix/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/foreign_items.rs @@ -15,7 +15,6 @@ use shims::unix::sync::EvalContextExt as _; use shims::unix::thread::EvalContextExt as _; -use shims::unix::android::foreign_items as android; use shims::unix::freebsd::foreign_items as freebsd; use shims::unix::linux::foreign_items as linux; use shims::unix::macos::foreign_items as macos; @@ -32,11 +31,10 @@ fn is_dyn_sym(name: &str, target_os: &str) -> bool { // Give specific OSes a chance to allow their symbols. _ => match target_os { - "android" => android::is_dyn_sym(name), "freebsd" => freebsd::is_dyn_sym(name), "linux" => linux::is_dyn_sym(name), "macos" => macos::is_dyn_sym(name), - target_os => panic!("unsupported Unix OS {target_os}"), + _ => false, }, } } @@ -706,7 +704,6 @@ fn emulate_foreign_item_inner( _ => { let target_os = &*this.tcx.sess.target.os; return match target_os { - "android" => android::EvalContextExt::emulate_foreign_item_inner(this, link_name, abi, args, dest), "freebsd" => freebsd::EvalContextExt::emulate_foreign_item_inner(this, link_name, abi, args, dest), "linux" => linux::EvalContextExt::emulate_foreign_item_inner(this, link_name, abi, args, dest), "macos" => macos::EvalContextExt::emulate_foreign_item_inner(this, link_name, abi, args, dest), diff --git a/src/tools/miri/src/shims/unix/mod.rs b/src/tools/miri/src/shims/unix/mod.rs index 2f801493352..638473da02b 100644 --- a/src/tools/miri/src/shims/unix/mod.rs +++ b/src/tools/miri/src/shims/unix/mod.rs @@ -5,7 +5,6 @@ mod sync; mod thread; -mod android; mod freebsd; mod linux; mod macos;