Auto merge of #2787 - DebugSteven:move-fcntl, r=oli-obk

move reject with isolation logic in fcntl

This PR moves the block of logic inside `fcntl` to reject if isolation is enabled into the branch checking if the command is `F_FULLFSYNC` on apple. This allows `fcntl` to duplicate file descriptors while using isolation.

This means we can now run the tokio tests with isolation.

This PR allows moves the now passing `fcntl` logic from `libc-fs-with-isolation.rs` into two different tests:
- `fcntl(1, libc::F_DUPFD, 0)` succeeds with isolation
- `fcntl(1, libc::F_FULLFSYNC, 0)` fails with isolation on MacOS

7 files changed, 26 insertions, 17 deletions

diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs
index 1eca389e984..de271548217 100644
--- a/src/tools/miri/src/shims/unix/fs.rs
+++ b/src/tools/miri/src/shims/unix/fs.rs
@@ -628,13 +628,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let fd = this.read_scalar(&args[0])?.to_i32()?;
         let cmd = this.read_scalar(&args[1])?.to_i32()?;
 
-        // Reject if isolation is enabled.
-        if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
-            this.reject_in_isolation("`fcntl`", reject_with)?;
-            this.set_last_error_from_io_error(ErrorKind::PermissionDenied)?;
-            return Ok(-1);
-        }
-
         // We only support getting the flags for a descriptor.
         if cmd == this.eval_libc_i32("F_GETFD") {
             // Currently this is the only flag that `F_GETFD` returns. It is OK to just return the
@@ -677,6 +670,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 None => this.handle_not_found(),
             }
         } else if this.tcx.sess.target.os == "macos" && cmd == this.eval_libc_i32("F_FULLFSYNC") {
+            // Reject if isolation is enabled.
+            if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
+                this.reject_in_isolation("`fcntl`", reject_with)?;
+                this.set_last_error_from_io_error(ErrorKind::PermissionDenied)?;
+                return Ok(-1);
+            }
+
             if let Some(file_descriptor) = this.machine.file_handler.handles.get(&fd) {
                 // FIXME: Support fullfsync for all FDs
                 let FileHandle { file, writable } =
diff --git a/src/tools/miri/tests/pass-dep/shims/fcntl_f-fullfsync_apple.rs b/src/tools/miri/tests/pass-dep/shims/fcntl_f-fullfsync_apple.rs
new file mode 100644
index 00000000000..307906f2583
--- /dev/null
+++ b/src/tools/miri/tests/pass-dep/shims/fcntl_f-fullfsync_apple.rs
@@ -0,0 +1,12 @@
+//@only-target-apple: F_FULLFSYNC only on apple systems
+//@compile-flags: -Zmiri-isolation-error=warn-nobacktrace
+
+use std::io::Error;
+
+fn main() {
+    // test `fcntl(F_FULLFSYNC)`
+    unsafe {
+        assert_eq!(libc::fcntl(1, libc::F_FULLFSYNC, 0), -1);
+        assert_eq!(Error::last_os_error().raw_os_error(), Some(libc::EPERM));
+    }
+}
diff --git a/src/tools/miri/tests/pass-dep/shims/fcntl_f-fullfsync_apple.stderr b/src/tools/miri/tests/pass-dep/shims/fcntl_f-fullfsync_apple.stderr
new file mode 100644
index 00000000000..09a24e1e5d7
--- /dev/null
+++ b/src/tools/miri/tests/pass-dep/shims/fcntl_f-fullfsync_apple.stderr
@@ -0,0 +1,2 @@
+warning: `fcntl` was made to return an error due to isolation
+
diff --git a/src/tools/miri/tests/pass-dep/shims/libc-fs-with-isolation.rs b/src/tools/miri/tests/pass-dep/shims/libc-fs-with-isolation.rs
index d6d19a3fe81..5185db0b0e2 100644
--- a/src/tools/miri/tests/pass-dep/shims/libc-fs-with-isolation.rs
+++ b/src/tools/miri/tests/pass-dep/shims/libc-fs-with-isolation.rs
@@ -7,10 +7,9 @@ use std::fs;
 use std::io::{Error, ErrorKind};
 
 fn main() {
-    // test `fcntl`
+    // test `fcntl(F_DUPFD): should work even with isolation.`
     unsafe {
-        assert_eq!(libc::fcntl(1, libc::F_DUPFD, 0), -1);
-        assert_eq!(Error::last_os_error().raw_os_error(), Some(libc::EPERM));
+        assert!(libc::fcntl(1, libc::F_DUPFD, 0) >= 0);
     }
 
     // test `readlink`
diff --git a/src/tools/miri/tests/pass-dep/shims/libc-fs-with-isolation.stderr b/src/tools/miri/tests/pass-dep/shims/libc-fs-with-isolation.stderr
index 21fcb65243e..b0cadfb970b 100644
--- a/src/tools/miri/tests/pass-dep/shims/libc-fs-with-isolation.stderr
+++ b/src/tools/miri/tests/pass-dep/shims/libc-fs-with-isolation.stderr
@@ -1,5 +1,3 @@
-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 --git a/src/tools/miri/tests/pass-dep/tokio/sleep.rs b/src/tools/miri/tests/pass-dep/tokio/sleep.rs
index 1341484dda4..00cc68eba3e 100644
--- a/src/tools/miri/tests/pass-dep/tokio/sleep.rs
+++ b/src/tools/miri/tests/pass-dep/tokio/sleep.rs
@@ -1,4 +1,4 @@
-//@compile-flags: -Zmiri-disable-isolation -Zmiri-permissive-provenance -Zmiri-backtrace=full
+//@compile-flags: -Zmiri-permissive-provenance -Zmiri-backtrace=full
 //@only-target-x86_64-unknown-linux: support for tokio only on linux and x86
 
 use tokio::time::{sleep, Duration, Instant};
@@ -7,8 +7,6 @@ use tokio::time::{sleep, Duration, Instant};
 async fn main() {
     let start = Instant::now();
     sleep(Duration::from_secs(1)).await;
-    // It takes 96 millisecond to sleep for 1 millisecond
-    // It takes 1025 millisecond to sleep for 1 second
     let time_elapsed = &start.elapsed().as_millis();
-    assert!(time_elapsed > &1000, "{}", time_elapsed);
+    assert!((1000..1100).contains(time_elapsed), "{}", time_elapsed);
 }
diff --git a/src/tools/miri/tests/pass-dep/tokio/tokio_mvp.rs b/src/tools/miri/tests/pass-dep/tokio/tokio_mvp.rs
index 0bca7cc069a..0ed2a941bc4 100644
--- a/src/tools/miri/tests/pass-dep/tokio/tokio_mvp.rs
+++ b/src/tools/miri/tests/pass-dep/tokio/tokio_mvp.rs
@@ -1,5 +1,5 @@
 // Need to disable preemption to stay on the supported MVP codepath in mio.
-//@compile-flags: -Zmiri-disable-isolation -Zmiri-permissive-provenance
+//@compile-flags: -Zmiri-permissive-provenance
 //@only-target-x86_64-unknown-linux: support for tokio exists only on linux and x86
 
 #[tokio::main]