Merge pull request #3990 from RalfJung/errno-cleanup

Consistently use io error handlers

src/tools/miri/src/lib.rs

@@ -147,7 +147,7 @@
 pub use crate::shims::EmulateItemResult;
 pub use crate::shims::env::{EnvVars, EvalContextExt as _};
 pub use crate::shims::foreign_items::{DynSym, EvalContextExt as _};
-pub use crate::shims::io_error::{EvalContextExt as _, LibcError};
+pub use crate::shims::io_error::{EvalContextExt as _, IoError, LibcError};
 pub use crate::shims::os_str::EvalContextExt as _;
 pub use crate::shims::panic::{CatchUnwindData, EvalContextExt as _};
 pub use crate::shims::time::EvalContextExt as _;

src/tools/miri/src/shims/unix/fd.rs

@@ -423,7 +423,7 @@ fn dup(&mut self, old_fd_num: i32) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
         let Some(fd) = this.machine.fds.get(old_fd_num) else {
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         };
         interp_ok(Scalar::from_i32(this.machine.fds.insert(fd)))
     }
@@ -432,7 +432,7 @@ fn dup2(&mut self, old_fd_num: i32, new_fd_num: i32) -> InterpResult<'tcx, Scala
         let this = self.eval_context_mut();
 
         let Some(fd) = this.machine.fds.get(old_fd_num) else {
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         };
         if new_fd_num != old_fd_num {
             // Close new_fd if it is previously opened.
@@ -448,7 +448,7 @@ fn dup2(&mut self, old_fd_num: i32, new_fd_num: i32) -> InterpResult<'tcx, Scala
     fn flock(&mut self, fd_num: i32, op: i32) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
         let Some(fd) = this.machine.fds.get(fd_num) else {
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         };
 
         // We need to check that there aren't unsupported options in `op`.
@@ -498,11 +498,11 @@ fn fcntl(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, Scalar> {
                 // `FD_CLOEXEC` value without checking if the flag is set for the file because `std`
                 // always sets this flag when opening a file. However we still need to check that the
                 // file itself is open.
-                interp_ok(Scalar::from_i32(if this.machine.fds.is_fd_num(fd_num) {
+                if !this.machine.fds.is_fd_num(fd_num) {
-                    this.eval_libc_i32("FD_CLOEXEC")
+                    this.set_last_error_and_return_i32(LibcError("EBADF"))
                 } else {
-                    this.fd_not_found()?
+                    interp_ok(this.eval_libc("FD_CLOEXEC"))
-                }))
+                }
             }
             cmd if cmd == f_dupfd || cmd == f_dupfd_cloexec => {
                 // Note that we always assume the FD_CLOEXEC flag is set for every open file, in part
@@ -521,7 +521,7 @@ fn fcntl(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, Scalar> {
                 if let Some(fd) = this.machine.fds.get(fd_num) {
                     interp_ok(Scalar::from_i32(this.machine.fds.insert_with_min_num(fd, start)))
                 } else {
-                    interp_ok(Scalar::from_i32(this.fd_not_found()?))
+                    this.set_last_error_and_return_i32(LibcError("EBADF"))
                 }
             }
             cmd if this.tcx.sess.target.os == "macos"
@@ -547,7 +547,7 @@ fn close(&mut self, fd_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
         let fd_num = this.read_scalar(fd_op)?.to_i32()?;
 
         let Some(fd) = this.machine.fds.remove(fd_num) else {
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         };
         let result = fd.close(this.machine.communicate(), this)?;
         // return `0` if close is successful
@@ -555,16 +555,6 @@ fn close(&mut self, fd_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
         interp_ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))
     }
 
-    /// Function used when a file descriptor does not exist. It returns `Ok(-1)`and sets
-    /// the last OS error to `libc::EBADF` (invalid file descriptor). This function uses
-    /// `T: From<i32>` instead of `i32` directly because some fs functions return different integer
-    /// types (like `read`, that returns an `i64`).
-    fn fd_not_found<T: From<i32>>(&mut self) -> InterpResult<'tcx, T> {
-        let this = self.eval_context_mut();
-        this.set_last_error(LibcError("EBADF"))?;
-        interp_ok((-1).into())
-    }
-
     /// Read data from `fd` into buffer specified by `buf` and `count`.
     ///
     /// If `offset` is `None`, reads data from current cursor position associated with `fd`
@@ -598,9 +588,7 @@ fn read(
         // We temporarily dup the FD to be able to retain mutable access to `this`.
         let Some(fd) = this.machine.fds.get(fd_num) else {
             trace!("read: FD not found");
-            let res: i32 = this.fd_not_found()?;
+            return this.set_last_error_and_return(LibcError("EBADF"), dest);
-            this.write_int(res, dest)?;
-            return interp_ok(());
         };
 
         trace!("read: FD mapped to {fd:?}");
@@ -645,9 +633,7 @@ fn write(
 
         // We temporarily dup the FD to be able to retain mutable access to `this`.
         let Some(fd) = this.machine.fds.get(fd_num) else {
-            let res: i32 = this.fd_not_found()?;
+            return this.set_last_error_and_return(LibcError("EBADF"), dest);
-            this.write_int(res, dest)?;
-            return interp_ok(());
         };
 
         match offset {

src/tools/miri/src/shims/unix/fs.rs

@@ -595,7 +595,7 @@ fn lseek64(&mut self, fd_num: i32, offset: i128, whence: i32) -> InterpResult<'t
         let communicate = this.machine.communicate();
 
         let Some(fd) = this.machine.fds.get(fd_num) else {
-            return interp_ok(Scalar::from_i64(this.fd_not_found()?));
+            return this.set_last_error_and_return_i64(LibcError("EBADF"));
         };
         let result = fd.seek(communicate, seek_from)?.map(|offset| i64::try_from(offset).unwrap());
         drop(fd);
@@ -671,8 +671,8 @@ fn macos_fbsd_stat(
 
         // `stat` always follows symlinks.
         let metadata = match FileMetadata::from_path(this, &path, true)? {
-            Some(metadata) => metadata,
+            Ok(metadata) => metadata,
-            None => return interp_ok(Scalar::from_i32(-1)), // `FileMetadata` has set errno
+            Err(err) => return this.set_last_error_and_return_i32(err),
         };
 
         interp_ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?))
@@ -700,8 +700,8 @@ fn macos_fbsd_lstat(
         }
 
         let metadata = match FileMetadata::from_path(this, &path, false)? {
-            Some(metadata) => metadata,
+            Ok(metadata) => metadata,
-            None => return interp_ok(Scalar::from_i32(-1)), // `FileMetadata` has set errno
+            Err(err) => return this.set_last_error_and_return_i32(err),
         };
 
         interp_ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?))
@@ -724,12 +724,12 @@ fn macos_fbsd_fstat(
         if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
             this.reject_in_isolation("`fstat`", reject_with)?;
             // Set error code as "EBADF" (bad fd)
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         }
 
         let metadata = match FileMetadata::from_fd_num(this, fd)? {
-            Some(metadata) => metadata,
+            Ok(metadata) => metadata,
-            None => return interp_ok(Scalar::from_i32(-1)),
+            Err(err) => return this.set_last_error_and_return_i32(err),
         };
         interp_ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?))
     }
