Auto merge of #983 - christianpoveda:fs-shims-tweaks, r=RalfJung

Various fixes to the file related shims

Hi @RalfJung, I'll be working incrementally over your comments for the new `fs` shims module here.

src/helpers.rs

@@ -291,4 +291,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             }
         }
     }
+
+    /// Helper function to get a `libc` constant as a `Scalar`.
+    fn eval_libc(&mut self, name: &str) -> InterpResult<'tcx, Scalar<Tag>> {
+        self.eval_context_mut()
+            .eval_path_scalar(&["libc", name])?
+            .ok_or_else(|| err_unsup_format!("Path libc::{} cannot be resolved.", name))?
+            .not_undef()
+    }
+
+    /// Helper function to get a `libc` constant as an `i32`.
+    fn eval_libc_i32(&mut self, name: &str) -> InterpResult<'tcx, i32> {
+        self.eval_libc(name)?.to_i32()
+    }
 }

src/lib.rs

@@ -34,7 +34,7 @@ pub use crate::shims::intrinsics::EvalContextExt as IntrinsicsEvalContextExt;
 pub use crate::shims::tls::{EvalContextExt as TlsEvalContextExt, TlsData};
 pub use crate::shims::dlsym::{Dlsym, EvalContextExt as DlsymEvalContextExt};
 pub use crate::shims::env::{EnvVars, EvalContextExt as EnvEvalContextExt};
-pub use crate::shims::io::{FileHandler, EvalContextExt as FileEvalContextExt};
+pub use crate::shims::fs::{FileHandler, EvalContextExt as FileEvalContextExt};
 pub use crate::operator::EvalContextExt as OperatorEvalContextExt;
 pub use crate::range_map::RangeMap;
 pub use crate::helpers::{EvalContextExt as HelpersEvalContextExt};

src/shims/env.rs

@@ -127,17 +127,21 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let tcx = &{ this.tcx.tcx };
 
         let buf = this.force_ptr(this.read_scalar(buf_op)?.not_undef()?)?;
-        let size = this.read_scalar(size_op)?.to_usize(&*this.tcx)?;
+        let size = this.read_scalar(size_op)?.to_usize(&*tcx)?;
         // If we cannot get the current directory, we return null
         match env::current_dir() {
             Ok(cwd) => {
                 // It is not clear what happens with non-utf8 paths here
                 let mut bytes = cwd.display().to_string().into_bytes();
-                // If the buffer is smaller or equal than the path, we return null.
+                // If `size` is smaller or equal than the `bytes.len()`, writing `bytes` plus the
+                // required null terminator to memory using the `buf` pointer would cause an
+                // overflow. The desired behavior in this case is to return null.
                 if (bytes.len() as u64) < size {
                     // We add a `/0` terminator
                     bytes.push(0);
-                    // This is ok because the buffer is larger than the path with the null terminator.
+                    // This is ok because the buffer was strictly larger than `bytes`, so after
+                    // adding the null terminator, the buffer size is larger or equal to
+                    // `bytes.len()`, meaning that `bytes` actually fit inside tbe buffer.
                     this.memory_mut()
                         .get_mut(buf.alloc_id)?
                         .write_bytes(tcx, buf, &bytes)?;
@@ -148,7 +152,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             }
             Err(e) => this.consume_io_error(e)?,
         }
-        Ok(Scalar::ptr_null(&*this.tcx))
+        Ok(Scalar::ptr_null(&*tcx))
     }
 
     fn chdir(&mut self, path_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> {

src/shims/foreign_items.rs

@@ -981,17 +981,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         return Ok(None);
     }
 
-    fn eval_libc(&mut self, name: &str) -> InterpResult<'tcx, Scalar<Tag>> {
-        self.eval_context_mut()
-            .eval_path_scalar(&["libc", name])?
-            .ok_or_else(|| err_unsup_format!("Path libc::{} cannot be resolved.", name).into())
-            .and_then(|scalar| scalar.not_undef())
-    }
-
-    fn eval_libc_i32(&mut self, name: &str) -> InterpResult<'tcx, i32> {
-        self.eval_libc(name).and_then(|scalar| scalar.to_i32())
-    }
-
     fn set_last_error(&mut self, scalar: Scalar<Tag>) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
         let tcx = &{ this.tcx.tcx };

src/shims/fs.rs

@@ -107,6 +107,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             if let Some(FileHandle { flag: old_flag, .. }) =
                 this.machine.file_handler.handles.get_mut(&fd)
             {
+                // Check that the only difference between the old flag and the current flag is
+                // exactly the `FD_CLOEXEC` value.
                 if flag ^ *old_flag == fd_cloexec {
                     *old_flag = flag;
                 } else {

src/shims/mod.rs

@@ -3,7 +3,7 @@ pub mod env;
 pub mod foreign_items;
 pub mod intrinsics;
 pub mod tls;
-pub mod io;
+pub mod fs;
 
 use rustc::{mir, ty};
 

tests/run-pass/change_current_dir.rs

@@ -1,14 +0,0 @@
-// ignore-windows: TODO the windows hook is not done yet
-// compile-flags: -Zmiri-disable-isolation
-use std::env;
-use std::path::Path;
-
-fn main() {
-    // test that `getcwd` is available
-    let cwd = env::current_dir().unwrap();
-    let parent = cwd.parent().unwrap_or(&cwd);
-    // test that `chdir` is available
-    assert!(env::set_current_dir(&Path::new("..")).is_ok());
-    // test that `..` goes to the parent directory
-    assert_eq!(env::current_dir().unwrap(), parent);
-}

tests/run-pass/current_dir.rs

@@ -0,0 +1,17 @@
+// ignore-windows: TODO the windows hook is not done yet
+// compile-flags: -Zmiri-disable-isolation
+use std::env;
+use std::path::Path;
+
+fn main() {
+    // Test that `getcwd` is available
+    let cwd = env::current_dir().unwrap();
+    // Test that changing dir to `..` actually sets the current directory to the parent of `cwd`.
+    // The only exception here is if `cwd` is the root directory, then changing directory must
+    // keep the current directory equal to `cwd`.
+    let parent = cwd.parent().unwrap_or(&cwd);
+    // Test that `chdir` is available
+    assert!(env::set_current_dir(&Path::new("..")).is_ok());
+    // Test that `..` goes to the parent directory
+    assert_eq!(env::current_dir().unwrap(), parent);
+}