Auto merge of #1502 - RalfJung:isolation, r=RalfJung

fs: move isolation handling to inside trait

Author: bors

src/helpers.rs

@@ -376,13 +376,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     /// case.
     fn check_no_isolation(&self, name: &str) -> InterpResult<'tcx> {
         if !self.eval_context_ref().machine.communicate {
-            throw_machine_stop!(TerminationInfo::UnsupportedInIsolation(format!(
-                "`{}` not available when isolation is enabled",
-                name,
-            )))
+            isolation_error(name)?;
         }
         Ok(())
     }
+
     /// Helper function used inside the shims of foreign functions to assert that the target OS
     /// is `target_os`. It panics showing a message with the `name` of the foreign function
     /// if this is not the case.
@@ -509,6 +507,13 @@ pub fn check_arg_count<'a, 'tcx, const N: usize>(args: &'a [OpTy<'tcx, Tag>]) ->
     throw_ub_format!("incorrect number of arguments: got {}, expected {}", args.len(), N)
 }
 
+pub fn isolation_error(name: &str) -> InterpResult<'static> {
+    throw_machine_stop!(TerminationInfo::UnsupportedInIsolation(format!(
+        "`{}` not available when isolation is enabled",
+        name,
+    )))
+}
+
 pub fn immty_from_int_checked<'tcx>(
     int: impl Into<i128>,
     layout: TyAndLayout<'tcx>,

src/shims/posix/fs.rs

@@ -25,25 +25,28 @@ struct FileHandle {
 trait FileDescriptor<'tcx> : std::fmt::Debug {
     fn as_file_handle(&self) -> InterpResult<'tcx, &FileHandle>;
 
-    fn read(&mut self, bytes: &mut [u8]) -> InterpResult<'tcx, io::Result<usize>>;
-    fn write(&mut self, bytes: &[u8]) -> InterpResult<'tcx, io::Result<usize>>;
-    fn seek(&mut self, offset: SeekFrom) -> InterpResult<'tcx, io::Result<u64>>;
+    fn read(&mut self, communicate_allowed: bool, bytes: &mut [u8]) -> InterpResult<'tcx, io::Result<usize>>;
+    fn write(&mut self, communicate_allowed: bool, bytes: &[u8]) -> InterpResult<'tcx, io::Result<usize>>;
+    fn seek(&mut self, communicate_allowed: bool, offset: SeekFrom) -> InterpResult<'tcx, io::Result<u64>>;
 }
 
 impl<'tcx> FileDescriptor<'tcx> for FileHandle {
     fn as_file_handle(&self) -> InterpResult<'tcx, &FileHandle> {
         Ok(&self)
     }
 
-    fn read(&mut self, bytes: &mut [u8]) -> InterpResult<'tcx, io::Result<usize>> {
+    fn read(&mut self, communicate_allowed: bool, bytes: &mut [u8]) -> InterpResult<'tcx, io::Result<usize>> {
+        assert!(communicate_allowed, "isolation should have prevented even opening a file");
         Ok(self.file.read(bytes))
     }
 
-    fn write(&mut self, bytes: &[u8]) -> InterpResult<'tcx, io::Result<usize>> {
+    fn write(&mut self, communicate_allowed: bool, bytes: &[u8]) -> InterpResult<'tcx, io::Result<usize>> {
+        assert!(communicate_allowed, "isolation should have prevented even opening a file");
         Ok(self.file.write(bytes))
     }
 
-    fn seek(&mut self, offset: SeekFrom) -> InterpResult<'tcx, io::Result<u64>> {
+    fn seek(&mut self, communicate_allowed: bool, offset: SeekFrom) -> InterpResult<'tcx, io::Result<u64>> {
+        assert!(communicate_allowed, "isolation should have prevented even opening a file");
         Ok(self.file.seek(offset))
     }
 }
@@ -53,15 +56,19 @@ impl<'tcx> FileDescriptor<'tcx> for io::Stdin {
         throw_unsup_format!("stdin cannot be used as FileHandle");
     }
 
-    fn read(&mut self, bytes: &mut [u8]) -> InterpResult<'tcx, io::Result<usize>> {
+    fn read(&mut self, communicate_allowed: bool, bytes: &mut [u8]) -> InterpResult<'tcx, io::Result<usize>> {
+        if !communicate_allowed {
+            // We want isolation mode to be deterministic, so we have to disallow all reads, even stdin.
+            helpers::isolation_error("read")?;
+        }
         Ok(Read::read(self, bytes))
     }
 
-    fn write(&mut self, _bytes: &[u8]) -> InterpResult<'tcx, io::Result<usize>> {
+    fn write(&mut self, _communicate_allowed: bool, _bytes: &[u8]) -> InterpResult<'tcx, io::Result<usize>> {
         throw_unsup_format!("cannot write to stdin");
     }
 
-    fn seek(&mut self, _offset: SeekFrom) -> InterpResult<'tcx, io::Result<u64>> {
+    fn seek(&mut self, _communicate_allowed: bool, _offset: SeekFrom) -> InterpResult<'tcx, io::Result<u64>> {
         throw_unsup_format!("cannot seek on stdin");
     }
 }
@@ -71,11 +78,12 @@ impl<'tcx> FileDescriptor<'tcx> for io::Stdout {
         throw_unsup_format!("stdout cannot be used as FileHandle");
     }
 
