Auto merge of #1208 - christianpoveda:environ-shim, r=RalfJung

Environ shim

Remake of #1147. There are three main problems with this:

1. For some reason `update_environ` is not updating `environ` when `setenv` or `unsetenv` are called. Even then it works during initialization.
2. I am not deallocating the old array with the variables in `update_environ`.
3. I had to store the `environ` place into `MemoryExtra` as a field to update it. I was thinking about changing `extern_statics` to store places instead of `AllocID`s to avoid this.

@RalfJung

Author: bors

src/eval.rs

@@ -77,8 +77,8 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
         ),
     );
     // Complete initialization.
+    EnvVars::init(&mut ecx, config.excluded_env_vars)?;
     MemoryExtra::init_extern_statics(&mut ecx)?;
-    EnvVars::init(&mut ecx, config.excluded_env_vars);
 
     // Setup first stack-frame
     let main_instance = ty::Instance::mono(tcx, main_id);

src/machine.rs

@@ -70,7 +70,7 @@ pub struct AllocExtra {
 
 /// Extra global memory data
 #[derive(Clone, Debug)]
-pub struct MemoryExtra {
+pub struct MemoryExtra<'tcx> {
     pub stacked_borrows: Option<stacked_borrows::MemoryExtra>,
     pub intptrcast: intptrcast::MemoryExtra,
 
@@ -84,9 +84,12 @@ pub struct MemoryExtra {
     /// An allocation ID to report when it is being allocated
     /// (helps for debugging memory leaks).
     tracked_alloc_id: Option<AllocId>,
+
+    /// Place where the `environ` static is stored. Lazily initialized, but then never changes.
+    pub(crate) environ: Option<MPlaceTy<'tcx, Tag>>,
 }
 
-impl MemoryExtra {
+impl<'tcx> MemoryExtra<'tcx> {
     pub fn new(rng: StdRng, stacked_borrows: bool, tracked_pointer_tag: Option<PtrId>, tracked_alloc_id: Option<AllocId>) -> Self {
         let stacked_borrows = if stacked_borrows {
             Some(Rc::new(RefCell::new(stacked_borrows::GlobalState::new(tracked_pointer_tag))))
@@ -99,14 +102,16 @@ impl MemoryExtra {
             extern_statics: FxHashMap::default(),
             rng: RefCell::new(rng),
             tracked_alloc_id,
+            environ: None,
         }
     }
 
     /// Sets up the "extern statics" for this machine.
-    pub fn init_extern_statics<'mir, 'tcx>(
+    pub fn init_extern_statics<'mir>(
         this: &mut MiriEvalContext<'mir, 'tcx>,
     ) -> InterpResult<'tcx> {
-        match this.tcx.sess.target.target.target_os.as_str() {
+        let target_os = this.tcx.sess.target.target.target_os.as_str();
+        match target_os {
             "linux" => {
                 // "__cxa_thread_atexit_impl"
                 // This should be all-zero, pointer-sized.
@@ -118,6 +123,12 @@ impl MemoryExtra {
                     .extern_statics
                     .insert(Symbol::intern("__cxa_thread_atexit_impl"), place.ptr.assert_ptr().alloc_id)
                     .unwrap_none();
+                // "environ"
+                this.memory
+                    .extra
+                    .extern_statics
+                    .insert(Symbol::intern("environ"), this.memory.extra.environ.unwrap().ptr.assert_ptr().alloc_id)
+                    .unwrap_none();
             }
             _ => {} // No "extern statics" supported on this platform
         }
@@ -203,7 +214,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
     type MemoryKinds = MiriMemoryKind;
 
     type FrameExtra = FrameData<'tcx>;
-    type MemoryExtra = MemoryExtra;
+    type MemoryExtra = MemoryExtra<'tcx>;
     type AllocExtra = AllocExtra;
     type PointerTag = Tag;
     type ExtraFnVal = Dlsym;
@@ -329,7 +340,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
     }
 
     fn init_allocation_extra<'b>(
-        memory_extra: &MemoryExtra,
+        memory_extra: &MemoryExtra<'tcx>,
         id: AllocId,
         alloc: Cow<'b, Allocation>,
         kind: Option<MemoryKind<Self::MemoryKinds>>,
@@ -366,7 +377,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
     }
 
     #[inline(always)]
-    fn tag_static_base_pointer(memory_extra: &MemoryExtra, id: AllocId) -> Self::PointerTag {
+    fn tag_static_base_pointer(memory_extra: &MemoryExtra<'tcx>, id: AllocId) -> Self::PointerTag {
         if let Some(stacked_borrows) = memory_extra.stacked_borrows.as_ref() {
             stacked_borrows.borrow_mut().static_base_ptr(id)
         } else {

src/shims/env.rs

@@ -2,6 +2,7 @@ use std::ffi::{OsString, OsStr};
 use std::env;
 
 use crate::stacked_borrows::Tag;
+use crate::rustc_target::abi::LayoutOf;
 use crate::*;
 
 use rustc_data_structures::fx::FxHashMap;
@@ -19,7 +20,7 @@ impl EnvVars {
     pub(crate) fn init<'mir, 'tcx>(
         ecx: &mut InterpCx<'mir, 'tcx, Evaluator<'tcx>>,
         excluded_env_vars: Vec<String>,
-    ) {
+    ) -> InterpResult<'tcx> {
         if ecx.machine.communicate {
             for (name, value) in env::vars() {
                 if !excluded_env_vars.contains(&name) {
@@ -29,6 +30,12 @@ impl EnvVars {
                 }
             }
         }
+        // Initialize the `environ` static
+        let layout = ecx.layout_of(ecx.tcx.types.usize)?;
+        let place = ecx.allocate(layout, MiriMemoryKind::Machine.into());
+        ecx.write_scalar(Scalar::from_machine_usize(0, &*ecx.tcx), place.into())?;
+        ecx.memory.extra.environ = Some(place);
+        ecx.update_environ()
     }
 }
 
@@ -82,6 +89,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 this.memory
                     .deallocate(var, None, MiriMemoryKind::Machine.into())?;
             }
+            this.update_environ()?;
             Ok(0)
         } else {
             Ok(-1)
@@ -104,6 +112,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 this.memory
                     .deallocate(var, None, MiriMemoryKind::Machine.into())?;
             }
+            this.update_environ()?;
             Ok(0)
         } else {
             Ok(-1)
@@ -150,4 +159,36 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             }
         }
     }
+
+    /// Updates the `environ` static. It should not be called before
+    /// `EnvVars::init`.
+    fn update_environ(&mut self) -> InterpResult<'tcx> {
+        let this = self.eval_context_mut();
+        // Deallocate the old environ value.
+        let old_vars_ptr = this.read_scalar(this.memory.extra.environ.unwrap().into())?.not_undef()?;
+        // The pointer itself can be null because `EnvVars::init` only
+        // initializes the place for the static but not the static itself.
+        if !this.is_null(old_vars_ptr)? {
+            this.memory.deallocate(this.force_ptr(old_vars_ptr)?, None, MiriMemoryKind::Machine.into())?;
+        }
+        // Collect all the pointers to each variable in a vector.
+        let mut vars: Vec<Scalar<Tag>> = this.machine.env_vars.map.values().map(|&ptr| ptr.into()).collect();
+        // Add the trailing null pointer.
+        vars.push(Scalar::from_int(0, this.pointer_size()));
+        // Make an array with all these pointers inside Miri.
+        let tcx = this.tcx;
+        let vars_layout =
+            this.layout_of(tcx.mk_array(tcx.types.usize, vars.len() as u64))?;
+        let vars_place = this.allocate(vars_layout, MiriMemoryKind::Machine.into());
+        for (idx, var) in vars.into_iter().enumerate() {
+            let place = this.mplace_field(vars_place, idx as u64)?;
+            this.write_scalar(var, place.into())?;
+        }
+        this.write_scalar(
+            vars_place.ptr,
+            this.memory.extra.environ.unwrap().into(),
+        )?;
+
+        Ok(())
+    }
 }

src/shims/foreign_items/posix/macos.rs

@@ -56,6 +56,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?;
             }
 