@@ -816,8 +816,8 @@ fn linux_statx(
             FileMetadata::from_path(this, &path, follow_symlink)?
         };
         let metadata = match metadata {
-            Some(metadata) => metadata,
+            Ok(metadata) => metadata,
-            None => return interp_ok(Scalar::from_i32(-1)),
+            Err(err) => return this.set_last_error_and_return_i32(err),
         };
 
         // The `mode` field specifies the type of the file and the permissions over the file for
@@ -1131,7 +1131,7 @@ fn macos_fbsd_readdir_r(
         if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
             this.reject_in_isolation("`readdir_r`", reject_with)?;
             // Set error code as "EBADF" (bad fd)
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         }
 
         let open_dir = this.machine.dirs.streams.get_mut(&dirp).ok_or_else(|| {
@@ -1242,20 +1242,20 @@ fn closedir(&mut self, dirp_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
         let dirp = this.read_target_usize(dirp_op)?;
 
         // Reject if isolation is enabled.
-        interp_ok(Scalar::from_i32(
         if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
             this.reject_in_isolation("`closedir`", reject_with)?;
-                this.fd_not_found()?
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
-            } else if let Some(open_dir) = this.machine.dirs.streams.remove(&dirp) {
+        }
+
+        let Some(open_dir) = this.machine.dirs.streams.remove(&dirp) else {
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
+        };
         if let Some(entry) = open_dir.entry {
             this.deallocate_ptr(entry, None, MiriMemoryKind::Runtime.into())?;
         }
         drop(open_dir);
-                0
+
-            } else {
+        interp_ok(Scalar::from_i32(0))
-                this.fd_not_found()?
-            },
-        ))
     }
 
     fn ftruncate64(&mut self, fd_num: i32, length: i128) -> InterpResult<'tcx, Scalar> {
