Auto merge of rust-lang#2609 - RalfJung:is_terminal, r=RalfJung

Use is_terminal to implement isatty

This means that Linux targets on Windows hosts should give a correct reply. We still don't have an implementation for Windows targets though, that requires some tracking of handles.

Fixes rust-lang/miri#2419

Also restructure our `fs` tests a bit to test the parts that don't need libc separately.

`as_unix_host_fd` is now not used any more, but it could become useful again in the future so I kept it.

Author: bors

Diff for: src/tools/miri/src/lib.rs

@@ -9,6 +9,7 @@
 #![feature(is_some_and)]
 #![feature(nonzero_ops)]
 #![feature(local_key_cell_methods)]
+#![feature(is_terminal)]
 // Configure clippy and other lints
 #![allow(
     clippy::collapsible_else_if,

Diff for: src/tools/miri/src/shims/unix/foreign_items.rs

@@ -452,7 +452,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
             "isatty" => {
                 let [fd] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
                 let result = this.isatty(fd)?;
-                this.write_scalar(Scalar::from_i32(result), dest)?;
+                this.write_scalar(result, dest)?;
             }
             "pthread_atfork" => {
                 let [prepare, parent, child] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;

Diff for: src/tools/miri/src/shims/unix/fs.rs

@@ -4,7 +4,7 @@ use std::convert::TryInto;
 use std::fs::{
     read_dir, remove_dir, remove_file, rename, DirBuilder, File, FileType, OpenOptions, ReadDir,
 };
-use std::io::{self, ErrorKind, Read, Seek, SeekFrom, Write};
+use std::io::{self, ErrorKind, IsTerminal, Read, Seek, SeekFrom, Write};
 use std::path::{Path, PathBuf};
 use std::time::SystemTime;
 
@@ -65,6 +65,8 @@ trait FileDescriptor: std::fmt::Debug {
 
     fn dup(&mut self) -> io::Result<Box<dyn FileDescriptor>>;
 
+    fn is_tty(&self) -> bool;
+
     #[cfg(unix)]
     fn as_unix_host_fd(&self) -> Option<i32> {
         None
@@ -143,6 +145,10 @@ impl FileDescriptor for FileHandle {
         use std::os::unix::io::AsRawFd;
         Some(self.file.as_raw_fd())
     }
+
+    fn is_tty(&self) -> bool {
+        self.file.is_terminal()
+    }
 }
 
 impl FileDescriptor for io::Stdin {
@@ -170,6 +176,10 @@ impl FileDescriptor for io::Stdin {
     fn as_unix_host_fd(&self) -> Option<i32> {
         Some(libc::STDIN_FILENO)
     }
+
+    fn is_tty(&self) -> bool {
+        self.is_terminal()
+    }
 }
 
 impl FileDescriptor for io::Stdout {
@@ -202,6 +212,10 @@ impl FileDescriptor for io::Stdout {
     fn as_unix_host_fd(&self) -> Option<i32> {
         Some(libc::STDOUT_FILENO)
     }
+
+    fn is_tty(&self) -> bool {
+        self.is_terminal()
+    }
 }
 
 impl FileDescriptor for io::Stderr {
@@ -227,12 +241,16 @@ impl FileDescriptor for io::Stderr {
     fn as_unix_host_fd(&self) -> Option<i32> {
         Some(libc::STDERR_FILENO)
     }
+
+    fn is_tty(&self) -> bool {
+        self.is_terminal()
+    }
 }
 
 #[derive(Debug)]
-struct DummyOutput;
+struct NullOutput;
 
-impl FileDescriptor for DummyOutput {
+impl FileDescriptor for NullOutput {
     fn name(&self) -> &'static str {
         "stderr and stdout"
     }
@@ -247,7 +265,11 @@ impl FileDescriptor for DummyOutput {
     }
 
     fn dup(&mut self) -> io::Result<Box<dyn FileDescriptor>> {
-        Ok(Box::new(DummyOutput))
+        Ok(Box::new(NullOutput))
+    }
+
+    fn is_tty(&self) -> bool {
+        false
     }
 }
 
@@ -267,8 +289,8 @@ impl FileHandler {
         let mut handles: BTreeMap<_, Box<dyn FileDescriptor>> = BTreeMap::new();
         handles.insert(0i32, Box::new(io::stdin()));
         if mute_stdout_stderr {
-            handles.insert(1i32, Box::new(DummyOutput));
-            handles.insert(2i32, Box::new(DummyOutput));
+            handles.insert(1i32, Box::new(NullOutput));
+            handles.insert(2i32, Box::new(NullOutput));
         } else {
             handles.insert(1i32, Box::new(io::stdout()));
             handles.insert(2i32, Box::new(io::stderr()));
@@ -1662,35 +1684,23 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
     }
 
     #[cfg_attr(not(unix), allow(unused))]
-    fn isatty(&mut self, miri_fd: &OpTy<'tcx, Provenance>) -> InterpResult<'tcx, i32> {
+    fn isatty(
+        &mut self,
+        miri_fd: &OpTy<'tcx, Provenance>,
+    ) -> InterpResult<'tcx, Scalar<Provenance>> {
         let this = self.eval_context_mut();
-        #[cfg(unix)]
+        // "returns 1 if fd is an open file descriptor referring to a terminal;
+        // otherwise 0 is returned, and errno is set to indicate the error"
         if matches!(this.machine.isolated_op, IsolatedOp::Allow) {
-            let miri_fd = this.read_scalar(miri_fd)?.to_i32()?;
-            if let Some(host_fd) =
-                this.machine.file_handler.handles.get(&miri_fd).and_then(|fd| fd.as_unix_host_fd())
-            {
-                // "returns 1 if fd is an open file descriptor referring to a terminal;
-                // otherwise 0 is returned, and errno is set to indicate the error"
-                // SAFETY: isatty has no preconditions
-                let is_tty = unsafe { libc::isatty(host_fd) };
-                if is_tty == 0 {
-                    let errno = std::io::Error::last_os_error()
-                        .raw_os_error()
-                        .map(Scalar::from_i32)
-                        .unwrap();
-                    this.set_last_error(errno)?;
-                }
-                return Ok(is_tty);
+            let fd = this.read_scalar(miri_fd)?.to_i32()?;
+            if this.machine.file_handler.handles.get(&fd).map(|fd| fd.is_tty()) == Some(true) {
+                return Ok(Scalar::from_i32(1));
             }
         }
-        // We are attemping to use a Unix interface on a non-Unix platform, or we are on a Unix
-        // platform and the passed file descriptor is not open, or isolation is enabled
-        // FIXME: It should be possible to emulate this at least on Windows by using
-        // GetConsoleMode.
+        // Fallback when the FD was not found or isolation is enabled.
         let enotty = this.eval_libc("ENOTTY")?;
         this.set_last_error(enotty)?;
-        Ok(0)
+        Ok(Scalar::from_i32(0))
     }
 
     fn realpath(

Diff for: src/tools/miri/tests/pass-dep/shims/libc-fs-with-isolation.rs

@@ -0,0 +1,28 @@
+//@ignore-target-windows: no libc on Windows
+//@compile-flags: -Zmiri-isolation-error=warn-nobacktrace
+//@normalize-stderr-test: "(stat(x)?)" -> "$$STAT"
+
+use std::ffi::CString;
+use std::fs;
+use std::io::{Error, ErrorKind};
+
+fn main() {
+    // test `fcntl`
+    unsafe {
+        assert_eq!(libc::fcntl(1, libc::F_DUPFD, 0), -1);
+        assert_eq!(Error::last_os_error().raw_os_error(), Some(libc::EPERM));
+    }
+
+    // test `readlink`
+    let symlink_c_str = CString::new("foo.txt").unwrap();
+    let mut buf = vec![0; "foo_link.txt".len() + 1];
+    unsafe {
+        assert_eq!(libc::readlink(symlink_c_str.as_ptr(), buf.as_mut_ptr(), buf.len()), -1);
+        assert_eq!(Error::last_os_error().raw_os_error(), Some(libc::EACCES));
+    }
+
+    // test `stat`
+    assert_eq!(fs::metadata("foo.txt").unwrap_err().kind(), ErrorKind::PermissionDenied);
+    // check that it is the right kind of `PermissionDenied`
+    assert_eq!(Error::last_os_error().raw_os_error(), Some(libc::EACCES));
+}

Diff for: src/tools/miri/tests/pass-dep/shims/libc-fs-with-isolation.stderr

@@ -0,0 +1,6 @@
+warning: `fcntl` was made to return an error due to isolation
+
+warning: `readlink` was made to return an error due to isolation
+
+warning: `$STAT` was made to return an error due to isolation
+

Diff for: src/tools/miri/tests/pass-dep/shims/libc-fs.rs

@@ -0,0 +1,137 @@
+//@ignore-target-windows: no libc on Windows
+//@compile-flags: -Zmiri-disable-isolation
+
+#![feature(io_error_more)]
+#![feature(io_error_uncategorized)]
+
+use std::convert::TryInto;
+use std::ffi::CString;
+use std::fs::{canonicalize, remove_file, File};
+use std::io::{Error, ErrorKind, Write};
+use std::os::unix::ffi::OsStrExt;
+use std::path::PathBuf;
+
+fn main() {
+    test_dup_stdout_stderr();
+    test_canonicalize_too_long();
+    test_readlink();
+    test_file_open_unix_allow_two_args();
+    test_file_open_unix_needs_three_args();
+    test_file_open_unix_extra_third_arg();
+}
+
+fn tmp() -> PathBuf {
+    std::env::var("MIRI_TEMP")
+        .map(|tmp| {
+            // MIRI_TEMP is set outside of our emulated
+            // program, so it may have path separators that don't
+            // correspond to our target platform. We normalize them here
+            // before constructing a `PathBuf`
+
+            #[cfg(windows)]
+            return PathBuf::from(tmp.replace("/", "\\"));
+
+            #[cfg(not(windows))]
+            return PathBuf::from(tmp.replace("\\", "/"));
+        })
+        .unwrap_or_else(|_| std::env::temp_dir())
+}
+
+/// Prepare: compute filename and make sure the file does not exist.
+fn prepare(filename: &str) -> PathBuf {
+    let path = tmp().join(filename);
+    // Clean the paths for robustness.
+    remove_file(&path).ok();
+    path
+}
+
+/// Prepare like above, and also write some initial content to the file.
+fn prepare_with_content(filename: &str, content: &[u8]) -> PathBuf {
+    let path = prepare(filename);
+    let mut file = File::create(&path).unwrap();
+    file.write(content).unwrap();
+    path
+}
+
+fn test_file_open_unix_allow_two_args() {
+    let path = prepare_with_content("test_file_open_unix_allow_two_args.txt", &[]);
+
+    let mut name = path.into_os_string();
+    name.push("\0");
+    let name_ptr = name.as_bytes().as_ptr().cast::<libc::c_char>();
+    let _fd = unsafe { libc::open(name_ptr, libc::O_RDONLY) };
+}
+
+fn test_file_open_unix_needs_three_args() {
+    let path = prepare_with_content("test_file_open_unix_needs_three_args.txt", &[]);
+
+    let mut name = path.into_os_string();
+    name.push("\0");
+    let name_ptr = name.as_bytes().as_ptr().cast::<libc::c_char>();
+    let _fd = unsafe { libc::open(name_ptr, libc::O_CREAT, 0o666) };
+}
+
+fn test_file_open_unix_extra_third_arg() {
+    let path = prepare_with_content("test_file_open_unix_extra_third_arg.txt", &[]);
+
+    let mut name = path.into_os_string();
+    name.push("\0");
+    let name_ptr = name.as_bytes().as_ptr().cast::<libc::c_char>();
+    let _fd = unsafe { libc::open(name_ptr, libc::O_RDONLY, 42) };
+}
+
+fn test_dup_stdout_stderr() {
+    let bytes = b"hello dup fd\n";
+    unsafe {
+        let new_stdout = libc::fcntl(1, libc::F_DUPFD, 0);
+        let new_stderr = libc::fcntl(2, libc::F_DUPFD, 0);
+        libc::write(new_stdout, bytes.as_ptr() as *const libc::c_void, bytes.len());
+        libc::write(new_stderr, bytes.as_ptr() as *const libc::c_void, bytes.len());
+    }
+}
+
+fn test_canonicalize_too_long() {
+    // Make sure we get an error for long paths.
+    let too_long = "x/".repeat(libc::PATH_MAX.try_into().unwrap());
+    assert!(canonicalize(too_long).is_err());
+}
+
+fn test_readlink() {
+    let bytes = b"Hello, World!\n";
+    let path = prepare_with_content("miri_test_fs_link_target.txt", bytes);
+    let expected_path = path.as_os_str().as_bytes();
+
+    let symlink_path = prepare("miri_test_fs_symlink.txt");
+    std::os::unix::fs::symlink(&path, &symlink_path).unwrap();
+
+    // Test that the expected string gets written to a buffer of proper
+    // length, and that a trailing null byte is not written.
+    let symlink_c_str = CString::new(symlink_path.as_os_str().as_bytes()).unwrap();
+    let symlink_c_ptr = symlink_c_str.as_ptr();
+
+    // Make the buf one byte larger than it needs to be,
+    // and check that the last byte is not overwritten.
+    let mut large_buf = vec![0xFF; expected_path.len() + 1];
+    let res =
+        unsafe { libc::readlink(symlink_c_ptr, large_buf.as_mut_ptr().cast(), large_buf.len()) };
+    // Check that the resovled path was properly written into the buf.
+    assert_eq!(&large_buf[..(large_buf.len() - 1)], expected_path);
+    assert_eq!(large_buf.last(), Some(&0xFF));
+    assert_eq!(res, large_buf.len() as isize - 1);
+
+    // Test that the resolved path is truncated if the provided buffer
+    // is too small.
+    let mut small_buf = [0u8; 2];
+    let res =
+        unsafe { libc::readlink(symlink_c_ptr, small_buf.as_mut_ptr().cast(), small_buf.len()) };
+    assert_eq!(small_buf, &expected_path[..small_buf.len()]);
+    assert_eq!(res, small_buf.len() as isize);
+
+    // Test that we report a proper error for a missing path.
+    let bad_path = CString::new("MIRI_MISSING_FILE_NAME").unwrap();
+    let res = unsafe {
+        libc::readlink(bad_path.as_ptr(), small_buf.as_mut_ptr().cast(), small_buf.len())
+    };
+    assert_eq!(res, -1);
+    assert_eq!(Error::last_os_error().kind(), ErrorKind::NotFound);
+}

Diff for: src/tools/miri/tests/pass-dep/shims/fs.stderr renamed to src/tools/miri/tests/pass-dep/shims/libc-fs.stderr

@@ -0,0 +1 @@
+hello dup fd

Diff for: src/tools/miri/tests/pass-dep/shims/fs.stdout renamed to src/tools/miri/tests/pass-dep/shims/libc-fs.stdout

@@ -2,21 +2,14 @@
 //@compile-flags: -Zmiri-isolation-error=warn-nobacktrace
 //@normalize-stderr-test: "(stat(x)?)" -> "$$STAT"
 
-use std::ffi::CString;
 use std::fs::{self, File};
-use std::io::{Error, ErrorKind};
+use std::io::ErrorKind;
 use std::os::unix;
 
 fn main() {
     // test `open`
     assert_eq!(File::create("foo.txt").unwrap_err().kind(), ErrorKind::PermissionDenied);
 
-    // test `fcntl`
-    unsafe {
-        assert_eq!(libc::fcntl(1, libc::F_DUPFD, 0), -1);
-        assert_eq!(Error::last_os_error().raw_os_error(), Some(libc::EPERM));
-    }
-
     // test `unlink`
     assert_eq!(fs::remove_file("foo.txt").unwrap_err().kind(), ErrorKind::PermissionDenied);
 
@@ -26,17 +19,8 @@ fn main() {
         ErrorKind::PermissionDenied
     );
 
-    // test `readlink`
-    let symlink_c_str = CString::new("foo.txt").unwrap();
-    let mut buf = vec![0; "foo_link.txt".len() + 1];
-    unsafe {
-        assert_eq!(libc::readlink(symlink_c_str.as_ptr(), buf.as_mut_ptr(), buf.len()), -1);
-        assert_eq!(Error::last_os_error().raw_os_error(), Some(libc::EACCES));
-    }
-
     // test `stat`
     assert_eq!(fs::metadata("foo.txt").unwrap_err().kind(), ErrorKind::PermissionDenied);
-    assert_eq!(Error::last_os_error().raw_os_error(), Some(libc::EACCES));
 
     // test `rename`
     assert_eq!(fs::rename("a.txt", "b.txt").unwrap_err().kind(), ErrorKind::PermissionDenied);
@@ -49,5 +33,4 @@ fn main() {
 
     // test `opendir`
     assert_eq!(fs::read_dir("foo/bar").unwrap_err().kind(), ErrorKind::PermissionDenied);
-    assert_eq!(Error::last_os_error().raw_os_error(), Some(libc::EACCES));
 }

Diff for: src/tools/miri/tests/pass-dep/shims/libc-rsfs.stdout

@@ -1,13 +1,9 @@
 warning: `open` was made to return an error due to isolation
 
-warning: `fcntl` was made to return an error due to isolation
-
 warning: `unlink` was made to return an error due to isolation
 
 warning: `symlink` was made to return an error due to isolation
 
-warning: `readlink` was made to return an error due to isolation
-
 warning: `$STAT` was made to return an error due to isolation
 
 warning: `rename` was made to return an error due to isolation