Skip to content

Commit

Permalink
Auto merge of #3941 - noahmbright:unix_shims, r=RalfJung
Browse files Browse the repository at this point in the history 
Replace set_last_error with set_last_error_and_return

Took care of the simple patterns. Other patterns involved setting an error and then using `write_int` or setting metadata and returning -1. Unsure if those are in the scope of this change

Looks like this has conflicts with #3779, so I can update when how to handle that is decided.

Part of #3930.
  • Loading branch information
@bors
bors committed Oct 21, 2024
2 parents da5eeb3 + ca5c26d commit f026d57
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 74 deletions.
10 changes: 10 additions & 0 deletions src/shims/io_error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
interp_ok(Scalar::from_i32(-1))
}

/// Sets the last OS error and return `-1` as a `i64`-typed Scalar
fn set_last_error_and_return_i64(
&mut self,
err: impl Into<IoError>,
) -> InterpResult<'tcx, Scalar> {
let this = self.eval_context_mut();
this.set_last_error(err)?;
interp_ok(Scalar::from_i64(-1))
}

/// Gets the last error variable.
fn get_last_error(&mut self) -> InterpResult<'tcx, Scalar> {
let this = self.eval_context_mut();
Expand Down
3 changes: 1 addition & 2 deletions src/shims/unix/fd.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -561,8 +561,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
/// 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();
let ebadf = this.eval_libc("EBADF");
this.set_last_error(ebadf)?;
this.set_last_error(LibcError("EBADF"))?;
interp_ok((-1).into())
}

Expand Down
82 changes: 25 additions & 57 deletions src/shims/unix/fs.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -528,8 +528,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let o_tmpfile = this.eval_libc_i32("O_TMPFILE");
if flag & o_tmpfile == o_tmpfile {
// if the flag contains `O_TMPFILE` then we return a graceful error
this.set_last_error(LibcError("EOPNOTSUPP"))?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("EOPNOTSUPP"));
}
}

Expand All @@ -548,9 +547,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// O_NOFOLLOW only fails when the trailing component is a symlink;
// the entire rest of the path can still contain symlinks.
if path.is_symlink() {
let eloop = this.eval_libc("ELOOP");
this.set_last_error(eloop)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("ELOOP"));
}
}
mirror |= o_nofollow;
Expand All @@ -565,8 +562,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`open`", reject_with)?;
this.set_last_error(ErrorKind::PermissionDenied)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(ErrorKind::PermissionDenied);
}

let fd = options
Expand All @@ -584,8 +580,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let seek_from = if whence == this.eval_libc_i32("SEEK_SET") {
if offset < 0 {
// Negative offsets return `EINVAL`.
this.set_last_error(LibcError("EINVAL"))?;
return interp_ok(Scalar::from_i64(-1));
return this.set_last_error_and_return_i64(LibcError("EINVAL"));
} else {
SeekFrom::Start(u64::try_from(offset).unwrap())
}
Expand All @@ -594,8 +589,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
} else if whence == this.eval_libc_i32("SEEK_END") {
SeekFrom::End(i64::try_from(offset).unwrap())
} else {
this.set_last_error(LibcError("EINVAL"))?;
return interp_ok(Scalar::from_i64(-1));
return this.set_last_error_and_return_i64(LibcError("EINVAL"));
};

let communicate = this.machine.communicate();
Expand All @@ -618,8 +612,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`unlink`", reject_with)?;
this.set_last_error(ErrorKind::PermissionDenied)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(ErrorKind::PermissionDenied);
}

let result = remove_file(path).map(|_| 0);
Expand Down Expand Up @@ -649,8 +642,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`symlink`", reject_with)?;
this.set_last_error(ErrorKind::PermissionDenied)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(ErrorKind::PermissionDenied);
}

let result = create_link(&target, &linkpath).map(|_| 0);
Expand All @@ -674,9 +666,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`stat`", reject_with)?;
let eacc = this.eval_libc("EACCES");
this.set_last_error(eacc)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("EACCES"));
}

// `stat` always follows symlinks.
Expand Down Expand Up @@ -706,9 +696,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`lstat`", reject_with)?;
let eacc = this.eval_libc("EACCES");
this.set_last_error(eacc)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("EACCES"));
}

let metadata = match FileMetadata::from_path(this, &path, false)? {
Expand Down Expand Up @@ -766,9 +754,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {

// If the statxbuf or pathname pointers are null, the function fails with `EFAULT`.
if this.ptr_is_null(statxbuf_ptr)? || this.ptr_is_null(pathname_ptr)? {
let efault = this.eval_libc("EFAULT");
this.set_last_error(efault)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("EFAULT"));
}

let statxbuf = this.deref_pointer_as(statxbuf_op, this.libc_ty_layout("statx"))?;
Expand Down Expand Up @@ -809,8 +795,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
assert!(empty_path_flag);
this.eval_libc("EBADF")
};
this.set_last_error(ecode)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(ecode);
}

// the `_mask_op` parameter specifies the file information that the caller requested.
Expand Down Expand Up @@ -939,9 +924,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let newpath_ptr = this.read_pointer(newpath_op)?;

if this.ptr_is_null(oldpath_ptr)? || this.ptr_is_null(newpath_ptr)? {
let efault = this.eval_libc("EFAULT");
this.set_last_error(efault)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("EFAULT"));
}

let oldpath = this.read_path_from_c_str(oldpath_ptr)?;
Expand All @@ -950,8 +933,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`rename`", reject_with)?;
this.set_last_error(ErrorKind::PermissionDenied)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(ErrorKind::PermissionDenied);
}