@@ -1265,11 +1265,11 @@ fn ftruncate64(&mut self, fd_num: i32, length: i128) -> InterpResult<'tcx, Scala
         if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
             this.reject_in_isolation("`ftruncate64`", reject_with)?;
             // Set error code as "EBADF" (bad fd)
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         }
 
         let Some(fd) = this.machine.fds.get(fd_num) else {
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         };
 
         // FIXME: Support ftruncate64 for all FDs
@@ -1308,7 +1308,7 @@ fn fsync(&mut self, fd_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
         if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
             this.reject_in_isolation("`fsync`", reject_with)?;
             // Set error code as "EBADF" (bad fd)
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         }
 
         self.ffullsync_fd(fd)
@@ -1317,7 +1317,7 @@ fn fsync(&mut self, fd_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
     fn ffullsync_fd(&mut self, fd_num: i32) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
         let Some(fd) = this.machine.fds.get(fd_num) else {
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         };
         // Only regular files support synchronization.
         let FileHandle { file, writable } = fd.downcast::<FileHandle>().ok_or_else(|| {
@@ -1337,11 +1337,11 @@ fn fdatasync(&mut self, fd_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
         if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
             this.reject_in_isolation("`fdatasync`", reject_with)?;
             // Set error code as "EBADF" (bad fd)
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         }
 
         let Some(fd) = this.machine.fds.get(fd) else {
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         };
         // Only regular files support synchronization.
         let FileHandle { file, writable } = fd.downcast::<FileHandle>().ok_or_else(|| {
@@ -1380,11 +1380,11 @@ fn sync_file_range(
         if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
             this.reject_in_isolation("`sync_file_range`", reject_with)?;
             // Set error code as "EBADF" (bad fd)
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         }
 
         let Some(fd) = this.machine.fds.get(fd) else {
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         };
         // Only regular files support synchronization.
         let FileHandle { file, writable } = fd.downcast::<FileHandle>().ok_or_else(|| {
@@ -1667,7 +1667,7 @@ fn from_path<'tcx>(
         ecx: &mut MiriInterpCx<'tcx>,
         path: &Path,
         follow_symlink: bool,
-    ) -> InterpResult<'tcx, Option<FileMetadata>> {
+    ) -> InterpResult<'tcx, Result<FileMetadata, IoError>> {
         let metadata =
             if follow_symlink { std::fs::metadata(path) } else { std::fs::symlink_metadata(path) };
 
@@ -1677,9 +1677,9 @@ fn from_path<'tcx>(
     fn from_fd_num<'tcx>(
         ecx: &mut MiriInterpCx<'tcx>,
         fd_num: i32,
-    ) -> InterpResult<'tcx, Option<FileMetadata>> {
+    ) -> InterpResult<'tcx, Result<FileMetadata, IoError>> {
         let Some(fd) = ecx.machine.fds.get(fd_num) else {
-            return ecx.fd_not_found().map(|_: i32| None);
+            return interp_ok(Err(LibcError("EBADF")));
         };
 
         let file = &fd
@@ -1699,12 +1699,11 @@ fn from_fd_num<'tcx>(
     fn from_meta<'tcx>(
         ecx: &mut MiriInterpCx<'tcx>,
         metadata: Result<std::fs::Metadata, std::io::Error>,
-    ) -> InterpResult<'tcx, Option<FileMetadata>> {
+    ) -> InterpResult<'tcx, Result<FileMetadata, IoError>> {
         let metadata = match metadata {
             Ok(metadata) => metadata,
             Err(e) => {
-                ecx.set_last_error(e)?;
+                return interp_ok(Err(e.into()));
-                return interp_ok(None);
             }
         };
 
@@ -1727,6 +1726,6 @@ fn from_meta<'tcx>(
         let modified = extract_sec_and_nsec(metadata.modified())?;
 
         // FIXME: Provide more fields using platform specific methods.
-        interp_ok(Some(FileMetadata { mode, size, created, accessed, modified }))
+        interp_ok(Ok(FileMetadata { mode, size, created, accessed, modified }))
     }
 }

src/tools/miri/src/shims/unix/linux/epoll.rs

