get rid of fd_not_found; improve error handling in FileMetadata

Author: RalfJung

src/tools/miri/src/lib.rs

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

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

@@ -423,7 +423,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 let this = self.eval_context_mut();
 
 letSome(fd) = this.machine.fds.get(old_fd_num)else{
-returninterp_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 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 let this = self.eval_context_mut();
 
 letSome(fd) = this.machine.fds.get(old_fd_num)else{
-returninterp_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 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 fnflock(&mutself,fd_num:i32,op:i32) -> InterpResult<'tcx,Scalar>{
 let this = self.eval_context_mut();
 letSome(fd) = this.machine.fds.get(fd_num)else{
-returninterp_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 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 // `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){
- this.eval_libc_i32("FD_CLOEXEC")
+if!this.machine.fds.is_fd_num(fd_num){
+ 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 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 ifletSome(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,24 +547,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 let fd_num = this.read_scalar(fd_op)?.to_i32()?;
 
 letSome(fd) = this.machine.fds.remove(fd_num)else{
-returninterp_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
 let result = result.map(|()| 0i32);
 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`).
-fnfd_not_found<T:From<i32>>(&mutself) -> 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 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 // We temporarily dup the FD to be able to retain mutable access to `this`.
 letSome(fd) = this.machine.fds.get(fd_num)else{
 trace!("read: FD not found");
-let res:i32 = this.fd_not_found()?;
- this.write_int(res, dest)?;
-returninterp_ok(());
+return this.set_last_error_and_return(LibcError("EBADF"), dest);
 };
 
 trace!("read: FD mapped to {fd:?}");
@@ -645,9 +633,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
 // We temporarily dup the FD to be able to retain mutable access to `this`.
 letSome(fd) = this.machine.fds.get(fd_num)else{
-let res:i32 = this.fd_not_found()?;
- this.write_int(res, dest)?;
-returninterp_ok(());
+return this.set_last_error_and_return(LibcError("EBADF"), dest);
 };
 
 match offset {

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

@@ -595,7 +595,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 let communicate = this.machine.communicate();
 
 letSome(fd) = this.machine.fds.get(fd_num)else{
-returninterp_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 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
 // `stat` always follows symlinks.
 let metadata = matchFileMetadata::from_path(this,&path,true)? {
-Some(metadata) => metadata,
-None => returninterp_ok(Scalar::from_i32(-1)),// `FileMetadata` has set errno
+Ok(metadata) => metadata,
+Err(err) => returnthis.set_last_error_and_return_i32(err),
 };
 
 interp_ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?))
@@ -700,8 +700,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 }
 
 let metadata = matchFileMetadata::from_path(this,&path,false)? {
-Some(metadata) => metadata,
-None => returninterp_ok(Scalar::from_i32(-1)),// `FileMetadata` has set errno
+Ok(metadata) => metadata,
+Err(err) => returnthis.set_last_error_and_return_i32(err),
 };
 
 interp_ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?))
@@ -724,12 +724,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 ifletIsolatedOp::Reject(reject_with) = this.machine.isolated_op{
  this.reject_in_isolation("`fstat`", reject_with)?;
 // Set error code as "EBADF" (bad fd)
-returninterp_ok(Scalar::from_i32(this.fd_not_found()?));
+return this.set_last_error_and_return_i32(LibcError("EBADF"));
 }
 
 let metadata = matchFileMetadata::from_fd_num(this, fd)? {
-Some(metadata) => metadata,
-None => returninterp_ok(Scalar::from_i32(-1)),
+Ok(metadata) => metadata,
+Err(err) => returnthis.set_last_error_and_return_i32(err),
 };
 interp_ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?))
 }
@@ -816,8 +816,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 FileMetadata::from_path(this,&path, follow_symlink)?
 };
 let metadata = match metadata {
-Some(metadata) => metadata,
-None => returninterp_ok(Scalar::from_i32(-1)),
+Ok(metadata) => metadata,
+Err(err) => returnthis.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 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 ifletIsolatedOp::Reject(reject_with) = this.machine.isolated_op{
  this.reject_in_isolation("`readdir_r`", reject_with)?;
 // Set error code as "EBADF" (bad fd)
-returninterp_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 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 let dirp = this.read_target_usize(dirp_op)?;
 
 // Reject if isolation is enabled.
-interp_ok(Scalar::from_i32(
-ifletIsolatedOp::Reject(reject_with) = this.machine.isolated_op{
-this.reject_in_isolation("`closedir`", reject_with)?;
- this.fd_not_found()?
-}elseifletSome(open_dir) = this.machine.dirs.streams.remove(&dirp){
-ifletSome(entry) = open_dir.entry{
-this.deallocate_ptr(entry,None,MiriMemoryKind::Runtime.into())?;
-}
-drop(open_dir);
-0
-}else{
- this.fd_not_found()?
-},
-))
+ifletIsolatedOp::Reject(reject_with) = this.machine.isolated_op{
+ this.reject_in_isolation("`closedir`", reject_with)?;
+returnthis.set_last_error_and_return_i32(LibcError("EBADF"));
+}
+
+letSome(open_dir) = this.machine.dirs.streams.remove(&dirp)else{
+returnthis.set_last_error_and_return_i32(LibcError("EBADF"));
+};
+ifletSome(entry) = open_dir.entry{
+this.deallocate_ptr(entry,None,MiriMemoryKind::Runtime.into())?;
+}
+drop(open_dir);
+
+interp_ok(Scalar::from_i32(0))
 }
 
 fnftruncate64(&mutself,fd_num:i32,length:i128) -> InterpResult<'tcx,Scalar>{
@@ -1265,11 +1265,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 ifletIsolatedOp::Reject(reject_with) = this.machine.isolated_op{
  this.reject_in_isolation("`ftruncate64`", reject_with)?;
 // Set error code as "EBADF" (bad fd)
-returninterp_ok(Scalar::from_i32(this.fd_not_found()?));
+return this.set_last_error_and_return_i32(LibcError("EBADF"));
 }
 
 letSome(fd) = this.machine.fds.get(fd_num)else{
-returninterp_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 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 ifletIsolatedOp::Reject(reject_with) = this.machine.isolated_op{
  this.reject_in_isolation("`fsync`", reject_with)?;
 // Set error code as "EBADF" (bad fd)
-returninterp_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 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 fnffullsync_fd(&mutself,fd_num:i32) -> InterpResult<'tcx,Scalar>{
 let this = self.eval_context_mut();
 letSome(fd) = this.machine.fds.get(fd_num)else{
-returninterp_ok(Scalar::from_i32(this.fd_not_found()?));
+return this.set_last_error_and_return_i32(LibcError("EBADF"));
 };
 // Only regular files support synchronization.
 letFileHandle{ file, writable } = fd.downcast::<FileHandle>().ok_or_else(|| {
@@ -1337,11 +1337,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 ifletIsolatedOp::Reject(reject_with) = this.machine.isolated_op{
  this.reject_in_isolation("`fdatasync`", reject_with)?;
 // Set error code as "EBADF" (bad fd)
-returninterp_ok(Scalar::from_i32(this.fd_not_found()?));
+return this.set_last_error_and_return_i32(LibcError("EBADF"));
 }
 
 letSome(fd) = this.machine.fds.get(fd)else{
-returninterp_ok(Scalar::from_i32(this.fd_not_found()?));
+return this.set_last_error_and_return_i32(LibcError("EBADF"));
 };
 // Only regular files support synchronization.
 letFileHandle{ file, writable } = fd.downcast::<FileHandle>().ok_or_else(|| {
@@ -1380,11 +1380,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 ifletIsolatedOp::Reject(reject_with) = this.machine.isolated_op{
  this.reject_in_isolation("`sync_file_range`", reject_with)?;
 // Set error code as "EBADF" (bad fd)
-returninterp_ok(Scalar::from_i32(this.fd_not_found()?));
+return this.set_last_error_and_return_i32(LibcError("EBADF"));
 }
 
 letSome(fd) = this.machine.fds.get(fd)else{
-returninterp_ok(Scalar::from_i32(this.fd_not_found()?));
+return this.set_last_error_and_return_i32(LibcError("EBADF"));
 };
 // Only regular files support synchronization.
 letFileHandle{ file, writable } = fd.downcast::<FileHandle>().ok_or_else(|| {
@@ -1667,7 +1667,7 @@ impl FileMetadata {
 ecx:&mutMiriInterpCx<'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 @@ impl FileMetadata {
 fnfrom_fd_num<'tcx>(
 ecx:&mutMiriInterpCx<'tcx>,
 fd_num:i32,
-) -> InterpResult<'tcx,Option<FileMetadata>>{
+) -> InterpResult<'tcx,Result<FileMetadata,IoError>>{
 letSome(fd) = ecx.machine.fds.get(fd_num)else{
-returnecx.fd_not_found().map(|_:i32| None);
+returninterp_ok(Err(LibcError("EBADF")));
 };
 
 let file = &fd
@@ -1699,12 +1699,11 @@ impl FileMetadata {
 fnfrom_meta<'tcx>(
 ecx:&mutMiriInterpCx<'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)?;
-returninterp_ok(None);
+returninterp_ok(Err(e.into()));
 }
 };
 
@@ -1727,6 +1726,6 @@ impl FileMetadata {
 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 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
 // Check if epfd is a valid epoll file descriptor.
 letSome(epfd) = this.machine.fds.get(epfd_value)else{
-returninterp_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 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 let ready_list = &epoll_file_description.ready_list;
 
 letSome(fd_ref) = this.machine.fds.get(fd)else{
-returninterp_ok(Scalar::from_i32(this.fd_not_found()?));
+return this.set_last_error_and_return_i32(LibcError("EBADF"));
 };
 let id = fd_ref.get_id();
 
@@ -446,9 +446,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 )?;
 
 letSome(epfd) = this.machine.fds.get(epfd_value)else{
-let result_value:i32 = this.fd_not_found()?;
- this.write_int(result_value, dest)?;
-returninterp_ok(());
+return this.set_last_error_and_return(LibcError("EBADF"), dest);
 };
 // 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.