Auto merge of #1497 - samrat:posix-fs-refactor, r=oli-obk

Add `impl FileDescriptor` for stdin, stdout, stderr

- Use `FileDescriptor::read` for stdin reads
- Use `FileDescriptor::write` for stdout/err writes
- Handle stdout/err reads in `FileDescriptor::read`

The `FileDescriptor` trait was added in PR #1495

Closes #1486

Author: bors

src/shims/posix/foreign_items.rs

@@ -1,6 +1,3 @@
-use std::convert::TryFrom;
-use std::io::{self, Read, Write};
-
 use log::trace;
 
 use rustc_middle::mir;
@@ -67,43 +64,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 let fd = this.read_scalar(fd)?.to_i32()?;
                 let buf = this.read_scalar(buf)?.check_init()?;
                 let count = this.read_scalar(count)?.to_machine_usize(this)?;
-                let result = if fd == 0 {
-
-                    this.check_no_isolation("read")?;
-
-                    // We cap the number of read bytes to the largest
-                    // value that we are able to fit in both the
-                    // host's and target's `isize`. This saves us from
-                    // having to handle overflows later.
-                    let count = count.min(this.machine_isize_max() as u64).min(isize::MAX as u64);
-
-                    // We want to read at most `count` bytes. We are
-                    // sure that `count` is not negative because it
-                    // was a target's `usize`. Also we are sure that
-                    // its smaller than `usize::MAX` because it is a
-                    // host's `isize`.
-                    let mut buffer = vec![0; count as usize];
-                    let res = io::stdin()
-                        .read(&mut buffer)
-                        // `Stdin::read` never returns a value larger
-                        // than `count`, so this cannot fail.
-                        .map(|c| i64::try_from(c).unwrap());
-
-                    match res {
-                        Ok(bytes) => {
-                            this.memory.write_bytes(buf, buffer)?;
-                            i64::try_from(bytes).unwrap()
-                        },
-                        Err(e) => {
-                            this.set_last_error_from_io_error(e)?;
-                            -1
-                        },
-                    }
-                } else if fd == 1 || fd == 2 {
-                    throw_unsup_format!("cannot read from stdout/stderr")
-                } else {
-                    this.read(fd, buf, count)?
-                };
+                let result = this.read(fd, buf, count)?;
                 this.write_scalar(Scalar::from_machine_isize(result, this), dest)?;
             }
             "write" => {
@@ -112,35 +73,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 let buf = this.read_scalar(buf)?.check_init()?;
                 let count = this.read_scalar(n)?.to_machine_usize(this)?;
                 trace!("Called write({:?}, {:?}, {:?})", fd, buf, count);
-                let result = if fd == 0 {
-                    throw_unsup_format!("cannot write to stdin")
-                } else if fd == 1 || fd == 2 {
-                    // stdout/stderr
-
-                    let buf_cont = this.memory.read_bytes(buf, Size::from_bytes(count))?;
-                    // We need to flush to make sure this actually appears on the screen
-                    let res = if fd == 1 {
-                        // Stdout is buffered, flush to make sure it appears on the screen.
-                        // This is the write() syscall of the interpreted program, we want it
-                        // to correspond to a write() syscall on the host -- there is no good
-                        // in adding extra buffering here.
-                        let res = io::stdout().write(buf_cont);
-                        io::stdout().flush().unwrap();
-                        res
-                    } else {
-                        // No need to flush, stderr is not buffered.
-                        io::stderr().write(buf_cont)
-                    };
-                    match res {
-                        Ok(n) => i64::try_from(n).unwrap(),
-                        Err(e) => {
-                            this.set_last_error_from_io_error(e)?;
-                            -1
-                        }
-                    }
-                } else {
-                    this.write(fd, buf, count)?
-                };
+                let result = this.write(fd, buf, count)?;
                 // Now, `result` is the value we return back to the program.
                 this.write_scalar(Scalar::from_machine_isize(result, this), dest)?;
             }

src/shims/posix/fs.rs

@@ -48,11 +48,85 @@ impl<'tcx> FileDescriptor<'tcx> for FileHandle {
     }
 }
 
