Auto merge of rust-lang#3184 - RalfJung:getenv, r=RalfJung

detect and test for data races between setenv and getenv

But only on Unix; Windows doesn't have such a data race. Also make target_os_is_unix properly check for unix, which then makes our completely empty android files useless.

Author: bors

src/tools/miri/src/helpers.rs

@@ -565,10 +565,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
     /// 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
@@ -1161,12 +1162,6 @@ pub fn get_local_crates(tcx: TyCtxt<'_>) -> Vec<CrateNum> {
     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<Provenance> {
     // 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

src/tools/miri/src/shims/env.rs

@@ -9,7 +9,6 @@ use rustc_middle::ty::layout::LayoutOf;
 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 @@ impl<'tcx> EnvVars<'tcx> {
         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 @@ impl<'tcx> EnvVars<'tcx> {
                 }
             }
         }
-        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 @@ impl<'tcx> EnvVars<'tcx> {
             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 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 
         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 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
             // 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 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
             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 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
     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,8 +463,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, '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(())
     }
 

src/tools/miri/src/shims/foreign_items.rs

@@ -22,7 +22,6 @@ use rustc_target::{
 };
 
 use super::backtrace::EvalContextExt as _;
-use crate::helpers::target_os_is_unix;
 use crate::*;
 
 /// Type of dynamic symbols (for `dlsym` et al)
@@ -1060,7 +1059,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
             // 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,
                         ),

src/tools/miri/src/shims/unix/android/foreign_items.rs

@@ -15,7 +15,6 @@ use shims::unix::mem::EvalContextExt as _;
 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 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
             _ => {
                 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),

src/tools/miri/src/shims/unix/android/mod.rs

@@ -5,7 +5,6 @@ mod mem;
 mod sync;
 mod thread;
 
-mod android;
 mod freebsd;
 mod linux;
 mod macos;

src/tools/miri/src/shims/unix/foreign_items.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();
+}

src/tools/miri/src/shims/unix/mod.rs

@@ -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 `<unnamed>` 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 `<unnamed>` 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
+

src/tools/miri/tests/fail-dep/shims/env-set_var-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.
     }
 }