address review

Author: LorrensP-2158466

src/shims/unix/foreign_items.rs

@@ -487,8 +487,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 this.write_null(dest)?;
             }
 
-            // only macos doesn't support `posix_fallocate`
-            "posix_fallocate" if &*this.tcx.sess.target.os != "macos" => {
+            "posix_fallocate" => {
+                // posix_fallocate is not supported by macos.
+                this.check_target_os(
+                    &["linux", "freebsd", "solaris", "illumos", "android"],
+                    link_name,
+                )?;
                 let [fd, offset, len] = this.check_shim_sig(
                     shim_sig!(extern "C" fn(i32, libc::off_t, libc::off_t) -> i32),
                     link_name,
@@ -504,8 +508,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 this.write_scalar(result, dest)?;
             }
 
-            // only macos doesn't support `posix_fallocate`
-            "posix_fallocate64" if &*this.tcx.sess.target.os != "macos" => {
+            "posix_fallocate64" => {
+                // posix_fallocate is not supported by macos.
+                this.check_target_os(
+                    &["linux", "freebsd", "solaris", "illumos", "android"],
+                    link_name,
+                )?;
                 let [fd, offset, len] = this.check_shim_sig(
                     shim_sig!(extern "C" fn(i32, libc::off64_t, libc::off64_t) -> i32),
                     link_name,

src/shims/unix/fs.rs

@@ -1197,6 +1197,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         }
     }
 
+    /// NOTE: According to the man page of `possix_fallocate`, it returns the error code instead
+    /// of setting `errno`.
     fn posix_fallocate(
         &mut self,
         fd_num: i32,
@@ -1205,54 +1207,56 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     ) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
-        // According to the man page of `possix_fallocate`, it returns the error code instead
-        // of setting `errno`.
-        let ebadf = Scalar::from_i32(this.eval_libc_i32("EBADF"));
-        let einval = Scalar::from_i32(this.eval_libc_i32("EINVAL"));
-        let enodev = Scalar::from_i32(this.eval_libc_i32("ENODEV"));
-
         // Reject if isolation is enabled.
         if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
             this.reject_in_isolation("`posix_fallocate`", reject_with)?;
-            // Set error code as "EBADF" (bad fd)
-            return interp_ok(ebadf);
+            // Return error code "EBADF" (bad fd).
+            return interp_ok(this.eval_libc("EBADF"));
         }
 
         let Some(fd) = this.machine.fds.get(fd_num) else {
-            return interp_ok(ebadf);
+            return interp_ok(this.eval_libc("EBADF"));
         };
 
         let file = match fd.downcast::<FileHandle>() {
             Some(file_handle) => file_handle,
             // Man page specifies to return ENODEV if `fd` is not a regular file.
-            None => return interp_ok(enodev),
+            None => return interp_ok(this.eval_libc("ENODEV")),
         };
 
         // EINVAL is returned when: "offset was less than 0, or len was less than or equal to 0".
         if offset < 0 || len <= 0 {
-            return interp_ok(einval);
+            return interp_ok(this.eval_libc("EINVAL"));
         }
 
-        if file.writable {
-            let current_size = match file.file.metadata() {
-                Ok(metadata) => metadata.len(),
-                Err(err) => return this.io_error_to_errnum(err),
-            };
-            let new_size = match offset.checked_add(len) {
-                Some(size) => size.try_into().unwrap(), // We just checked negative `offset` and `len`.
-                None => return interp_ok(einval),
+        if !file.writable {
+            // The file is not writable.
+            return interp_ok(this.eval_libc("EBADF"));
+        }
+
+        let current_size = match file.file.metadata() {
+            Ok(metadata) => metadata.len(),
+            Err(err) => return this.io_error_to_errnum(err),
+        };
+        let new_size = match offset.checked_add(len) {
+            // u64::from(i128) can fail if:
+            //  - the value is negative, but we checed this above with `offset < 0 || len <= 0`
+            //  - the value is too big/small to fit in u64, this should not happen since the callers
+            //    check if the value is a `i32` or `i64`.
+            // So this unwrap is safe to do.
+            Some(size) => u64::try_from(size).unwrap(),
+            None => return interp_ok(this.eval_libc("EFBIG")), // Size to big
+        };
+        // `posix_fallocate` only specifies increasing the file size.
+        if current_size < new_size {
+            let result = match file.file.set_len(new_size) {
+                Ok(()) => Scalar::from_i32(0),
+                Err(e) => this.io_error_to_errnum(e)?,
             };
-            // `posix_fallocate` only specifies increasing the file size.
-            if current_size < new_size {
-                let result = file.file.set_len(new_size);
-                let result = this.try_unwrap_io_result(result.map(|_| 0i32))?;
-                interp_ok(Scalar::from_i32(result))
-            } else {
-                interp_ok(Scalar::from_i32(0))
-            }
+
+            interp_ok(result)
         } else {
-            // The file is not writable.
-            interp_ok(ebadf)
+            interp_ok(Scalar::from_i32(0))
         }
     }
 

tests/pass-dep/libc/libc-fs.rs

@@ -36,9 +36,7 @@ fn main() {
     test_posix_realpath_errors();
     #[cfg(target_os = "linux")]
     test_posix_fadvise();
-    #[cfg(target_os = "linux")]
     test_posix_fallocate::<libc::off_t>(libc::posix_fallocate);
-    #[cfg(target_os = "linux")]
     test_posix_fallocate::<libc::off64_t>(libc::posix_fallocate64);
     #[cfg(target_os = "linux")]
     test_sync_file_range();
@@ -339,7 +337,6 @@ fn test_posix_fadvise() {
     assert_eq!(result, 0);
 }
 
-#[cfg(target_os = "linux")]
 fn test_posix_fallocate<T: From<i32>>(
     posix_fallocate: unsafe extern "C" fn(fd: libc::c_int, offset: T, len: T) -> libc::c_int,
 ) {
@@ -351,7 +348,7 @@ fn test_posix_fallocate<T: From<i32>>(
         let ret = unsafe { posix_fallocate(42, T::from(0), T::from(10)) };
         assert_eq!(ret, libc::EBADF);
 
-        let path = utils::prepare("miri_test_libc_possix_fallocate_errors.txt");
+        let path = utils::prepare("miri_test_libc_posix_fallocate_errors.txt");
         let file = File::create(&path).unwrap();
 
         // invalid offset
@@ -371,7 +368,7 @@ fn test_posix_fallocate<T: From<i32>>(
 
     let test = || {
         let bytes = b"hello";
-        let path = utils::prepare("miri_test_libc_fs_ftruncate.txt");
+        let path = utils::prepare("miri_test_libc_posix_fallocate.txt");
         let mut file = File::create(&path).unwrap();
         file.write_all(bytes).unwrap();
         file.sync_all().unwrap();