+            // Environment related shims
+            "_NSGetEnviron" => {
+                this.write_scalar(this.memory.extra.environ.unwrap().ptr, dest)?;
+            }
+
             // Time related shims
             "gettimeofday" => {
                 let result = this.gettimeofday(args[0], args[1])?;

tests/compile-fail/environ-gets-deallocated.rs

@@ -0,0 +1,24 @@
+//ignore-windows: TODO env var emulation stubbed out on Windows
+
+#[cfg(target_os="linux")]
+fn get_environ() -> *const *const u8 {
+  extern "C" {
+    static mut environ: *const *const u8;
+  }
+  unsafe { environ }
+}
+
+#[cfg(target_os="macos")]
+fn get_environ() -> *const *const u8 {
+    extern "C" {
+        fn _NSGetEnviron() -> *mut *const *const u8;
+    }
+    unsafe { *_NSGetEnviron() }
+}
+
+fn main() {
+    let pointer = get_environ();
+    let _x = unsafe { *pointer };
+    std::env::set_var("FOO", "BAR");
+    let _y = unsafe { *pointer }; //~ ERROR dangling pointer was dereferenced
+}

tests/run-pass/env.rs

@@ -3,9 +3,26 @@
 use std::env;
 
 fn main() {
+    // Test that miri environment is isolated when communication is disabled.
+    // (`MIRI_ENV_VAR_TEST` is set by the test harness.)
+    assert_eq!(env::var("MIRI_ENV_VAR_TEST"), Err(env::VarError::NotPresent));
+
+    // Test base state.
+    println!("{:#?}", env::vars().collect::<Vec<_>>());
     assert_eq!(env::var("MIRI_TEST"), Err(env::VarError::NotPresent));
+
+    // Set the variable.
     env::set_var("MIRI_TEST", "the answer");
     assert_eq!(env::var("MIRI_TEST"), Ok("the answer".to_owned()));
-    // Test that miri environment is isolated when communication is disabled.
-    assert!(env::var("MIRI_ENV_VAR_TEST").is_err());
+    println!("{:#?}", env::vars().collect::<Vec<_>>());
+
+    // Change the variable.
+    env::set_var("MIRI_TEST", "42");
+    assert_eq!(env::var("MIRI_TEST"), Ok("42".to_owned()));
+    println!("{:#?}", env::vars().collect::<Vec<_>>());
+
+    // Remove the variable.
+    env::remove_var("MIRI_TEST");
+    assert_eq!(env::var("MIRI_TEST"), Err(env::VarError::NotPresent));
+    println!("{:#?}", env::vars().collect::<Vec<_>>());
 }

tests/run-pass/env.stdout

@@ -0,0 +1,14 @@
+[]
+[
+    (
+        "MIRI_TEST",
+        "the answer",
+    ),
+]
+[
+    (
+        "MIRI_TEST",
+        "42",
+    ),
+]
+[]