Review comments

Author: divergentdave

src/shims/foreign_items/posix.rs

@@ -137,11 +137,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 this.write_scalar(Scalar::from_i64(result), dest)?;
             }
             "fsync" => {
-                let result = this.fsync(args[0])?;
+                let &[fd] = check_arg_count(args)?;
+                let result = this.fsync(fd)?;
                 this.write_scalar(Scalar::from_i32(result), dest)?;
             }
             "fdatasync" => {
-                let result = this.fdatasync(args[0])?;
+                let &[fd] = check_arg_count(args)?;
+                let result = this.fdatasync(fd)?;
                 this.write_scalar(Scalar::from_i32(result), dest)?;
             }
 

src/shims/foreign_items/posix/linux.rs

@@ -54,9 +54,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 // fadvise is only informational, we can ignore it.
                 this.write_null(dest)?;
             }
-            // Linux-only
             "sync_file_range" => {
-                let result = this.sync_file_range(args[0], args[1], args[2], args[3])?;
+                let &[fd, offset, nbytes, flags] = check_arg_count(args)?;
+                let result = this.sync_file_range(fd, offset, nbytes, flags)?;
                 this.write_scalar(Scalar::from_i32(result), dest)?;
             }
 

src/shims/fs.rs

@@ -378,6 +378,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         } else if this.tcx.sess.target.target.target_os == "macos"
             && cmd == this.eval_libc_i32("F_FULLFSYNC")?
         {
+            // On macOS, fsync does not wait for the underlying disk to finish writing, while this
+            // F_FULLFSYNC operation does. The standard library uses F_FULLFSYNC for both
+            // File::sync_data() and File::sync_all().
+            let &[_, _] = check_arg_count(args)?;
             if let Some(FileHandle { file, writable: _ }) = this.machine.file_handler.handles.get_mut(&fd) {
                 let result = file.sync_all();
                 this.try_unwrap_io_result(result.map(|_| 0i32))
@@ -1153,9 +1157,24 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         this.check_no_isolation("sync_file_range")?;
 
         let fd = this.read_scalar(fd_op)?.to_i32()?;
-        let _offset = this.read_scalar(offset_op)?.to_i64()?;
-        let _nbytes = this.read_scalar(nbytes_op)?.to_i64()?;
-        let _flags = this.read_scalar(flags_op)?.to_u32()?;
+        let offset = this.read_scalar(offset_op)?.to_i64()?;
+        let nbytes = this.read_scalar(nbytes_op)?.to_i64()?;
+        let flags = this.read_scalar(flags_op)?.to_i32()?;
+
+        if offset < 0 || nbytes < 0 {
+            let einval = this.eval_libc("EINVAL")?;
+            this.set_last_error(einval)?;
+            return Ok(-1);
+        }
+        let allowed_flags = this.eval_libc_i32("SYNC_FILE_RANGE_WAIT_BEFORE")?
+            | this.eval_libc_i32("SYNC_FILE_RANGE_WRITE")?
+            | this.eval_libc_i32("SYNC_FILE_RANGE_WAIT_AFTER")?;
+        if flags & allowed_flags != flags {
+            let einval = this.eval_libc("EINVAL")?;
+            this.set_last_error(einval)?;
+            return Ok(-1);
+        }
+
         if let Some(FileHandle { file, writable: _ }) = this.machine.file_handler.handles.get_mut(&fd) {
             // In the interest of host compatibility, we conservatively ignore
             // offset, nbytes, and flags, and sync the entire file.

tests/run-pass/fs.rs

@@ -14,8 +14,7 @@ fn main() {
     test_seek();
     test_metadata();
     test_file_set_len();
-    test_file_sync_all();
-    test_file_sync_data();
+    test_file_sync();
     test_symlink();
     test_errors();
     test_rename();
@@ -184,24 +183,14 @@ fn test_file_set_len() {
     remove_file(&path).unwrap();
 }
 
-fn test_file_sync_all() {
+fn test_file_sync() {
     let bytes = b"Hello, World!\n";
-    let path = prepare_with_content("miri_test_fs_sync_all.txt", bytes);
+    let path = prepare_with_content("miri_test_fs_sync.txt", bytes);
 
-    // Test that we can call sync_all (can't readily test effects of this operation)
-    let file = File::open(&path).unwrap();
-    file.sync_all().unwrap();
-
-    remove_file(&path).unwrap();
-}
-
-fn test_file_sync_data() {
-    let bytes = b"Hello, World!\n";
-    let path = prepare_with_content("miri_test_fs_sync_data.txt", bytes);
-
-    // Test that we can call sync_data (can't readily test effects of this operation)
+    // Test that we can call sync_data and sync_all (can't readily test effects of this operation)
     let file = File::open(&path).unwrap();
     file.sync_data().unwrap();
+    file.sync_all().unwrap();
 
     remove_file(&path).unwrap();
 }