Skip to content

Commit

Permalink
Auto merge of rust-lang#3184 - RalfJung:getenv, r=RalfJung
Browse files Browse the repository at this point in the history
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.
bors committed Nov 23, 2023
2 parents 933bdbc + 4b69e52 commit 2a1e0ce
Showing 10 changed files with 79 additions and 71 deletions.
15 changes: 5 additions & 10 deletions src/tools/miri/src/helpers.rs
Original file line number Diff line number Diff line change
@@ -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
53 changes: 33 additions & 20 deletions src/tools/miri/src/shims/env.rs
Original file line number Diff line number Diff line change
@@ -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(())
}

3 changes: 1 addition & 2 deletions src/tools/miri/src/shims/foreign_items.rs
Original file line number Diff line number Diff line change
@@ -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,
),
30 changes: 0 additions & 30 deletions src/tools/miri/src/shims/unix/android/foreign_items.rs

This file was deleted.

1 change: 0 additions & 1 deletion src/tools/miri/src/shims/unix/android/mod.rs

This file was deleted.

5 changes: 1 addition & 4 deletions src/tools/miri/src/shims/unix/foreign_items.rs
Original file line number Diff line number Diff line change
@@ -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),
1 change: 0 additions & 1 deletion src/tools/miri/src/shims/unix/mod.rs
Original file line number Diff line number Diff line change
@@ -5,7 +5,6 @@ mod mem;
mod sync;
mod thread;

mod android;
mod freebsd;
mod linux;
mod macos;
17 changes: 17 additions & 0 deletions src/tools/miri/tests/fail-dep/shims/env-set_var-data-race.rs
Original file line number Diff line number Diff line change
@@ -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();
}
20 changes: 20 additions & 0 deletions src/tools/miri/tests/fail-dep/shims/env-set_var-data-race.stderr
Original file line number Diff line number Diff line change
@@ -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

5 changes: 2 additions & 3 deletions src/tools/miri/tests/pass-dep/shims/env-cleanup-data-race.rs
Original file line number Diff line number Diff line change
@@ -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.
}
}

0 comments on commit 2a1e0ce

Please sign in to comment.