Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistently use io error handlers #3990

Merged
merged 3 commits into from
Oct 25, 2024
Merged

Consistently use io error handlers #3990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
269b503
get rid of fd_not_found; improve error handling in FileMetadata
RalfJung Oct 25, 2024
ce615e8
some more opportunities for set_last_error_and_return
RalfJung Oct 25, 2024
95fbffd
indicate more explicitly where we close host file/dir handles
RalfJung Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/lib.rs
View file
Open in desktop
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/shims/unix/fd.rs
View file
Open in desktop
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
82 changes: 42 additions & 40 deletions src/shims/unix/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ impl FileDescription for FileHandle {
// to handle possible errors correctly.
let result = self.file.sync_all();
// Now we actually close the file and return the result.
drop(*self);
interp_ok(result)
} else {
// We drop the file, this closes it but ignores any errors
Expand All @@ -157,6 +158,7 @@ impl FileDescription for FileHandle {
// `/dev/urandom` which are read-only. Check
// https://github.com/rust-lang/miri/issues/999#issuecomment-568920439
// for a deeper discussion.
drop(*self);
interp_ok(Ok(()))
}
}
Expand Down Expand Up @@ -595,7 +597,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 +673,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 +702,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 +726,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 +818,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 +1133,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 +1244,21 @@ 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(mut 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.take() {
this.deallocate_ptr(entry, None, MiriMemoryKind::Runtime.into())?;
}
// We drop the `open_dir`, which will close the host dir handle.
drop(open_dir);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this drop actually doing anything? It'll be dropped anyway at the upcoming close brace, right?

Suggested change
drop(open_dir);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be intended to clarify that we own open_dir here... but it doesn't really achieve that to be fair.


interp_ok(Scalar::from_i32(0))
}

fn ftruncate64(&mut self, fd_num: i32, length: i128) -> InterpResult<'tcx, Scalar> {
Expand All @@ -1265,11 +1268,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 +1311,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 +1320,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 +1340,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 +1383,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 +1670,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 +1680,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 +1702,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 +1729,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/shims/unix/linux/epoll.rs
View file
Open in desktop
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