-#[derive(Debug, Default)]
+impl<'tcx> FileDescriptor<'tcx> for io::Stdin {
+    fn as_file_handle(&self) -> InterpResult<'tcx, &FileHandle> {
+        throw_unsup_format!("stdin cannot be used as FileHandle");
+    }
+
+    fn read(&mut self, bytes: &mut [u8]) -> InterpResult<'tcx, io::Result<usize>> {
+        Ok(Read::read(self, bytes))
+    }
+
+    fn write(&mut self, _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>> {
+        throw_unsup_format!("cannot seek on stdin");
+    }
+}
+
+impl<'tcx> FileDescriptor<'tcx> for io::Stdout {
+    fn as_file_handle(&self) -> InterpResult<'tcx, &FileHandle> {
+        throw_unsup_format!("stdout cannot be used as FileHandle");
+    }
+
+    fn read(&mut self, _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>> {
+        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
+        // program, we want it to correspond to a write() syscall on
+        // the host -- there is no good in adding extra buffering
+        // here.
+        io::stdout().flush().unwrap();
+
+        Ok(result)
+    }
+
+    fn seek(&mut self, _offset: SeekFrom) -> InterpResult<'tcx, io::Result<u64>> {
+        throw_unsup_format!("cannot seek on stdout");
+    }
+}
+
+impl<'tcx> FileDescriptor<'tcx> for io::Stderr {
+    fn as_file_handle(&self) -> InterpResult<'tcx, &FileHandle> {
+        throw_unsup_format!("stdout cannot be used as FileHandle");
+    }
+
+    fn read(&mut self, _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>> {
+        Ok(Write::write(self, bytes))
+    }
+
+    fn seek(&mut self, _offset: SeekFrom) -> InterpResult<'tcx, io::Result<u64>> {
+        throw_unsup_format!("cannot seek on stderr");
+    }
+}
+
+#[derive(Debug)]
 pub struct FileHandler<'tcx> {
     handles: BTreeMap<i32, Box<dyn FileDescriptor<'tcx>>>,
 }
 
+impl<'tcx> Default for FileHandler<'tcx> {
+    fn default() -> Self {
+        let mut handles = BTreeMap::new();
+        handles.insert(0i32, Box::new(io::stdin()) as Box<dyn FileDescriptor<'_>>);
+        handles.insert(1i32, Box::new(io::stdout()) as Box<dyn FileDescriptor<'_>>);
+        handles.insert(2i32, Box::new(io::stderr()) as Box<dyn FileDescriptor<'_>>);
+        FileHandler {
+            handles
+        }
+    }
+}
+
 
 // fd numbers 0, 1, and 2 are reserved for stdin, stdout, and stderr
 const MIN_NORMAL_FILE_FD: i32 = 3;
@@ -485,7 +559,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let this = self.eval_context_mut();
 
         this.check_no_isolation("read")?;
-        assert!(fd >= 3);
 
         trace!("Reading from FD {}, size {}", fd, count);
 
@@ -537,8 +610,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     ) -> InterpResult<'tcx, i64> {
         let this = self.eval_context_mut();
 
-        this.check_no_isolation("write")?;
-        assert!(fd >= 3);
+        if fd >= 3 {
+            this.check_no_isolation("write")?;
+        }
 
         // Check that the *entire* buffer is actually valid memory.
         this.memory.check_ptr_access(

tests/compile-fail/fs/read_from_stdout.rs

@@ -0,0 +1,14 @@
+// compile-flags: -Zmiri-disable-isolation
+// 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(1, bytes.as_mut_ptr() as *mut libc::c_void, 512); //~ ERROR cannot read from stdout
+    }
+    Ok(())
+}

tests/compile-fail/fs/write_to_stdin.rs

@@ -0,0 +1,13 @@
+// ignore-windows: No libc on Windows
+
+#![feature(rustc_private)]
+
+extern crate libc;
+
+fn main() -> std::io::Result<()> {
+    let bytes = b"hello";
+    unsafe {
+        libc::write(0, bytes.as_ptr() as *const libc::c_void, 5); //~ ERROR cannot write to stdin
+    }
+    Ok(())
+}