@@ -263,7 +263,7 @@ fn epoll_ctl(
 
         // Check if epfd is a valid epoll file descriptor.
         let Some(epfd) = this.machine.fds.get(epfd_value) else {
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         };
         let epoll_file_description = epfd
             .downcast::<Epoll>()
@@ -273,7 +273,7 @@ fn epoll_ctl(
         let ready_list = &epoll_file_description.ready_list;
 
         let Some(fd_ref) = this.machine.fds.get(fd) else {
-            return interp_ok(Scalar::from_i32(this.fd_not_found()?));
+            return this.set_last_error_and_return_i32(LibcError("EBADF"));
         };
         let id = fd_ref.get_id();
 
@@ -433,9 +433,7 @@ fn epoll_wait(
         let timeout = this.read_scalar(timeout)?.to_i32()?;
 
         if epfd_value <= 0 || maxevents <= 0 {
-            this.set_last_error(LibcError("EINVAL"))?;
+            return this.set_last_error_and_return(LibcError("EINVAL"), dest);
-            this.write_int(-1, dest)?;
-            return interp_ok(());
         }
 
         // This needs to come after the maxevents value check, or else maxevents.try_into().unwrap()
@@ -446,9 +444,7 @@ fn epoll_wait(
         )?;
 
         let Some(epfd) = this.machine.fds.get(epfd_value) else {
-            let result_value: i32 = this.fd_not_found()?;
+            return this.set_last_error_and_return(LibcError("EBADF"), dest);
-            this.write_int(result_value, dest)?;
-            return interp_ok(());
         };
         // Create a weak ref of epfd and pass it to callback so we will make sure that epfd
         // is not close after the thread unblocks.

src/tools/miri/src/shims/unix/linux/sync.rs

@@ -63,8 +63,7 @@ pub fn futex<'tcx>(
             };
 
             if bitset == 0 {
-                this.set_last_error(LibcError("EINVAL"))?;
+                this.set_last_error_and_return(LibcError("EINVAL"), dest)?;
-                this.write_scalar(Scalar::from_target_isize(-1, this), dest)?;
                 return interp_ok(());
             }
 
@@ -75,9 +74,7 @@ pub fn futex<'tcx>(
                 let duration = match this.read_timespec(&timeout)? {
                     Some(duration) => duration,
                     None => {
-                        this.set_last_error(LibcError("EINVAL"))?;
+                        return this.set_last_error_and_return(LibcError("EINVAL"), dest);
-                        this.write_scalar(Scalar::from_target_isize(-1, this), dest)?;
-                        return interp_ok(());
                     }
                 };
                 let timeout_clock = if op & futex_realtime == futex_realtime {
@@ -153,12 +150,12 @@ pub fn futex<'tcx>(
                     Scalar::from_target_isize(0, this), // retval_succ
                     Scalar::from_target_isize(-1, this), // retval_timeout
                     dest.clone(),
-                    this.eval_libc("ETIMEDOUT"),
+                    this.eval_libc("ETIMEDOUT"), // errno_timeout
                 );
             } else {
                 // The futex value doesn't match the expected value, so we return failure
                 // right away without sleeping: -1 and errno set to EAGAIN.
-                this.set_last_error_and_return(LibcError("EAGAIN"), dest)?;
+                return this.set_last_error_and_return(LibcError("EAGAIN"), dest);
             }
         }
         // FUTEX_WAKE: (int *addr, int op = FUTEX_WAKE, int val)
@@ -178,9 +175,7 @@ pub fn futex<'tcx>(
                 u32::MAX
             };
             if bitset == 0 {
-                this.set_last_error(LibcError("EINVAL"))?;
+                return this.set_last_error_and_return(LibcError("EINVAL"), dest);
-                this.write_scalar(Scalar::from_target_isize(-1, this), dest)?;
-                return interp_ok(());
             }
             // Together with the SeqCst fence in futex_wait, this makes sure that futex_wait
             // will see the latest value on addr which could be changed by our caller

src/tools/miri/src/shims/windows/sync.rs

@@ -202,7 +202,7 @@ fn WaitOnAddress(
                 Scalar::from_i32(1), // retval_succ
                 Scalar::from_i32(0), // retval_timeout
                 dest.clone(),
-                this.eval_windows("c", "ERROR_TIMEOUT"),
+                this.eval_windows("c", "ERROR_TIMEOUT"), // errno_timeout
             );
         }