let result = rename(oldpath, newpath).map(|_| 0);
Expand All @@ -974,8 +956,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`mkdir`", reject_with)?;
this.set_last_error(ErrorKind::PermissionDenied)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(ErrorKind::PermissionDenied);
}

#[cfg_attr(not(unix), allow(unused_mut))]
Expand All @@ -1002,8 +983,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`rmdir`", reject_with)?;
this.set_last_error(ErrorKind::PermissionDenied)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(ErrorKind::PermissionDenied);
}

let result = remove_dir(path).map(|_| 0i32);
Expand All @@ -1019,8 +999,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`opendir`", reject_with)?;
let eacc = this.eval_libc("EACCES");
this.set_last_error(eacc)?;
this.set_last_error(LibcError("EACCES"))?;
return interp_ok(Scalar::null_ptr(this));
}

Expand Down Expand Up @@ -1307,14 +1286,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
interp_ok(Scalar::from_i32(result))
} else {
drop(fd);
this.set_last_error(LibcError("EINVAL"))?;
interp_ok(Scalar::from_i32(-1))
this.set_last_error_and_return_i32(LibcError("EINVAL"))
}
} else {
drop(fd);
// The file is not writable
this.set_last_error(LibcError("EINVAL"))?;
interp_ok(Scalar::from_i32(-1))
this.set_last_error_and_return_i32(LibcError("EINVAL"))
}
}

Expand Down Expand Up @@ -1391,15 +1368,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let flags = this.read_scalar(flags_op)?.to_i32()?;

if offset < 0 || nbytes < 0 {
this.set_last_error(LibcError("EINVAL"))?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("EINVAL"));
}
let allowed_flags = this.eval_libc_i32("SYNC_FILE_RANGE_WAIT_BEFORE")
| this.eval_libc_i32("SYNC_FILE_RANGE_WRITE")
| this.eval_libc_i32("SYNC_FILE_RANGE_WAIT_AFTER");
if flags & allowed_flags != flags {
this.set_last_error(LibcError("EINVAL"))?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("EINVAL"));
}

// Reject if isolation is enabled.
Expand Down Expand Up @@ -1436,8 +1411,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`readlink`", reject_with)?;
let eacc = this.eval_libc("EACCES");
this.set_last_error(eacc)?;
this.set_last_error(LibcError("EACCES"))?;
return interp_ok(-1);
}

Expand Down Expand Up @@ -1574,9 +1548,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`mkstemp`", reject_with)?;
let eacc = this.eval_libc("EACCES");
this.set_last_error(eacc)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("EACCES"));
}

// Get the bytes of the suffix we expect in _target_ encoding.
Expand All @@ -1592,8 +1564,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {

// If we don't find the suffix, it is an error.
if last_six_char_bytes != suffix_bytes {
this.set_last_error(LibcError("EINVAL"))?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("EINVAL"));
}

// At this point we know we have 6 ASCII 'X' characters as a suffix.
Expand Down Expand Up @@ -1658,17 +1629,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
_ => {
// "On error, -1 is returned, and errno is set to
// indicate the error"
this.set_last_error(e)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(e);
}
},
}
}

// We ran out of attempts to create the file, return an error.
let eexist = this.eval_libc("EEXIST");
this.set_last_error(eexist)?;
interp_ok(Scalar::from_i32(-1))
this.set_last_error_and_return_i32(LibcError("EEXIST"))
}
}

Expand Down
15 changes: 4 additions & 11 deletions src/shims/unix/linux/epoll.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,8 +266,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {

// Throw EINVAL if epfd and fd have the same value.
if epfd_value == fd {
this.set_last_error(LibcError("EINVAL"))?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("EINVAL"));
}

// Check if epfd is a valid epoll file descriptor.
Expand Down Expand Up @@ -332,15 +331,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Check the existence of fd in the interest list.
if op == epoll_ctl_add {
if interest_list.contains_key(&epoll_key) {
let eexist = this.eval_libc("EEXIST");
this.set_last_error(eexist)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("EEXIST"));
}
} else {
if !interest_list.contains_key(&epoll_key) {
let enoent = this.eval_libc("ENOENT");
this.set_last_error(enoent)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("ENOENT"));
}
}

Expand Down Expand Up @@ -374,9 +369,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {

// Remove epoll_event_interest from interest_list.
let Some(epoll_interest) = interest_list.remove(&epoll_key) else {
let enoent = this.eval_libc("ENOENT");
this.set_last_error(enoent)?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("ENOENT"));
};
// All related Weak<EpollEventInterest> will fail to upgrade after the drop.
drop(epoll_interest);
Expand Down
6 changes: 2 additions & 4 deletions src/shims/unix/mem.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// as a dealloc.
#[allow(clippy::arithmetic_side_effects)] // PAGE_SIZE is nonzero
if addr.addr().bytes() % this.machine.page_size != 0 {
this.set_last_error(this.eval_libc("EINVAL"))?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("EINVAL"));
}

let Some(length) = length.checked_next_multiple_of(this.machine.page_size) else {
this.set_last_error(this.eval_libc("EINVAL"))?;
return interp_ok(Scalar::from_i32(-1));
return this.set_last_error_and_return_i32(LibcError("EINVAL"));
};
if length > this.target_usize_max() {
this.set_last_error(this.eval_libc("EINVAL"))?;
Expand Down

0 comments on commit f026d57

Please sign in to comment.