Skip to content

Commit

Permalink
Merge pull request #3990 from RalfJung/errno-cleanup
Browse files Browse the repository at this point in the history 
Consistently use io error handlers
  • Loading branch information
@RalfJung
RalfJung authored Oct 25, 2024
2 parents a21369f + 2350ba7 commit cbf6c86
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 85 deletions.
2 changes: 1 addition & 1 deletion src/tools/miri/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ pub use crate::range_map::RangeMap;
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 _;
Expand Down
36 changes: 11 additions & 25 deletions src/tools/miri/src/shims/unix/fd.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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)))
}
Expand All @@ -432,7 +432,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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.
Expand All @@ -448,7 +448,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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`.
Expand Down Expand Up @@ -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
Expand All @@ -521,7 +521,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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"
Expand All @@ -547,24 +547,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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
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`).
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`
Expand Down Expand Up @@ -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`.
let Some(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)?;
return interp_ok(());
return this.set_last_error_and_return(LibcError("EBADF"), dest);
};

trace!("read: FD mapped to {fd:?}");
Expand Down Expand Up @@ -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`.
let Some(fd) = this.machine.fds.get(fd_num) else {
let res: i32 = this.fd_not_found()?;
this.write_int(res, dest)?;
return interp_ok(());
return this.set_last_error_and_return(LibcError("EBADF"), dest);
};

match offset {
Expand Down
79 changes: 39 additions & 40 deletions src/tools/miri/src/shims/unix/fs.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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);
Expand Down Expand Up @@ -671,8 +671,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {

// `stat` always follows symlinks.
let metadata = match FileMetadata::from_path(this, &path, true)? {
Some(metadata) => metadata,
None => return interp_ok(Scalar::from_i32(-1)), // `FileMetadata` has set errno
Ok(metadata) => metadata,
Err(err) => return this.set_last_error_and_return_i32(err),
};

interp_ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?))
Expand Down Expand Up @@ -700,8 +700,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
}

let metadata = match FileMetadata::from_path(this, &path, false)? {
Some(metadata) => metadata,
None => return interp_ok(Scalar::from_i32(-1)), // `FileMetadata` has set errno
Ok(metadata) => metadata,
Err(err) => return this.set_last_error_and_return_i32(err),
};

interp_ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?))
Expand All @@ -724,12 +724,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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,
None => return interp_ok(Scalar::from_i32(-1)),
Ok(metadata) => metadata,
Err(err) => return this.set_last_error_and_return_i32(err),
};
interp_ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?))
}
Expand Down Expand Up @@ -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 => return interp_ok(Scalar::from_i32(-1)),
Ok(metadata) => metadata,
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
Expand Down Expand Up @@ -1131,7 +1131,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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(|| {
Expand Down Expand Up @@ -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(
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`closedir`", reject_with)?;
this.fd_not_found()?
} else if let Some(open_dir) = this.machine.dirs.streams.remove(&dirp) {
if let Some(entry) = open_dir.entry {
this.deallocate_ptr(entry, None, MiriMemoryKind::Runtime.into())?;
}
drop(open_dir);
0
} else {
this.fd_not_found()?
},
))
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`closedir`", reject_with)?;
return this.set_last_error_and_return_i32(LibcError("EBADF"));
}

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);

interp_ok(Scalar::from_i32(0))
}

fn ftruncate64(&mut self, fd_num: i32, length: i128) -> InterpResult<'tcx, Scalar> {
Expand All @@ -1265,11 +1265,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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
Expand Down Expand Up @@ -1308,7 +1308,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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)
Expand All @@ -1317,7 +1317,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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(|| {
Expand All @@ -1337,11 +1337,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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(|| {
Expand Down Expand Up @@ -1380,11 +1380,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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(|| {
Expand Down Expand Up @@ -1667,7 +1667,7 @@ impl FileMetadata {
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) };

Expand All @@ -1677,9 +1677,9 @@ impl FileMetadata {
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
Expand All @@ -1699,12 +1699,11 @@ impl FileMetadata {
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(None);
return interp_ok(Err(e.into()));
}
};

Expand All @@ -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 }))
}
}
12 changes: 4 additions & 8 deletions src/tools/miri/src/shims/unix/linux/epoll.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {

// 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>()
Expand All @@ -273,7 +273,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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();

Expand Down Expand Up @@ -433,9 +433,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let timeout = this.read_scalar(timeout)?.to_i32()?;

if epfd_value <= 0 || maxevents <= 0 {
this.set_last_error(LibcError("EINVAL"))?;
this.write_int(-1, dest)?;
return interp_ok(());
return this.set_last_error_and_return(LibcError("EINVAL"), dest);
}

// This needs to come after the maxevents value check, or else maxevents.try_into().unwrap()
Expand All @@ -446,9 +444,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
)?;

let Some(epfd) = this.machine.fds.get(epfd_value) else {
let result_value: i32 = this.fd_not_found()?;
this.write_int(result_value, dest)?;
return interp_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.
Expand Down
Loading

0 comments on commit cbf6c86

Please sign in to comment.