From 17f6d2e83f05be30947ea279fa3f864c48eca6ec Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Wed, 31 Jul 2024 15:12:25 +0000 Subject: [PATCH 1/6] Handle writing to dest within the foreign item handler --- src/shims/unix/env.rs | 5 ++--- src/shims/unix/foreign_items.rs | 3 +-- src/shims/unix/fs.rs | 26 +++++++++++++++++--------- src/shims/unix/linux/foreign_items.rs | 18 +++++++----------- src/shims/unix/linux/mem.rs | 9 +++++---- src/shims/unix/mem.rs | 21 ++++++++++++++------- 6 files changed, 46 insertions(+), 36 deletions(-) diff --git a/src/shims/unix/env.rs b/src/shims/unix/env.rs index 324607cc1e..8c2f33e137 100644 --- a/src/shims/unix/env.rs +++ b/src/shims/unix/env.rs @@ -282,7 +282,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Ok(Scalar::from_u32(this.get_pid())) } - fn linux_gettid(&mut self) -> InterpResult<'tcx, Scalar> { + fn linux_gettid(&mut self, dest: &MPlaceTy<'tcx>) -> InterpResult<'tcx> { let this = self.eval_context_ref(); this.assert_target_os("linux", "gettid"); @@ -290,7 +290,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Compute a TID for this thread, ensuring that the main thread has PID == TID. let tid = this.get_pid().strict_add(index); - - Ok(Scalar::from_u32(tid)) + self.eval_context_mut().write_int(tid, dest) } } diff --git a/src/shims/unix/foreign_items.rs b/src/shims/unix/foreign_items.rs index c06ce57e61..61465fbad9 100644 --- a/src/shims/unix/foreign_items.rs +++ b/src/shims/unix/foreign_items.rs @@ -327,8 +327,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { "mmap" => { let [addr, length, prot, flags, fd, offset] = this.check_shim(abi, Abi::C {unwind: false}, link_name, args)?; let offset = this.read_scalar(offset)?.to_int(this.libc_ty_layout("off_t").size)?; - let ptr = this.mmap(addr, length, prot, flags, fd, offset)?; - this.write_scalar(ptr, dest)?; + this.mmap(addr, length, prot, flags, fd, offset, dest)?; } "munmap" => { let [addr, length] = this.check_shim(abi, Abi::C {unwind: false}, link_name, args)?; diff --git a/src/shims/unix/fs.rs b/src/shims/unix/fs.rs index 1b657db5cc..bc09fba2d4 100644 --- a/src/shims/unix/fs.rs +++ b/src/shims/unix/fs.rs @@ -1038,7 +1038,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } } - fn linux_readdir64(&mut self, dirp_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> { + fn linux_readdir64( + &mut self, + dirp_op: &OpTy<'tcx>, + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); this.assert_target_os("linux", "readdir64"); @@ -1050,7 +1054,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.reject_in_isolation("`readdir`", reject_with)?; let eacc = this.eval_libc("EBADF"); this.set_last_error(eacc)?; - return Ok(Scalar::null_ptr(this)); + return this.write_scalar(eacc, dest); } let open_dir = this.machine.dirs.streams.get_mut(&dirp).ok_or_else(|| { @@ -1128,7 +1132,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.deallocate_ptr(old_entry, None, MiriMemoryKind::Runtime.into())?; } - Ok(Scalar::from_maybe_pointer(entry.unwrap_or_else(Pointer::null), this)) + this.write_pointer(entry.unwrap_or_else(Pointer::null), dest) } fn macos_fbsd_readdir_r( @@ -1378,7 +1382,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { offset_op: &OpTy<'tcx>, nbytes_op: &OpTy<'tcx>, flags_op: &OpTy<'tcx>, - ) -> InterpResult<'tcx, Scalar> { + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); let fd = this.read_scalar(fd_op)?.to_i32()?; @@ -1389,7 +1394,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if offset < 0 || nbytes < 0 { let einval = this.eval_libc("EINVAL"); this.set_last_error(einval)?; - return Ok(Scalar::from_i32(-1)); + return this.write_int(-1, dest); } let allowed_flags = this.eval_libc_i32("SYNC_FILE_RANGE_WAIT_BEFORE") | this.eval_libc_i32("SYNC_FILE_RANGE_WRITE") @@ -1397,18 +1402,20 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if flags & allowed_flags != flags { let einval = this.eval_libc("EINVAL"); this.set_last_error(einval)?; - return Ok(Scalar::from_i32(-1)); + return this.write_int(-1, dest); } // Reject if isolation is enabled. 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 Ok(Scalar::from_i32(this.fd_not_found()?)); + let i: i32 = this.fd_not_found()?; + return this.write_int(i, dest); } let Some(fd) = this.machine.fds.get(fd) else { - return Ok(Scalar::from_i32(this.fd_not_found()?)); + let i: i32 = this.fd_not_found()?; + return this.write_int(i, dest); }; // Only regular files support synchronization. let FileHandle { file, writable } = fd.downcast::().ok_or_else(|| { @@ -1416,7 +1423,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { })?; let io_result = maybe_sync_file(file, *writable, File::sync_data); drop(fd); - Ok(Scalar::from_i32(this.try_unwrap_io_result(io_result)?)) + let i: i32 = this.try_unwrap_io_result(io_result)?; + this.write_int(i, dest) } fn readlink( diff --git a/src/shims/unix/linux/foreign_items.rs b/src/shims/unix/linux/foreign_items.rs index 07527a9d6e..c3941f0492 100644 --- a/src/shims/unix/linux/foreign_items.rs +++ b/src/shims/unix/linux/foreign_items.rs @@ -31,14 +31,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // File related shims "readdir64" => { let [dirp] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.linux_readdir64(dirp)?; - this.write_scalar(result, dest)?; + this.linux_readdir64(dirp, dest)?; } "sync_file_range" => { let [fd, offset, nbytes, flags] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.sync_file_range(fd, offset, nbytes, flags)?; - this.write_scalar(result, dest)?; + this.sync_file_range(fd, offset, nbytes, flags, dest)?; } "statx" => { let [dirfd, pathname, flags, mask, statxbuf] = @@ -95,12 +93,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } "gettid" => { let [] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.linux_gettid()?; - this.write_scalar(result, dest)?; + this.linux_gettid(dest)?; } // Dynamically invoked syscalls "syscall" => { + // TODO pull this match arm into a method // We do not use `check_shim` here because `syscall` is variadic. The argument // count is checked bellow. this.check_abi_and_shim_symbol_clash(abi, Abi::C { unwind: false }, link_name)?; @@ -158,19 +156,17 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let [addr, length, prot, flags, fd, offset] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; let offset = this.read_scalar(offset)?.to_i64()?; - let ptr = this.mmap(addr, length, prot, flags, fd, offset.into())?; - this.write_scalar(ptr, dest)?; + this.mmap(addr, length, prot, flags, fd, offset.into(), dest)?; } "mremap" => { let [old_address, old_size, new_size, flags] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let ptr = this.mremap(old_address, old_size, new_size, flags)?; - this.write_scalar(ptr, dest)?; + this.mremap(old_address, old_size, new_size, flags, dest)?; } "__errno_location" => { let [] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; let errno_place = this.last_error_place()?; - this.write_scalar(errno_place.to_ref(this).to_scalar(), dest)?; + this.write_immediate(errno_place.to_ref(this), dest)?; } "__libc_current_sigrtmin" => { let [] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; diff --git a/src/shims/unix/linux/mem.rs b/src/shims/unix/linux/mem.rs index 3b32612e8b..de1e5a8819 100644 --- a/src/shims/unix/linux/mem.rs +++ b/src/shims/unix/linux/mem.rs @@ -12,7 +12,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { old_size: &OpTy<'tcx>, new_size: &OpTy<'tcx>, flags: &OpTy<'tcx>, - ) -> InterpResult<'tcx, Scalar> { + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); let old_address = this.read_pointer(old_address)?; @@ -24,7 +25,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { #[allow(clippy::arithmetic_side_effects)] // PAGE_SIZE is nonzero if old_address.addr().bytes() % this.machine.page_size != 0 || new_size == 0 { this.set_last_error(this.eval_libc("EINVAL"))?; - return Ok(this.eval_libc("MAP_FAILED")); + return this.write_scalar(this.eval_libc("MAP_FAILED"), dest); } if flags & this.eval_libc_i32("MREMAP_FIXED") != 0 { @@ -38,7 +39,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if flags & this.eval_libc_i32("MREMAP_MAYMOVE") == 0 { // We only support MREMAP_MAYMOVE, so not passing the flag is just a failure this.set_last_error(this.eval_libc("EINVAL"))?; - return Ok(this.eval_libc("MAP_FAILED")); + return this.write_scalar(this.eval_libc("MAP_FAILED"), dest); } let align = this.machine.page_align(); @@ -59,6 +60,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { .unwrap(); } - Ok(Scalar::from_pointer(ptr, this)) + this.write_pointer(ptr, dest) } } diff --git a/src/shims/unix/mem.rs b/src/shims/unix/mem.rs index 33ed0e2698..bad64a2306 100644 --- a/src/shims/unix/mem.rs +++ b/src/shims/unix/mem.rs @@ -27,7 +27,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { flags: &OpTy<'tcx>, fd: &OpTy<'tcx>, offset: i128, - ) -> InterpResult<'tcx, Scalar> { + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx, EmulateItemResult> { let this = self.eval_context_mut(); // We do not support MAP_FIXED, so the addr argument is always ignored (except for the MacOS hack) @@ -48,7 +49,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { && matches!(&*this.tcx.sess.target.os, "macos" | "solaris" | "illumos") && (flags & map_fixed) != 0 { - return Ok(Scalar::from_maybe_pointer(Pointer::from_addr_invalid(addr), this)); + this.write_pointer(Pointer::from_addr_invalid(addr), dest)?; + return Ok(EmulateItemResult::NeedsReturn); } let prot_read = this.eval_libc_i32("PROT_READ"); @@ -57,11 +59,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // First, we do some basic argument validation as required by mmap if (flags & (map_private | map_shared)).count_ones() != 1 { this.set_last_error(this.eval_libc("EINVAL"))?; - return Ok(this.eval_libc("MAP_FAILED")); + this.write_scalar(this.eval_libc("MAP_FAILED"), dest)?; + return Ok(EmulateItemResult::NeedsReturn); } if length == 0 { this.set_last_error(this.eval_libc("EINVAL"))?; - return Ok(this.eval_libc("MAP_FAILED")); + this.write_scalar(this.eval_libc("MAP_FAILED"), dest)?; + return Ok(EmulateItemResult::NeedsReturn); } // If a user tries to map a file, we want to loudly inform them that this is not going @@ -103,11 +107,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let align = this.machine.page_align(); let Some(map_length) = length.checked_next_multiple_of(this.machine.page_size) else { this.set_last_error(this.eval_libc("EINVAL"))?; - return Ok(this.eval_libc("MAP_FAILED")); + this.write_scalar(this.eval_libc("MAP_FAILED"), dest)?; + return Ok(EmulateItemResult::NeedsReturn); }; if map_length > this.target_usize_max() { this.set_last_error(this.eval_libc("EINVAL"))?; - return Ok(this.eval_libc("MAP_FAILED")); + this.write_scalar(this.eval_libc("MAP_FAILED"), dest)?; + return Ok(EmulateItemResult::NeedsReturn); } let ptr = @@ -120,7 +126,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) .unwrap(); - Ok(Scalar::from_pointer(ptr, this)) + this.write_pointer(ptr, dest)?; + Ok(EmulateItemResult::NeedsReturn) } fn munmap(&mut self, addr: &OpTy<'tcx>, length: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> { From 7dd5651c03f6e11277290585f3eec5d2b71ba5be Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Thu, 1 Aug 2024 10:11:03 +0000 Subject: [PATCH 2/6] Move `linux_statx` to writing to `dest` directly --- src/shims/unix/fs.rs | 11 ++++++----- src/shims/unix/linux/foreign_items.rs | 3 +-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/shims/unix/fs.rs b/src/shims/unix/fs.rs index bc09fba2d4..4a85f26576 100644 --- a/src/shims/unix/fs.rs +++ b/src/shims/unix/fs.rs @@ -749,7 +749,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { flags_op: &OpTy<'tcx>, // Should be an `int` mask_op: &OpTy<'tcx>, // Should be an `unsigned int` statxbuf_op: &OpTy<'tcx>, // Should be a `struct statx *` - ) -> InterpResult<'tcx, Scalar> { + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); this.assert_target_os("linux", "statx"); @@ -764,7 +765,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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 Ok(Scalar::from_i32(-1)); + return this.write_int(-1, dest); } let statxbuf = this.deref_pointer_as(statxbuf_op, this.libc_ty_layout("statx"))?; @@ -806,7 +807,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.eval_libc("EBADF") }; this.set_last_error(ecode)?; - return Ok(Scalar::from_i32(-1)); + return this.write_int(-1, dest); } // the `_mask_op` parameter specifies the file information that the caller requested. @@ -828,7 +829,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; let metadata = match metadata { Some(metadata) => metadata, - None => return Ok(Scalar::from_i32(-1)), + None => return this.write_int(-1, dest), }; // The `mode` field specifies the type of the file and the permissions over the file for @@ -921,7 +922,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { &this.project_field_named(&statxbuf, "stx_mtime")?, )?; - Ok(Scalar::from_i32(0)) + this.write_null(dest) } fn rename( diff --git a/src/shims/unix/linux/foreign_items.rs b/src/shims/unix/linux/foreign_items.rs index c3941f0492..f40437d94b 100644 --- a/src/shims/unix/linux/foreign_items.rs +++ b/src/shims/unix/linux/foreign_items.rs @@ -41,8 +41,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { "statx" => { let [dirfd, pathname, flags, mask, statxbuf] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.linux_statx(dirfd, pathname, flags, mask, statxbuf)?; - this.write_scalar(result, dest)?; + this.linux_statx(dirfd, pathname, flags, mask, statxbuf, dest)?; } // epoll, eventfd From 7981e3f034c493c66f041a016ec1b2c56a0845d3 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Thu, 1 Aug 2024 10:52:30 +0000 Subject: [PATCH 3/6] Add helpers for setting errors and writing -1 --- src/helpers.rs | 22 ++++++++++++++++++++++ src/shims/unix/fs.rs | 31 ++++++++++--------------------- src/shims/unix/linux/sync.rs | 19 ++++--------------- 3 files changed, 36 insertions(+), 36 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 10e5882b5b..9b26469964 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -775,6 +775,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_scalar(scalar, &errno_place) } + fn set_libc_err_and_return_neg1( + &mut self, + err: &str, + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { + let err = self.eval_libc(err); + self.set_last_error(err)?; + self.eval_context_mut().write_int(-1, dest) + } + /// Gets the last error variable. fn get_last_error(&mut self) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); @@ -846,6 +856,18 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { self.set_last_error(self.io_error_to_errnum(err)?) } + /// Helper function that consumes an `std::io::Result` and writes the `Ok` integer + /// to the destination. In case the result is an error, this function writes + /// `-1` and sets the last OS error accordingly. + fn write_io_result( + &mut self, + result: std::io::Result, + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { + let res = self.try_unwrap_io_result(result)?; + self.eval_context_mut().write_int(res, dest) + } + /// Helper function that consumes an `std::io::Result` and returns an /// `InterpResult<'tcx,T>::Ok` instead. In case the result is an error, this function returns /// `Ok(-1)` and sets the last OS error accordingly. diff --git a/src/shims/unix/fs.rs b/src/shims/unix/fs.rs index 4a85f26576..fb8f996363 100644 --- a/src/shims/unix/fs.rs +++ b/src/shims/unix/fs.rs @@ -763,9 +763,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 this.write_int(-1, dest); + return this.set_libc_err_and_return_neg1("EFAULT", dest); } let statxbuf = this.deref_pointer_as(statxbuf_op, this.libc_ty_layout("statx"))?; @@ -795,19 +793,18 @@ 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("`statx`", reject_with)?; - let ecode = if path.is_absolute() || dirfd == this.eval_libc_i32("AT_FDCWD") { + let ename = if path.is_absolute() || dirfd == this.eval_libc_i32("AT_FDCWD") { // since `path` is provided, either absolute or // relative to CWD, `EACCES` is the most relevant. - this.eval_libc("EACCES") + "EACCES" } else { // `dirfd` is set to target file, and `path` is empty // (or we would have hit the `throw_unsup_format` // above). `EACCES` would violate the spec. assert!(empty_path_flag); - this.eval_libc("EBADF") + "EBADF" }; - this.set_last_error(ecode)?; - return this.write_int(-1, dest); + return this.set_libc_err_and_return_neg1(ename, dest); } // the `_mask_op` parameter specifies the file information that the caller requested. @@ -1393,30 +1390,23 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let flags = this.read_scalar(flags_op)?.to_i32()?; if offset < 0 || nbytes < 0 { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; - return this.write_int(-1, dest); + return this.set_libc_err_and_return_neg1("EINVAL", dest); } 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 { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; - return this.write_int(-1, dest); + return this.set_libc_err_and_return_neg1("EINVAL", dest); } // Reject if isolation is enabled. 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) - let i: i32 = this.fd_not_found()?; - return this.write_int(i, dest); + return this.set_libc_err_and_return_neg1("EBADF", dest); } let Some(fd) = this.machine.fds.get(fd) else { - let i: i32 = this.fd_not_found()?; - return this.write_int(i, dest); + return this.set_libc_err_and_return_neg1("EBADF", dest); }; // Only regular files support synchronization. let FileHandle { file, writable } = fd.downcast::().ok_or_else(|| { @@ -1424,8 +1414,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { })?; let io_result = maybe_sync_file(file, *writable, File::sync_data); drop(fd); - let i: i32 = this.try_unwrap_io_result(io_result)?; - this.write_int(i, dest) + this.write_io_result(io_result, dest) } fn readlink( diff --git a/src/shims/unix/linux/sync.rs b/src/shims/unix/linux/sync.rs index bd21203907..6227797113 100644 --- a/src/shims/unix/linux/sync.rs +++ b/src/shims/unix/linux/sync.rs @@ -75,10 +75,7 @@ pub fn futex<'tcx>( }; if bitset == 0 { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; - this.write_scalar(Scalar::from_target_isize(-1, this), dest)?; - return Ok(()); + return this.set_libc_err_and_return_neg1("EINVAL", dest); } let timeout = this.deref_pointer_as(&args[3], this.libc_ty_layout("timespec"))?; @@ -88,10 +85,7 @@ pub fn futex<'tcx>( let duration = match this.read_timespec(&timeout)? { Some(duration) => duration, None => { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; - this.write_scalar(Scalar::from_target_isize(-1, this), dest)?; - return Ok(()); + return this.set_libc_err_and_return_neg1("EINVAL", dest); } }; let timeout_clock = if op & futex_realtime == futex_realtime { @@ -172,9 +166,7 @@ pub fn futex<'tcx>( } else { // The futex value doesn't match the expected value, so we return failure // right away without sleeping: -1 and errno set to EAGAIN. - let eagain = this.eval_libc("EAGAIN"); - this.set_last_error(eagain)?; - this.write_scalar(Scalar::from_target_isize(-1, this), dest)?; + this.set_libc_err_and_return_neg1("EAGAIN", dest)?; } } // FUTEX_WAKE: (int *addr, int op = FUTEX_WAKE, int val) @@ -198,10 +190,7 @@ pub fn futex<'tcx>( u32::MAX }; if bitset == 0 { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; - this.write_scalar(Scalar::from_target_isize(-1, this), dest)?; - return Ok(()); + return this.set_libc_err_and_return_neg1("EINVAL", dest); } // 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 From ec4bef5cc963337a1505c39347d79ecb44cc299e Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Sat, 28 Sep 2024 13:27:18 +0200 Subject: [PATCH 4/6] Don't create a scalar from an int just to write it again, write the int directly --- src/shims/unix/linux/foreign_items.rs | 2 +- src/shims/unix/linux/sync.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shims/unix/linux/foreign_items.rs b/src/shims/unix/linux/foreign_items.rs index f40437d94b..2fde3f2c5a 100644 --- a/src/shims/unix/linux/foreign_items.rs +++ b/src/shims/unix/linux/foreign_items.rs @@ -135,7 +135,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let _flags = this.read_scalar(&args[3])?.to_i32(); this.gen_random(ptr, len)?; - this.write_scalar(Scalar::from_target_usize(len, this), dest)?; + this.write_int(len, dest)?; } // `futex` is used by some synchronization primitives. id if id == sys_futex => { diff --git a/src/shims/unix/linux/sync.rs b/src/shims/unix/linux/sync.rs index 6227797113..c931efe8f5 100644 --- a/src/shims/unix/linux/sync.rs +++ b/src/shims/unix/linux/sync.rs @@ -205,7 +205,7 @@ pub fn futex<'tcx>( break; } } - this.write_scalar(Scalar::from_target_isize(n, this), dest)?; + this.write_int(n, dest)?; } op => throw_unsup_format!("Miri does not support `futex` syscall with op={}", op), } From c4d5754be76b18a26f126297a3038737b750d45d Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Thu, 1 Aug 2024 11:31:53 +0000 Subject: [PATCH 5/6] Move some macos functions to writing to dest directly --- src/shims/unix/freebsd/foreign_items.rs | 12 ++---- src/shims/unix/fs.rs | 57 +++++++++++++------------ src/shims/unix/macos/foreign_items.rs | 12 ++---- 3 files changed, 37 insertions(+), 44 deletions(-) diff --git a/src/shims/unix/freebsd/foreign_items.rs b/src/shims/unix/freebsd/foreign_items.rs index 36f25767a8..8c1208fc16 100644 --- a/src/shims/unix/freebsd/foreign_items.rs +++ b/src/shims/unix/freebsd/foreign_items.rs @@ -48,25 +48,21 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { "stat" | "stat@FBSD_1.0" => { let [path, buf] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.macos_fbsd_stat(path, buf)?; - this.write_scalar(result, dest)?; + this.macos_fbsd_stat(path, buf, dest)?; } "lstat" | "lstat@FBSD_1.0" => { let [path, buf] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.macos_fbsd_lstat(path, buf)?; - this.write_scalar(result, dest)?; + this.macos_fbsd_lstat(path, buf, dest)?; } "fstat" | "fstat@FBSD_1.0" => { let [fd, buf] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.macos_fbsd_fstat(fd, buf)?; - this.write_scalar(result, dest)?; + this.macos_fbsd_fstat(fd, buf, dest)?; } "readdir_r" | "readdir_r@FBSD_1.0" => { let [dirp, entry, result] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.macos_fbsd_readdir_r(dirp, entry, result)?; - this.write_scalar(result, dest)?; + this.macos_fbsd_readdir_r(dirp, entry, result, dest)?; } // Miscellaneous diff --git a/src/shims/unix/fs.rs b/src/shims/unix/fs.rs index fb8f996363..f2b16b7ca0 100644 --- a/src/shims/unix/fs.rs +++ b/src/shims/unix/fs.rs @@ -657,7 +657,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { &mut self, path_op: &OpTy<'tcx>, buf_op: &OpTy<'tcx>, - ) -> InterpResult<'tcx, Scalar> { + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); if !matches!(&*this.tcx.sess.target.os, "macos" | "freebsd") { @@ -670,18 +671,16 @@ 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 Ok(Scalar::from_i32(-1)); + return this.set_libc_err_and_return_neg1("EACCES", dest); } // `stat` always follows symlinks. let metadata = match FileMetadata::from_path(this, &path, true)? { Some(metadata) => metadata, - None => return Ok(Scalar::from_i32(-1)), // `FileMetadata` has set errno + None => return this.write_int(-1, dest), // `FileMetadata` has set errno }; - - Ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?)) + let res = this.macos_stat_write_buf(metadata, buf_op)?; + this.write_int(res, dest) } // `lstat` is used to get symlink metadata. @@ -689,7 +688,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { &mut self, path_op: &OpTy<'tcx>, buf_op: &OpTy<'tcx>, - ) -> InterpResult<'tcx, Scalar> { + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); if !matches!(&*this.tcx.sess.target.os, "macos" | "freebsd") { @@ -702,24 +702,23 @@ 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 Ok(Scalar::from_i32(-1)); + return this.set_libc_err_and_return_neg1("EACCES", dest); } let metadata = match FileMetadata::from_path(this, &path, false)? { Some(metadata) => metadata, - None => return Ok(Scalar::from_i32(-1)), // `FileMetadata` has set errno + None => return this.write_int(-1, dest), // `FileMetadata` has set errno }; - - Ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?)) + let res = this.macos_stat_write_buf(metadata, buf_op)?; + this.write_int(res, dest) } fn macos_fbsd_fstat( &mut self, fd_op: &OpTy<'tcx>, buf_op: &OpTy<'tcx>, - ) -> InterpResult<'tcx, Scalar> { + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); if !matches!(&*this.tcx.sess.target.os, "macos" | "freebsd") { @@ -731,15 +730,15 @@ 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("`fstat`", reject_with)?; - // Set error code as "EBADF" (bad fd) - return Ok(Scalar::from_i32(this.fd_not_found()?)); + return this.set_libc_err_and_return_neg1("EBADF", dest); } let metadata = match FileMetadata::from_fd_num(this, fd)? { Some(metadata) => metadata, - None => return Ok(Scalar::from_i32(-1)), + None => return this.write_int(-1, dest), // `FileMetadata` has set errno }; - Ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?)) + let res = this.macos_stat_write_buf(metadata, buf_op)?; + this.write_int(res, dest) } fn linux_statx( @@ -1138,7 +1137,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { dirp_op: &OpTy<'tcx>, entry_op: &OpTy<'tcx>, result_op: &OpTy<'tcx>, - ) -> InterpResult<'tcx, Scalar> { + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); if !matches!(&*this.tcx.sess.target.os, "macos" | "freebsd") { @@ -1150,14 +1150,14 @@ 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("`readdir_r`", reject_with)?; - // Set error code as "EBADF" (bad fd) - return Ok(Scalar::from_i32(this.fd_not_found()?)); + // return positive error number on error + return this.write_scalar(this.eval_libc("EBADF"), dest); } let open_dir = this.machine.dirs.streams.get_mut(&dirp).ok_or_else(|| { err_unsup_format!("the DIR pointer passed to readdir_r did not come from opendir") })?; - Ok(Scalar::from_i32(match open_dir.read_dir.next() { + match open_dir.read_dir.next() { Some(Ok(dir_entry)) => { // Write into entry, write pointer to result, return 0 on success. // The name is written with write_os_str_to_c_str, while the rest of the @@ -1234,18 +1234,17 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let result_place = this.deref_pointer(result_op)?; this.write_scalar(this.read_scalar(entry_op)?, &result_place)?; - - 0 + this.write_null(dest)?; } None => { // end of stream: return 0, assign *result=NULL this.write_null(&this.deref_pointer(result_op)?)?; - 0 + this.write_null(dest)?; } Some(Err(e)) => match e.raw_os_error() { // return positive error number on error - Some(error) => error, + Some(error) => this.write_int(error, dest)?, None => { throw_unsup_format!( "the error {} couldn't be converted to a return value", @@ -1253,7 +1252,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) } }, - })) + } + + Ok(()) } fn closedir(&mut self, dirp_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> { diff --git a/src/shims/unix/macos/foreign_items.rs b/src/shims/unix/macos/foreign_items.rs index ce4ea0816f..903c113ee6 100644 --- a/src/shims/unix/macos/foreign_items.rs +++ b/src/shims/unix/macos/foreign_items.rs @@ -39,19 +39,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { "stat" | "stat64" | "stat$INODE64" => { let [path, buf] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.macos_fbsd_stat(path, buf)?; - this.write_scalar(result, dest)?; + this.macos_fbsd_stat(path, buf, dest)?; } "lstat" | "lstat64" | "lstat$INODE64" => { let [path, buf] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.macos_fbsd_lstat(path, buf)?; - this.write_scalar(result, dest)?; + this.macos_fbsd_lstat(path, buf, dest)?; } "fstat" | "fstat64" | "fstat$INODE64" => { let [fd, buf] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.macos_fbsd_fstat(fd, buf)?; - this.write_scalar(result, dest)?; + this.macos_fbsd_fstat(fd, buf, dest)?; } "opendir$INODE64" => { let [name] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; @@ -61,8 +58,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { "readdir_r" | "readdir_r$INODE64" => { let [dirp, entry, result] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.macos_fbsd_readdir_r(dirp, entry, result)?; - this.write_scalar(result, dest)?; + this.macos_fbsd_readdir_r(dirp, entry, result, dest)?; } "realpath$DARWIN_EXTSN" => { let [path, resolved_path] = From f162e510b9d2564542626d132107f912da83bb2f Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Thu, 1 Aug 2024 11:43:24 +0000 Subject: [PATCH 6/6] Write `-1` together with setting the global error value --- src/helpers.rs | 8 ++++++++ src/shims/unix/fs.rs | 29 ++++++++++++++--------------- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 9b26469964..b483e1017c 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -781,6 +781,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { dest: &MPlaceTy<'tcx>, ) -> InterpResult<'tcx> { let err = self.eval_libc(err); + self.set_last_err_and_return_neg1(err, dest) + } + + fn set_last_err_and_return_neg1( + &mut self, + err: Scalar, + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { self.set_last_error(err)?; self.eval_context_mut().write_int(-1, dest) } diff --git a/src/shims/unix/fs.rs b/src/shims/unix/fs.rs index f2b16b7ca0..abc14ef195 100644 --- a/src/shims/unix/fs.rs +++ b/src/shims/unix/fs.rs @@ -676,8 +676,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 this.write_int(-1, dest), // `FileMetadata` has set errno + Ok(metadata) => metadata, + Err(e) => return this.set_last_err_and_return_neg1(e, dest), }; let res = this.macos_stat_write_buf(metadata, buf_op)?; this.write_int(res, dest) @@ -706,8 +706,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } let metadata = match FileMetadata::from_path(this, &path, false)? { - Some(metadata) => metadata, - None => return this.write_int(-1, dest), // `FileMetadata` has set errno + Ok(metadata) => metadata, + Err(e) => return this.set_last_err_and_return_neg1(e, dest), }; let res = this.macos_stat_write_buf(metadata, buf_op)?; this.write_int(res, dest) @@ -734,8 +734,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } let metadata = match FileMetadata::from_fd_num(this, fd)? { - Some(metadata) => metadata, - None => return this.write_int(-1, dest), // `FileMetadata` has set errno + Ok(metadata) => metadata, + Err(e) => return this.set_last_err_and_return_neg1(e, dest), }; let res = this.macos_stat_write_buf(metadata, buf_op)?; this.write_int(res, dest) @@ -824,8 +824,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { FileMetadata::from_path(this, &path, follow_symlink)? }; let metadata = match metadata { - Some(metadata) => metadata, - None => return this.write_int(-1, dest), + Ok(metadata) => metadata, + Err(e) => return this.set_last_err_and_return_neg1(e, dest), }; // The `mode` field specifies the type of the file and the permissions over the file for @@ -1699,7 +1699,7 @@ impl FileMetadata { ecx: &mut MiriInterpCx<'tcx>, path: &Path, follow_symlink: bool, - ) -> InterpResult<'tcx, Option> { + ) -> InterpResult<'tcx, Result> { let metadata = if follow_symlink { std::fs::metadata(path) } else { std::fs::symlink_metadata(path) }; @@ -1709,9 +1709,9 @@ impl FileMetadata { fn from_fd_num<'tcx>( ecx: &mut MiriInterpCx<'tcx>, fd_num: i32, - ) -> InterpResult<'tcx, Option> { + ) -> InterpResult<'tcx, Result> { let Some(fd) = ecx.machine.fds.get(fd_num) else { - return ecx.fd_not_found().map(|_: i32| None); + return Ok(Err(ecx.eval_libc("EBADF"))); }; let file = &fd @@ -1731,12 +1731,11 @@ impl FileMetadata { fn from_meta<'tcx>( ecx: &mut MiriInterpCx<'tcx>, metadata: Result, - ) -> InterpResult<'tcx, Option> { + ) -> InterpResult<'tcx, Result> { let metadata = match metadata { Ok(metadata) => metadata, Err(e) => { - ecx.set_last_error_from_io_error(e)?; - return Ok(None); + return Ok(Err(ecx.io_error_to_errnum(e)?)); } }; @@ -1759,6 +1758,6 @@ impl FileMetadata { let modified = extract_sec_and_nsec(metadata.modified())?; // FIXME: Provide more fields using platform specific methods. - Ok(Some(FileMetadata { mode, size, created, accessed, modified })) + Ok(Ok(FileMetadata { mode, size, created, accessed, modified })) } }