-    fn read(&mut self, _bytes: &mut [u8]) -> InterpResult<'tcx, io::Result<usize>> {
+    fn read(&mut self, _communicate_allowed: bool, _bytes: &mut [u8]) -> InterpResult<'tcx, io::Result<usize>> {
         throw_unsup_format!("cannot read from stdout");
     }
 
-    fn write(&mut self, bytes: &[u8]) -> InterpResult<'tcx, io::Result<usize>> {
+    fn write(&mut self, _communicate_allowed: bool, bytes: &[u8]) -> InterpResult<'tcx, io::Result<usize>> {
+        // We allow writing to stderr even with isolation enabled.
         let result = Write::write(self, bytes);
         // Stdout is buffered, flush to make sure it appears on the
         // screen.  This is the write() syscall of the interpreted
@@ -87,7 +95,7 @@ impl<'tcx> FileDescriptor<'tcx> for io::Stdout {
         Ok(result)
     }
 
-    fn seek(&mut self, _offset: SeekFrom) -> InterpResult<'tcx, io::Result<u64>> {
+    fn seek(&mut self, _communicate_allowed: bool, _offset: SeekFrom) -> InterpResult<'tcx, io::Result<u64>> {
         throw_unsup_format!("cannot seek on stdout");
     }
 }
@@ -97,15 +105,16 @@ impl<'tcx> FileDescriptor<'tcx> for io::Stderr {
         throw_unsup_format!("stdout cannot be used as FileHandle");
     }
 
-    fn read(&mut self, _bytes: &mut [u8]) -> InterpResult<'tcx, io::Result<usize>> {
+    fn read(&mut self, _communicate_allowed: bool, _bytes: &mut [u8]) -> InterpResult<'tcx, io::Result<usize>> {
         throw_unsup_format!("cannot read from stderr");
     }
 
-    fn write(&mut self, bytes: &[u8]) -> InterpResult<'tcx, io::Result<usize>> {
+    fn write(&mut self, _communicate_allowed: bool, bytes: &[u8]) -> InterpResult<'tcx, io::Result<usize>> {
+        // We allow writing to stderr even with isolation enabled.
         Ok(Write::write(self, bytes))
     }
 
-    fn seek(&mut self, _offset: SeekFrom) -> InterpResult<'tcx, io::Result<u64>> {
+    fn seek(&mut self, _communicate_allowed: bool, _offset: SeekFrom) -> InterpResult<'tcx, io::Result<u64>> {
         throw_unsup_format!("cannot seek on stderr");
     }
 }
@@ -553,7 +562,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     ) -> InterpResult<'tcx, i64> {
         let this = self.eval_context_mut();
 
-        this.check_no_isolation("read")?;
+        // Isolation check is done via `FileDescriptor` trait.
 
         trace!("Reading from FD {}, size {}", fd, count);
 
@@ -577,7 +586,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             // `File::read` never returns a value larger than `count`,
             // so this cannot fail.
             let result = file_descriptor
-                .read(&mut bytes)?
+                .read(this.machine.communicate, &mut bytes)?
                 .map(|c| i64::try_from(c).unwrap());
 
             match result {
@@ -605,9 +614,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     ) -> InterpResult<'tcx, i64> {
         let this = self.eval_context_mut();
 
-        if fd >= 3 {
-            this.check_no_isolation("write")?;
-        }
+        // Isolation check is done via `FileDescriptor` trait.
 
         // Check that the *entire* buffer is actually valid memory.
         this.memory.check_ptr_access(
@@ -623,7 +630,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         if let Some(file_descriptor) = this.machine.file_handler.handles.get_mut(&fd) {
             let bytes = this.memory.read_bytes(buf, Size::from_bytes(count))?;
             let result = file_descriptor
-                .write(&bytes)?
+                .write(this.machine.communicate, &bytes)?
                 .map(|c| i64::try_from(c).unwrap());
             this.try_unwrap_io_result(result)
         } else {
@@ -639,7 +646,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     ) -> InterpResult<'tcx, i64> {
         let this = self.eval_context_mut();
 
-        this.check_no_isolation("lseek64")?;
+        // Isolation check is done via `FileDescriptor` trait.
 
         let fd = this.read_scalar(fd_op)?.to_i32()?;
         let offset = this.read_scalar(offset_op)?.to_i64()?;
@@ -659,7 +666,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         if let Some(file_descriptor) = this.machine.file_handler.handles.get_mut(&fd) {
             let result = file_descriptor
-                .seek(seek_from)?
+                .seek(this.machine.communicate, seek_from)?
                 .map(|offset| i64::try_from(offset).unwrap());
             this.try_unwrap_io_result(result)
         } else {

tests/compile-fail/fs/isolated_stdin.rs

@@ -0,0 +1,13 @@
+// ignore-windows: No libc on Windows
+
+#![feature(rustc_private)]
+
+extern crate libc;
+
+fn main() -> std::io::Result<()> {
+    let mut bytes = [0u8; 512];
+    unsafe {
+        libc::read(0, bytes.as_mut_ptr() as *mut libc::c_void, 512); //~ ERROR `read` not available when isolation is enabled
+    }
+    Ok(())
+}