From 9b61c05ff0897e1ab0f3d8708540fa0b0dc6015f Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sun, 1 Dec 2024 19:27:32 -0800 Subject: [PATCH 1/8] Refactor `Handle` slightly - consistent handling, invalid handles are always an abort - Helper for reading handles that does the checking and machine stop - Use real handle types more places - Adds `File` and `Invalid` types of handle. File support will be added next --- src/shims/windows/foreign_items.rs | 66 +++++++++++------------------- src/shims/windows/handle.rs | 55 +++++++++++++++++++++++-- src/shims/windows/thread.rs | 8 ++-- 3 files changed, 78 insertions(+), 51 deletions(-) diff --git a/src/shims/windows/foreign_items.rs b/src/shims/windows/foreign_items.rs index d6a180451d..b409784bff 100644 --- a/src/shims/windows/foreign_items.rs +++ b/src/shims/windows/foreign_items.rs @@ -7,14 +7,9 @@ use rustc_span::Symbol; use self::shims::windows::handle::{Handle, PseudoHandle}; use crate::shims::os_str::bytes_to_os_str; -use crate::shims::windows::handle::HandleError; use crate::shims::windows::*; use crate::*; -// The NTSTATUS STATUS_INVALID_HANDLE (0xC0000008) encoded as a HRESULT by setting the N bit. -// (https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/0642cb2f-2075-4469-918c-4441e69c548a) -const STATUS_INVALID_HANDLE: u32 = 0xD0000008; - pub fn is_dyn_sym(name: &str) -> bool { // std does dynamic detection for these symbols matches!( @@ -520,53 +515,38 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let [handle, name] = this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; - let handle = this.read_scalar(handle)?; + let handle = this.read_handle(handle)?; let name = this.read_wide_str(this.read_pointer(name)?)?; - let thread = match Handle::try_from_scalar(handle, this)? { - Ok(Handle::Thread(thread)) => Ok(thread), - Ok(Handle::Pseudo(PseudoHandle::CurrentThread)) => Ok(this.active_thread()), - Ok(_) | Err(HandleError::InvalidHandle) => - this.invalid_handle("SetThreadDescription")?, - Err(HandleError::ThreadNotFound(e)) => Err(e), - }; - let res = match thread { - Ok(thread) => { - // FIXME: use non-lossy conversion - this.set_thread_name(thread, String::from_utf16_lossy(&name).into_bytes()); - Scalar::from_u32(0) - } - Err(_) => Scalar::from_u32(STATUS_INVALID_HANDLE), + let thread = match handle { + Handle::Thread(thread) => thread, + Handle::Pseudo(PseudoHandle::CurrentThread) => this.active_thread(), + _ => this.invalid_handle("SetThreadDescription")?, }; - - this.write_scalar(res, dest)?; + // FIXME: use non-lossy conversion + this.set_thread_name(thread, String::from_utf16_lossy(&name).into_bytes()); + this.write_scalar(Scalar::from_u32(0), dest)?; } "GetThreadDescription" => { let [handle, name_ptr] = this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; - let handle = this.read_scalar(handle)?; + let handle = this.read_handle(handle)?; let name_ptr = this.deref_pointer(name_ptr)?; // the pointer where we should store the ptr to the name - let thread = match Handle::try_from_scalar(handle, this)? { - Ok(Handle::Thread(thread)) => Ok(thread), - Ok(Handle::Pseudo(PseudoHandle::CurrentThread)) => Ok(this.active_thread()), - Ok(_) | Err(HandleError::InvalidHandle) => - this.invalid_handle("GetThreadDescription")?, - Err(HandleError::ThreadNotFound(e)) => Err(e), - }; - let (name, res) = match thread { - Ok(thread) => { - // Looks like the default thread name is empty. - let name = this.get_thread_name(thread).unwrap_or(b"").to_owned(); - let name = this.alloc_os_str_as_wide_str( - bytes_to_os_str(&name)?, - MiriMemoryKind::WinLocal.into(), - )?; - (Scalar::from_maybe_pointer(name, this), Scalar::from_u32(0)) - } - Err(_) => (Scalar::null_ptr(this), Scalar::from_u32(STATUS_INVALID_HANDLE)), + let thread = match handle { + Handle::Thread(thread) => thread, + Handle::Pseudo(PseudoHandle::CurrentThread) => this.active_thread(), + _ => this.invalid_handle("GetThreadDescription")?, }; + // Looks like the default thread name is empty. + let name = this.get_thread_name(thread).unwrap_or(b"").to_owned(); + let name = this.alloc_os_str_as_wide_str( + bytes_to_os_str(&name)?, + MiriMemoryKind::WinLocal.into(), + )?; + let name = Scalar::from_maybe_pointer(name, this); + let res = Scalar::from_u32(0); this.write_scalar(name, &name_ptr)?; this.write_scalar(res, dest)?; @@ -667,11 +647,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; this.check_no_isolation("`GetModuleFileNameW`")?; - let handle = this.read_target_usize(handle)?; + let handle = this.read_handle(handle)?; let filename = this.read_pointer(filename)?; let size = this.read_scalar(size)?.to_u32()?; - if handle != 0 { + if handle != Handle::Null { throw_unsup_format!("`GetModuleFileNameW` only supports the NULL handle"); } diff --git a/src/shims/windows/handle.rs b/src/shims/windows/handle.rs index 3d872b65a6..8d684c9495 100644 --- a/src/shims/windows/handle.rs +++ b/src/shims/windows/handle.rs @@ -1,4 +1,5 @@ use std::mem::variant_count; +use std::panic::Location; use rustc_abi::HasDataLayout; @@ -16,6 +17,8 @@ pub enum Handle { Null, Pseudo(PseudoHandle), Thread(ThreadId), + File(i32), + Invalid, } impl PseudoHandle { @@ -47,12 +50,16 @@ impl Handle { const NULL_DISCRIMINANT: u32 = 0; const PSEUDO_DISCRIMINANT: u32 = 1; const THREAD_DISCRIMINANT: u32 = 2; + const FILE_DISCRIMINANT: u32 = 3; + const INVALID_DISCRIMINANT: u32 = 7; fn discriminant(self) -> u32 { match self { Self::Null => Self::NULL_DISCRIMINANT, Self::Pseudo(_) => Self::PSEUDO_DISCRIMINANT, Self::Thread(_) => Self::THREAD_DISCRIMINANT, + Self::File(_) => Self::FILE_DISCRIMINANT, + Self::Invalid => Self::INVALID_DISCRIMINANT, } } @@ -61,11 +68,16 @@ impl Handle { Self::Null => 0, Self::Pseudo(pseudo_handle) => pseudo_handle.value(), Self::Thread(thread) => thread.to_u32(), + #[expect(clippy::cast_sign_loss)] + Self::File(fd) => fd as u32, + Self::Invalid => 0x1FFFFFFF, } } fn packed_disc_size() -> u32 { // ceil(log2(x)) is how many bits it takes to store x numbers + // We ensure that INVALID_HANDLE_VALUE (0xFFFFFFFF) decodes to Handle::Invalid + // see https://devblogs.microsoft.com/oldnewthing/20230914-00/?p=108766 let variant_count = variant_count::(); // however, std's ilog2 is floor(log2(x)) @@ -93,7 +105,7 @@ impl Handle { assert!(discriminant < 2u32.pow(disc_size)); // make sure the data fits into `data_size` bits - assert!(data < 2u32.pow(data_size)); + assert!(data <= 2u32.pow(data_size)); // packs the data into the lower `data_size` bits // and packs the discriminant right above the data @@ -105,6 +117,9 @@ impl Handle { Self::NULL_DISCRIMINANT if data == 0 => Some(Self::Null), Self::PSEUDO_DISCRIMINANT => Some(Self::Pseudo(PseudoHandle::from_value(data)?)), Self::THREAD_DISCRIMINANT => Some(Self::Thread(ThreadId::new_unchecked(data))), + #[expect(clippy::cast_possible_wrap)] + Self::FILE_DISCRIMINANT => Some(Self::File(data as i32)), + Self::INVALID_DISCRIMINANT => Some(Self::Invalid), _ => None, } } @@ -171,6 +186,26 @@ impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} #[allow(non_snake_case)] pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { + #[track_caller] + fn read_handle(&self, handle: &OpTy<'tcx>) -> InterpResult<'tcx, Handle> { + let this = self.eval_context_ref(); + let handle = this.read_scalar(handle)?; + match Handle::try_from_scalar(handle, this)? { + Ok(handle) => interp_ok(handle), + Err(HandleError::InvalidHandle) => + throw_machine_stop!(TerminationInfo::Abort(format!( + "invalid handle {} at {}", + handle.to_target_isize(this)?, + Location::caller(), + ))), + Err(HandleError::ThreadNotFound(_)) => + throw_machine_stop!(TerminationInfo::Abort(format!( + "invalid thread ID: {}", + Location::caller() + ))), + } + } + fn invalid_handle(&mut self, function_name: &str) -> InterpResult<'tcx, !> { throw_machine_stop!(TerminationInfo::Abort(format!( "invalid handle passed to `{function_name}`" @@ -180,12 +215,24 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn CloseHandle(&mut self, handle_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let handle = this.read_scalar(handle_op)?; - let ret = match Handle::try_from_scalar(handle, this)? { - Ok(Handle::Thread(thread)) => { + let handle = this.read_handle(handle_op)?; + let ret = match handle { + Handle::Thread(thread) => { this.detach_thread(thread, /*allow_terminated_joined*/ true)?; this.eval_windows("c", "TRUE") } + Handle::File(fd) => + if let Some(file) = this.machine.fds.get(fd) { + let err = file.close(this.machine.communicate(), this)?; + if let Err(e) = err { + this.set_last_error(e)?; + this.eval_windows("c", "FALSE") + } else { + this.eval_windows("c", "TRUE") + } + } else { + this.invalid_handle("CloseHandle")? + }, _ => this.invalid_handle("CloseHandle")?, }; diff --git a/src/shims/windows/thread.rs b/src/shims/windows/thread.rs index efc1c2286b..2c4d5c2297 100644 --- a/src/shims/windows/thread.rs +++ b/src/shims/windows/thread.rs @@ -62,14 +62,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let handle = this.read_scalar(handle_op)?; + let handle = this.read_handle(handle_op)?; let timeout = this.read_scalar(timeout_op)?.to_u32()?; - let thread = match Handle::try_from_scalar(handle, this)? { - Ok(Handle::Thread(thread)) => thread, + let thread = match handle { + Handle::Thread(thread) => thread, // Unlike on posix, the outcome of joining the current thread is not documented. // On current Windows, it just deadlocks. - Ok(Handle::Pseudo(PseudoHandle::CurrentThread)) => this.active_thread(), + Handle::Pseudo(PseudoHandle::CurrentThread) => this.active_thread(), _ => this.invalid_handle("WaitForSingleObject")?, }; From ab0e13896048f385222b3959905fdac1ed50588d Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sun, 1 Dec 2024 19:33:41 -0800 Subject: [PATCH 2/8] Implement trivial file operations - opening and closing handles. Just enough to get file metadata. --- src/shims/windows/foreign_items.rs | 27 ++ src/shims/windows/fs.rs | 392 +++++++++++++++++++++++++++++ src/shims/windows/mod.rs | 2 + tests/pass/shims/fs.rs | 39 +-- 4 files changed, 441 insertions(+), 19 deletions(-) create mode 100644 src/shims/windows/fs.rs diff --git a/src/shims/windows/foreign_items.rs b/src/shims/windows/foreign_items.rs index b409784bff..7e7cb69e8a 100644 --- a/src/shims/windows/foreign_items.rs +++ b/src/shims/windows/foreign_items.rs @@ -241,6 +241,33 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; this.write_scalar(result, dest)?; } + "CreateFileW" => { + let [ + file_name, + desired_access, + share_mode, + security_attributes, + creation_disposition, + flags_and_attributes, + template_file, + ] = this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; + let handle = this.CreateFileW( + file_name, + desired_access, + share_mode, + security_attributes, + creation_disposition, + flags_and_attributes, + template_file, + )?; + this.write_scalar(handle.to_scalar(this), dest)?; + } + "GetFileInformationByHandle" => { + let [handle, info] = + this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; + let res = this.GetFileInformationByHandle(handle, info)?; + this.write_scalar(res, dest)?; + } // Allocation "HeapAlloc" => { diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs new file mode 100644 index 0000000000..5eea09cd0e --- /dev/null +++ b/src/shims/windows/fs.rs @@ -0,0 +1,392 @@ +use std::fs::{File, Metadata, OpenOptions}; +use std::io; +use std::io::{IsTerminal, Read, Seek, SeekFrom, Write}; +use std::path::{Path, PathBuf}; +use std::time::SystemTime; + +use rustc_abi::Size; + +use crate::shims::files::{EvalContextExt as _, FileDescription, FileDescriptionRef}; +use crate::shims::time::system_time_to_duration; +use crate::shims::windows::handle::{EvalContextExt as _, Handle}; +use crate::*; + +#[derive(Debug)] +pub struct FileHandle { + pub(crate) file: File, + pub(crate) writable: bool, +} + +impl FileDescription for FileHandle { + fn name(&self) -> &'static str { + "file" + } + + fn read<'tcx>( + &self, + _self_ref: &FileDescriptionRef, + communicate_allowed: bool, + ptr: Pointer, + len: usize, + dest: &MPlaceTy<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { + assert!(communicate_allowed, "isolation should have prevented even opening a file"); + let mut bytes = vec![0; len]; + let result = (&mut &self.file).read(&mut bytes); + match result { + Ok(read_size) => ecx.return_read_success(ptr, &bytes, read_size, dest), + Err(e) => ecx.set_last_error_and_return(e, dest), + } + } + + fn write<'tcx>( + &self, + _self_ref: &FileDescriptionRef, + communicate_allowed: bool, + ptr: Pointer, + len: usize, + dest: &MPlaceTy<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { + assert!(communicate_allowed, "isolation should have prevented even opening a file"); + let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; + let result = (&mut &self.file).write(bytes); + match result { + Ok(write_size) => ecx.return_write_success(write_size, dest), + Err(e) => ecx.set_last_error_and_return(e, dest), + } + } + + fn seek<'tcx>( + &self, + communicate_allowed: bool, + offset: SeekFrom, + ) -> InterpResult<'tcx, io::Result> { + assert!(communicate_allowed, "isolation should have prevented even opening a file"); + interp_ok((&mut &self.file).seek(offset)) + } + + fn close<'tcx>( + self: Box, + communicate_allowed: bool, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, io::Result<()>> { + assert!(communicate_allowed, "isolation should have prevented even opening a file"); + // We sync the file if it was opened in a mode different than read-only. + if self.writable { + // `File::sync_all` does the checks that are done when closing a file. We do this to + // 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 + // produced when closing it. This is done because + // `File::sync_all` cannot be done over files like + // `/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(())) + } + } + + fn metadata<'tcx>(&self) -> InterpResult<'tcx, io::Result> { + interp_ok(self.file.metadata()) + } + + fn is_tty(&self, communicate_allowed: bool) -> bool { + communicate_allowed && self.file.is_terminal() + } +} + +#[derive(Debug)] +pub struct DirHandle { + pub(crate) path: PathBuf, +} + +impl FileDescription for DirHandle { + fn name(&self) -> &'static str { + "directory" + } + + fn metadata<'tcx>(&self) -> InterpResult<'tcx, io::Result> { + interp_ok(self.path.metadata()) + } +} + +#[derive(Debug)] +pub struct MetadataHandle { + pub(crate) path: PathBuf, +} + +impl FileDescription for MetadataHandle { + fn name(&self) -> &'static str { + "metadata-only" + } + + fn metadata<'tcx>(&self) -> InterpResult<'tcx, io::Result> { + interp_ok(self.path.metadata()) + } +} + +impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} +#[allow(non_snake_case)] +pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { + fn CreateFileW( + &mut self, + file_name: &OpTy<'tcx>, // LPCWSTR + desired_access: &OpTy<'tcx>, // DWORD + share_mode: &OpTy<'tcx>, // DWORD + security_attributes: &OpTy<'tcx>, // LPSECURITY_ATTRIBUTES + creation_disposition: &OpTy<'tcx>, // DWORD + flags_and_attributes: &OpTy<'tcx>, // DWORD + template_file: &OpTy<'tcx>, // HANDLE + ) -> InterpResult<'tcx, Handle> { + // ^ Returns HANDLE + let this = self.eval_context_mut(); + this.assert_target_os("windows", "CreateFileW"); + this.check_no_isolation("`CreateFileW`")?; + + let file_name = + String::from_utf16_lossy(&this.read_wide_str(this.read_pointer(file_name)?)?); + let file_name = Path::new(&file_name); + let desired_access = this.read_scalar(desired_access)?.to_u32()?; + let share_mode = this.read_scalar(share_mode)?.to_u32()?; + let security_attributes = this.read_pointer(security_attributes)?; + let creation_disposition = this.read_scalar(creation_disposition)?.to_u32()?; + let flags_and_attributes = this.read_scalar(flags_and_attributes)?.to_u32()?; + let template_file = this.read_target_usize(template_file)?; + + let generic_read = this.eval_windows_u32("c", "GENERIC_READ"); + let generic_write = this.eval_windows_u32("c", "GENERIC_WRITE"); + + if desired_access & !(generic_read | generic_write) != 0 { + throw_unsup_format!("CreateFileW: Unsupported access mode: {desired_access}"); + } + + let file_share_delete = this.eval_windows_u32("c", "FILE_SHARE_DELETE"); + let file_share_read = this.eval_windows_u32("c", "FILE_SHARE_READ"); + let file_share_write = this.eval_windows_u32("c", "FILE_SHARE_WRITE"); + + if share_mode & !(file_share_delete | file_share_read | file_share_write) != 0 + || share_mode == 0 + { + throw_unsup_format!("CreateFileW: Unsupported share mode: {share_mode}"); + } + if !this.ptr_is_null(security_attributes)? { + throw_unsup_format!("CreateFileW: Security attributes are not supported"); + } + + let create_always = this.eval_windows_u32("c", "CREATE_ALWAYS"); + let create_new = this.eval_windows_u32("c", "CREATE_NEW"); + let open_always = this.eval_windows_u32("c", "OPEN_ALWAYS"); + let open_existing = this.eval_windows_u32("c", "OPEN_EXISTING"); + let truncate_existing = this.eval_windows_u32("c", "TRUNCATE_EXISTING"); + + if ![create_always, create_new, open_always, open_existing, truncate_existing] + .contains(&creation_disposition) + { + throw_unsup_format!( + "CreateFileW: Unsupported creation disposition: {creation_disposition}" + ); + } + + let file_attribute_normal = this.eval_windows_u32("c", "FILE_ATTRIBUTE_NORMAL"); + // This must be passed to allow getting directory handles. If not passed, we error on trying + // to open directories below + let file_flag_backup_semantics = this.eval_windows_u32("c", "FILE_FLAG_BACKUP_SEMANTICS"); + let file_flag_open_reparse_point = + this.eval_windows_u32("c", "FILE_FLAG_OPEN_REPARSE_POINT"); + + let flags_and_attributes = match flags_and_attributes { + 0 => file_attribute_normal, + _ => flags_and_attributes, + }; + if !(file_attribute_normal | file_flag_backup_semantics | file_flag_open_reparse_point) + & flags_and_attributes + != 0 + { + throw_unsup_format!( + "CreateFileW: Unsupported flags_and_attributes: {flags_and_attributes}" + ); + } + + if flags_and_attributes & file_flag_open_reparse_point != 0 + && creation_disposition == create_always + { + throw_machine_stop!(TerminationInfo::Abort("Invalid CreateFileW argument combination: FILE_FLAG_OPEN_REPARSE_POINT with CREATE_ALWAYS".to_string())); + } + + if template_file != 0 { + throw_unsup_format!("CreateFileW: Template files are not supported"); + } + + let desired_read = desired_access & generic_read != 0; + let desired_write = desired_access & generic_write != 0; + let exists = file_name.exists(); + let is_dir = file_name.is_dir(); + + if flags_and_attributes == file_attribute_normal && is_dir { + this.set_last_error(IoError::WindowsError("ERROR_ACCESS_DENIED"))?; + return interp_ok(Handle::Invalid); + } + + let mut options = OpenOptions::new(); + if desired_read { + options.read(true); + } + if desired_write { + options.write(true); + } + + if creation_disposition == create_always { + if file_name.exists() { + this.set_last_error(IoError::WindowsError("ERROR_ALREADY_EXISTS"))?; + } + options.create(true); + options.truncate(true); + } else if creation_disposition == create_new { + options.create_new(true); + if !desired_write { + options.append(true); + } + } else if creation_disposition == open_always { + if file_name.exists() { + this.set_last_error(IoError::WindowsError("ERROR_ALREADY_EXISTS"))?; + } + options.create(true); + } else if creation_disposition == open_existing { + // Nothing + } else if creation_disposition == truncate_existing { + options.truncate(true); + } + + let handle = if is_dir && exists { + let fh = &mut this.machine.fds; + let fd = fh.insert_new(DirHandle { path: file_name.into() }); + Ok(Handle::File(fd)) + } else if creation_disposition == open_existing && desired_access == 0 { + // Windows supports handles with no permissions. These allow things such as reading + // metadata, but not file content. + let fh = &mut this.machine.fds; + let fd = fh.insert_new(MetadataHandle { path: file_name.into() }); + Ok(Handle::File(fd)) + } else { + options.open(file_name).map(|file| { + let fh = &mut this.machine.fds; + let fd = fh.insert_new(FileHandle { file, writable: desired_write }); + Handle::File(fd) + }) + }; + + match handle { + Ok(handle) => interp_ok(handle), + Err(e) => { + this.set_last_error(e)?; + interp_ok(Handle::Invalid) + } + } + } + + fn GetFileInformationByHandle( + &mut self, + file: &OpTy<'tcx>, // HANDLE + file_information: &OpTy<'tcx>, // LPBY_HANDLE_FILE_INFORMATION + ) -> InterpResult<'tcx, Scalar> { + // ^ Returns BOOL (i32 on Windows) + let this = self.eval_context_mut(); + this.assert_target_os("windows", "GetFileInformationByHandle"); + this.check_no_isolation("`GetFileInformationByHandle`")?; + + let file = this.read_handle(file)?; + let file_information = this.deref_pointer_as( + file_information, + this.windows_ty_layout("BY_HANDLE_FILE_INFORMATION"), + )?; + + let fd = if let Handle::File(fd) = file { + fd + } else { + this.invalid_handle("GetFileInformationByHandle")? + }; + + let Some(desc) = this.machine.fds.get(fd) else { + this.invalid_handle("GetFileInformationByHandle")? + }; + + let metadata = match desc.metadata()? { + Ok(meta) => meta, + Err(e) => { + this.set_last_error(e)?; + return interp_ok(this.eval_windows("c", "FALSE")); + } + }; + + let size = metadata.len(); + + let file_type = metadata.file_type(); + let attributes = if file_type.is_dir() { + this.eval_windows_u32("c", "FILE_ATTRIBUTE_DIRECTORY") + } else if file_type.is_file() { + this.eval_windows_u32("c", "FILE_ATTRIBUTE_NORMAL") + } else { + this.eval_windows_u32("c", "FILE_ATTRIBUTE_DEVICE") + }; + + let created = extract_windows_epoch(metadata.created())?.unwrap_or((0, 0)); + let accessed = extract_windows_epoch(metadata.accessed())?.unwrap_or((0, 0)); + let written = extract_windows_epoch(metadata.modified())?.unwrap_or((0, 0)); + + this.write_int_fields_named(&[("dwFileAttributes", attributes.into())], &file_information)?; + write_filetime_field(this, &file_information, "ftCreationTime", created)?; + write_filetime_field(this, &file_information, "ftLastAccessTime", accessed)?; + write_filetime_field(this, &file_information, "ftLastWriteTime", written)?; + this.write_int_fields_named( + &[ + ("dwVolumeSerialNumber", 0), + ("nFileSizeHigh", (size >> 32).into()), + ("nFileSizeLow", (size & 0xFFFFFFFF).into()), + ("nNumberOfLinks", 1), + ("nFileIndexHigh", 0), + ("nFileIndexLow", 0), + ], + &file_information, + )?; + + interp_ok(this.eval_windows("c", "TRUE")) + } +} + +/// Windows FILETIME is measured in 100-nanosecs since 1601 +fn extract_windows_epoch<'tcx>( + time: io::Result, +) -> InterpResult<'tcx, Option<(u32, u32)>> { + // (seconds in a year) * (369 years between 1970 and 1601) * 10 million (nanoseconds/second / 100) + const TIME_TO_EPOCH: u64 = 31_556_926 * 369 * 10_000_000; + match time.ok() { + Some(time) => { + let duration = system_time_to_duration(&time)?; + let secs = duration.as_secs().saturating_mul(10_000_000); + let nanos_hundred: u64 = (duration.subsec_nanos() / 100).into(); + let total = secs.saturating_add(nanos_hundred).saturating_add(TIME_TO_EPOCH); + #[allow(clippy::cast_possible_truncation)] + interp_ok(Some((total as u32, (total >> 32) as u32))) + } + None => interp_ok(None), + } +} + +fn write_filetime_field<'tcx>( + cx: &mut MiriInterpCx<'tcx>, + val: &MPlaceTy<'tcx>, + name: &str, + (low, high): (u32, u32), +) -> InterpResult<'tcx> { + cx.write_int_fields_named( + &[("dwLowDateTime", low.into()), ("dwHighDateTime", high.into())], + &cx.project_field_named(val, name)?, + ) +} diff --git a/src/shims/windows/mod.rs b/src/shims/windows/mod.rs index 892bd6924f..442c5a0dd1 100644 --- a/src/shims/windows/mod.rs +++ b/src/shims/windows/mod.rs @@ -1,12 +1,14 @@ pub mod foreign_items; mod env; +mod fs; mod handle; mod sync; mod thread; // All the Windows-specific extension traits pub use self::env::{EvalContextExt as _, WindowsEnvVars}; +pub use self::fs::EvalContextExt as _; pub use self::handle::EvalContextExt as _; pub use self::sync::EvalContextExt as _; pub use self::thread::EvalContextExt as _; diff --git a/tests/pass/shims/fs.rs b/tests/pass/shims/fs.rs index 3e514d95ee..5f18ae7b5d 100644 --- a/tests/pass/shims/fs.rs +++ b/tests/pass/shims/fs.rs @@ -1,4 +1,3 @@ -//@ignore-target: windows # File handling is not implemented yet //@compile-flags: -Zmiri-disable-isolation #![feature(io_error_more)] @@ -18,23 +17,25 @@ mod utils; fn main() { test_path_conversion(); - test_file(); - test_file_clone(); - test_file_create_new(); - test_seek(); - test_metadata(); - test_file_set_len(); - test_file_sync(); - test_errors(); - test_rename(); - // solarish needs to support readdir/readdir64 for these tests. - if cfg!(not(any(target_os = "solaris", target_os = "illumos"))) { - test_directory(); - test_canonicalize(); + if cfg!(not(windows)) { + test_file(); + test_file_create_new(); + test_seek(); + test_file_clone(); + test_metadata(); + test_file_set_len(); + test_file_sync(); + test_errors(); + test_rename(); + // solarish needs to support readdir/readdir64 for these tests. + if cfg!(not(any(target_os = "solaris", target_os = "illumos"))) { + test_directory(); + test_canonicalize(); + } + test_from_raw_os_error(); + #[cfg(unix)] + test_pread_pwrite(); } - test_from_raw_os_error(); - #[cfg(unix)] - test_pread_pwrite(); } fn test_path_conversion() { @@ -147,10 +148,10 @@ fn test_metadata() { let path = utils::prepare_with_content("miri_test_fs_metadata.txt", bytes); // Test that metadata of an absolute path is correct. - check_metadata(bytes, &path).unwrap(); + check_metadata(bytes, &path).expect("Absolute path metadata"); // Test that metadata of a relative path is correct. std::env::set_current_dir(path.parent().unwrap()).unwrap(); - check_metadata(bytes, Path::new(path.file_name().unwrap())).unwrap(); + check_metadata(bytes, Path::new(path.file_name().unwrap())).expect("Relative path metadata"); // Removing file should succeed. remove_file(&path).unwrap(); From e19deaa06fe44bced13a905d64a71ac5f35981aa Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sun, 1 Dec 2024 19:55:44 -0800 Subject: [PATCH 3/8] Fix MAIN_THREAD handle in windows_join_main --- tests/fail-dep/concurrency/windows_join_main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fail-dep/concurrency/windows_join_main.rs b/tests/fail-dep/concurrency/windows_join_main.rs index 279201df86..3ee2bf14f9 100644 --- a/tests/fail-dep/concurrency/windows_join_main.rs +++ b/tests/fail-dep/concurrency/windows_join_main.rs @@ -13,7 +13,7 @@ use windows_sys::Win32::System::Threading::{INFINITE, WaitForSingleObject}; // XXX HACK: This is how miri represents the handle for thread 0. // This value can be "legitimately" obtained by using `GetCurrentThread` with `DuplicateHandle` // but miri does not implement `DuplicateHandle` yet. -const MAIN_THREAD: HANDLE = (2i32 << 30) as HANDLE; +const MAIN_THREAD: HANDLE = (2i32 << 29) as HANDLE; fn main() { thread::spawn(|| { From f1bb77df9ca949d7a014ff651af0e26247eb3e15 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sun, 1 Dec 2024 20:09:45 -0800 Subject: [PATCH 4/8] Try fix for Windows paths on non-Windows machines --- src/shims/windows/fs.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index 5eea09cd0e..03c6d289e7 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::fs::{File, Metadata, OpenOptions}; use std::io; use std::io::{IsTerminal, Read, Seek, SeekFrom, Write}; @@ -152,7 +153,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let file_name = String::from_utf16_lossy(&this.read_wide_str(this.read_pointer(file_name)?)?); - let file_name = Path::new(&file_name); + let file_name = local_path(&file_name); let desired_access = this.read_scalar(desired_access)?.to_u32()?; let share_mode = this.read_scalar(share_mode)?.to_u32()?; let security_attributes = this.read_pointer(security_attributes)?; @@ -390,3 +391,13 @@ fn write_filetime_field<'tcx>( &cx.project_field_named(val, name)?, ) } + +fn local_path(path: &str) -> Cow<'_, Path> { + if cfg!(windows) { + Cow::Borrowed(path.as_ref()) + } else { + let stripped = if path.starts_with(r"\\?\") { &path[3..] } else { path }; + let path = PathBuf::from(stripped.replace("\\", "/")); + Cow::Owned(path) + } +} From d8720c9c11a5d3fedfe492e8cfd3cc8495a40736 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Wed, 20 Nov 2024 22:36:51 -0800 Subject: [PATCH 5/8] Implement file read, write, and delete APIs. STDIN/ERR/OUT are now proper pseudo-handles. --- src/shims/windows/foreign_items.rs | 126 +++++++------- src/shims/windows/fs.rs | 257 ++++++++++++++++++++++++++++- src/shims/windows/handle.rs | 36 ++++ tests/pass/shims/fs.rs | 6 +- 4 files changed, 358 insertions(+), 67 deletions(-) diff --git a/src/shims/windows/foreign_items.rs b/src/shims/windows/foreign_items.rs index 7e7cb69e8a..35ee0e8fea 100644 --- a/src/shims/windows/foreign_items.rs +++ b/src/shims/windows/foreign_items.rs @@ -136,6 +136,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let result = this.GetUserProfileDirectoryW(token, buf, size)?; this.write_scalar(result, dest)?; } + "GetCurrentProcess" => { + let [] = + this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; + this.write_scalar( + Handle::Pseudo(PseudoHandle::CurrentProcess).to_scalar(this), + dest, + )?; + } "GetCurrentProcessId" => { let [] = this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; @@ -145,71 +153,54 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // File related shims "NtWriteFile" => { - if !this.frame_in_std() { - throw_unsup_format!( - "`NtWriteFile` support is crude and just enough for stdout to work" - ); - } - let [ handle, - _event, - _apc_routine, - _apc_context, + event, + apc_routine, + apc_context, io_status_block, buf, n, byte_offset, - _key, + key, ] = this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; - let handle = this.read_target_isize(handle)?; - let buf = this.read_pointer(buf)?; - let n = this.read_scalar(n)?.to_u32()?; - let byte_offset = this.read_target_usize(byte_offset)?; // is actually a pointer - let io_status_block = this - .deref_pointer_as(io_status_block, this.windows_ty_layout("IO_STATUS_BLOCK"))?; - - if byte_offset != 0 { - throw_unsup_format!( - "`NtWriteFile` `ByteOffset` parameter is non-null, which is unsupported" - ); - } - - let written = if handle == -11 || handle == -12 { - // stdout/stderr - use io::Write; - - let buf_cont = - this.read_bytes_ptr_strip_provenance(buf, Size::from_bytes(u64::from(n)))?; - let res = if this.machine.mute_stdout_stderr { - Ok(buf_cont.len()) - } else if handle == -11 { - io::stdout().write(buf_cont) - } else { - io::stderr().write(buf_cont) - }; - // We write at most `n` bytes, which is a `u32`, so we cannot have written more than that. - res.ok().map(|n| u32::try_from(n).unwrap()) - } else { - throw_unsup_format!( - "on Windows, writing to anything except stdout/stderr is not supported" - ) - }; - // We have to put the result into io_status_block. - if let Some(n) = written { - let io_status_information = - this.project_field_named(&io_status_block, "Information")?; - this.write_scalar( - Scalar::from_target_usize(n.into(), this), - &io_status_information, - )?; - } - // Return whether this was a success. >= 0 is success. - // For the error code we arbitrarily pick 0xC0000185, STATUS_IO_DEVICE_ERROR. - this.write_scalar( - Scalar::from_u32(if written.is_some() { 0 } else { 0xC0000185u32 }), - dest, + let res = this.NtWriteFile( + handle, + event, + apc_routine, + apc_context, + io_status_block, + buf, + n, + byte_offset, + key, + )?; + this.write_scalar(res, dest)?; + } + "NtReadFile" => { + let [ + handle, + event, + apc_routine, + apc_context, + io_status_block, + buf, + n, + byte_offset, + key, + ] = this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; + let res = this.NtReadFile( + handle, + event, + apc_routine, + apc_context, + io_status_block, + buf, + n, + byte_offset, + key, )?; + this.write_scalar(res, dest)?; } "GetFullPathNameW" => { let [filename, size, buffer, filepart] = @@ -268,6 +259,19 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let res = this.GetFileInformationByHandle(handle, info)?; this.write_scalar(res, dest)?; } + "DeleteFileW" => { + let [file_name] = + this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; + let res = this.DeleteFileW(file_name)?; + this.write_scalar(res, dest)?; + } + "SetFilePointerEx" => { + let [file, distance_to_move, new_file_pointer, move_method] = + this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; + let res = + this.SetFilePointerEx(file, distance_to_move, new_file_pointer, move_method)?; + this.write_scalar(res, dest)?; + } // Allocation "HeapAlloc" => { @@ -654,12 +658,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { "GetStdHandle" => { let [which] = this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; - let which = this.read_scalar(which)?.to_i32()?; - // We just make this the identity function, so we know later in `NtWriteFile` which - // one it is. This is very fake, but libtest needs it so we cannot make it a - // std-only shim. - // FIXME: this should return real HANDLEs when io support is added - this.write_scalar(Scalar::from_target_isize(which.into(), this), dest)?; + let res = this.GetStdHandle(which)?; + this.write_scalar(res, dest)?; } "CloseHandle" => { let [handle] = diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index 03c6d289e7..902686be7f 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -9,7 +9,7 @@ use rustc_abi::Size; use crate::shims::files::{EvalContextExt as _, FileDescription, FileDescriptionRef}; use crate::shims::time::system_time_to_duration; -use crate::shims::windows::handle::{EvalContextExt as _, Handle}; +use crate::shims::windows::handle::{EvalContextExt as _, Handle, PseudoHandle}; use crate::*; #[derive(Debug)] @@ -359,6 +359,261 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { interp_ok(this.eval_windows("c", "TRUE")) } + + fn DeleteFileW( + &mut self, + file_name: &OpTy<'tcx>, // LPCWSTR + ) -> InterpResult<'tcx, Scalar> { + // ^ Returns BOOL (i32 on Windows) + let this = self.eval_context_mut(); + this.assert_target_os("windows", "DeleteFileW"); + this.check_no_isolation("`DeleteFileW`")?; + let file_name = + String::from_utf16_lossy(&this.read_wide_str(this.read_pointer(file_name)?)?); + let file_name = Path::new(&file_name); + match std::fs::remove_file(file_name) { + Ok(_) => interp_ok(this.eval_windows("c", "TRUE")), + Err(e) => { + this.set_last_error(e)?; + interp_ok(this.eval_windows("c", "FALSE")) + } + } + } + + fn NtWriteFile( + &mut self, + handle: &OpTy<'tcx>, // HANDLE + event: &OpTy<'tcx>, // HANDLE + apc_routine: &OpTy<'tcx>, // PIO_APC_ROUTINE + apc_ctx: &OpTy<'tcx>, // PVOID + io_status_block: &OpTy<'tcx>, // PIO_STATUS_BLOCK + buf: &OpTy<'tcx>, // PVOID + n: &OpTy<'tcx>, // ULONG + byte_offset: &OpTy<'tcx>, // PLARGE_INTEGER + key: &OpTy<'tcx>, // PULONG + ) -> InterpResult<'tcx, Scalar> { + // ^ Returns NTSTATUS (u32 on Windows) + let this = self.eval_context_mut(); + let handle = this.read_handle(handle)?; + let event = this.read_handle(event)?; + let apc_routine = this.read_pointer(apc_routine)?; + let apc_ctx = this.read_pointer(apc_ctx)?; + let buf = this.read_pointer(buf)?; + let n = this.read_scalar(n)?.to_u32()?; + let byte_offset = this.read_target_usize(byte_offset)?; // is actually a pointer + let key = this.read_pointer(key)?; + let io_status_block = + this.deref_pointer_as(io_status_block, this.windows_ty_layout("IO_STATUS_BLOCK"))?; + + if event != Handle::Null { + throw_unsup_format!( + "`NtWriteFile` `Event` parameter is non-null, which is unsupported" + ); + } + + if !this.ptr_is_null(apc_routine)? { + throw_unsup_format!( + "`NtWriteFile` `ApcRoutine` parameter is not null, which is unsupported" + ); + } + + if !this.ptr_is_null(apc_ctx)? { + throw_unsup_format!( + "`NtWriteFile` `ApcContext` parameter is not null, which is unsupported" + ); + } + + if byte_offset != 0 { + throw_unsup_format!( + "`NtWriteFile` `ByteOffset` parameter is non-null, which is unsupported" + ); + } + + if !this.ptr_is_null(key)? { + throw_unsup_format!("`NtWriteFile` `Key` parameter is not null, which is unsupported"); + } + + let written = match handle { + Handle::Pseudo(pseudo @ (PseudoHandle::Stdout | PseudoHandle::Stderr)) => { + // stdout/stderr + let buf_cont = + this.read_bytes_ptr_strip_provenance(buf, Size::from_bytes(u64::from(n)))?; + let res = if this.machine.mute_stdout_stderr { + Ok(buf_cont.len()) + } else if pseudo == PseudoHandle::Stdout { + io::Write::write(&mut io::stdout(), buf_cont) + } else { + io::Write::write(&mut io::stderr(), buf_cont) + }; + // We write at most `n` bytes, which is a `u32`, so we cannot have written more than that. + res.ok().map(|n| u32::try_from(n).unwrap()) + } + Handle::File(fd) => { + let Some(desc) = this.machine.fds.get(fd) else { + this.invalid_handle("NtWriteFile")? + }; + + let errno_layout = this.machine.layouts.u32; + let out_place = this.allocate(errno_layout, MiriMemoryKind::Machine.into())?; + desc.write(&desc, this.machine.communicate(), buf, n as usize, &out_place, this)?; + let written = this.read_scalar(&out_place)?.to_u32()?; + this.deallocate_ptr(out_place.ptr(), None, MiriMemoryKind::Machine.into())?; + Some(written) + } + _ => this.invalid_handle("NtWriteFile")?, + }; + + // We have to put the result into io_status_block. + if let Some(n) = written { + let io_status_information = + this.project_field_named(&io_status_block, "Information")?; + this.write_scalar(Scalar::from_target_usize(n.into(), this), &io_status_information)?; + } + + // Return whether this was a success. >= 0 is success. + // For the error code we arbitrarily pick 0xC0000185, STATUS_IO_DEVICE_ERROR. + interp_ok(Scalar::from_u32(if written.is_some() { 0 } else { 0xC0000185u32 })) + } + + fn NtReadFile( + &mut self, + handle: &OpTy<'tcx>, // HANDLE + event: &OpTy<'tcx>, // HANDLE + apc_routine: &OpTy<'tcx>, // PIO_APC_ROUTINE + apc_ctx: &OpTy<'tcx>, // PVOID + io_status_block: &OpTy<'tcx>, // PIO_STATUS_BLOCK + buf: &OpTy<'tcx>, // PVOID + n: &OpTy<'tcx>, // ULONG + byte_offset: &OpTy<'tcx>, // PLARGE_INTEGER + key: &OpTy<'tcx>, // PULONG + ) -> InterpResult<'tcx, Scalar> { + // ^ Returns NTSTATUS (u32 on Windows) + let this = self.eval_context_mut(); + let handle = this.read_handle(handle)?; + let event = this.read_handle(event)?; + let apc_routine = this.read_pointer(apc_routine)?; + let apc_ctx = this.read_pointer(apc_ctx)?; + let buf = this.read_pointer(buf)?; + let n = this.read_scalar(n)?.to_u32()?; + let byte_offset = this.read_target_usize(byte_offset)?; // is actually a pointer + let key = this.read_pointer(key)?; + let io_status_block = + this.deref_pointer_as(io_status_block, this.windows_ty_layout("IO_STATUS_BLOCK"))?; + + if event != Handle::Null { + throw_unsup_format!( + "`NtWriteFile` `Event` parameter is non-null, which is unsupported" + ); + } + + if !this.ptr_is_null(apc_routine)? { + throw_unsup_format!( + "`NtWriteFile` `ApcRoutine` parameter is not null, which is unsupported" + ); + } + + if !this.ptr_is_null(apc_ctx)? { + throw_unsup_format!( + "`NtWriteFile` `ApcContext` parameter is not null, which is unsupported" + ); + } + + if byte_offset != 0 { + throw_unsup_format!( + "`NtWriteFile` `ByteOffset` parameter is non-null, which is unsupported" + ); + } + + if !this.ptr_is_null(key)? { + throw_unsup_format!("`NtWriteFile` `Key` parameter is not null, which is unsupported"); + } + + let read = match handle { + Handle::Pseudo(PseudoHandle::Stdin) => { + // stdout/stderr + let mut buf_cont = vec![0u8; n as usize]; + let res = io::Read::read(&mut io::stdin(), &mut buf_cont); + this.write_bytes_ptr(buf, buf_cont)?; + // We write at most `n` bytes, which is a `u32`, so we cannot have written more than that. + res.ok().map(|n| u32::try_from(n).unwrap()) + } + Handle::File(fd) => { + let Some(desc) = this.machine.fds.get(fd) else { + this.invalid_handle("NtReadFile")? + }; + + let errno_layout = this.machine.layouts.u32; + let out_place = this.allocate(errno_layout, MiriMemoryKind::Machine.into())?; + desc.read(&desc, this.machine.communicate(), buf, n as usize, &out_place, this)?; + let read = this.read_scalar(&out_place)?.to_u32()?; + this.deallocate_ptr(out_place.ptr(), None, MiriMemoryKind::Machine.into())?; + Some(read) + } + _ => this.invalid_handle("NtReadFile")?, + }; + + // We have to put the result into io_status_block. + if let Some(n) = read { + let io_status_information = + this.project_field_named(&io_status_block, "Information")?; + this.write_scalar(Scalar::from_target_usize(n.into(), this), &io_status_information)?; + } + + // Return whether this was a success. >= 0 is success. + // For the error code we arbitrarily pick 0xC0000185, STATUS_IO_DEVICE_ERROR. + interp_ok(Scalar::from_u32(if read.is_some() { 0 } else { 0xC0000185u32 })) + } + + fn SetFilePointerEx( + &mut self, + file: &OpTy<'tcx>, // HANDLE + dist_to_move: &OpTy<'tcx>, // LARGE_INTEGER + new_fp: &OpTy<'tcx>, // PLARGE_INTEGER + move_method: &OpTy<'tcx>, // DWORD + ) -> InterpResult<'tcx, Scalar> { + // ^ Returns BOOL (i32 on Windows) + let this = self.eval_context_mut(); + let file = this.read_handle(file)?; + let dist_to_move = this.read_scalar(dist_to_move)?.to_i64()?; + let move_method = this.read_scalar(move_method)?.to_u32()?; + + let fd = match file { + Handle::File(fd) => fd, + _ => this.invalid_handle("SetFilePointerEx")?, + }; + + let Some(desc) = this.machine.fds.get(fd) else { + throw_unsup_format!("`SetFilePointerEx` is only supported on file backed handles"); + }; + + let file_begin = this.eval_windows_u32("c", "FILE_BEGIN"); + let file_current = this.eval_windows_u32("c", "FILE_CURRENT"); + let file_end = this.eval_windows_u32("c", "FILE_END"); + + let seek = if move_method == file_begin { + SeekFrom::Start(dist_to_move.try_into().unwrap()) + } else if move_method == file_current { + SeekFrom::Current(dist_to_move) + } else if move_method == file_end { + SeekFrom::End(dist_to_move) + } else { + throw_unsup_format!("Invalid move method: {move_method}") + }; + + match desc.seek(this.machine.communicate(), seek)? { + Ok(n) => { + this.write_scalar( + Scalar::from_i64(n.try_into().unwrap()), + &this.deref_pointer(new_fp)?, + )?; + interp_ok(this.eval_windows("c", "TRUE")) + } + Err(e) => { + this.set_last_error(e)?; + interp_ok(this.eval_windows("c", "FALSE")) + } + } + } } /// Windows FILETIME is measured in 100-nanosecs since 1601 diff --git a/src/shims/windows/handle.rs b/src/shims/windows/handle.rs index 8d684c9495..9c8db5730a 100644 --- a/src/shims/windows/handle.rs +++ b/src/shims/windows/handle.rs @@ -9,6 +9,10 @@ use crate::*; #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] pub enum PseudoHandle { CurrentThread, + CurrentProcess, + Stdin, + Stdout, + Stderr, } /// Miri representation of a Windows `HANDLE` @@ -23,16 +27,28 @@ pub enum Handle { impl PseudoHandle { const CURRENT_THREAD_VALUE: u32 = 0; + const STDIN_VALUE: u32 = 1; + const STDOUT_VALUE: u32 = 2; + const STDERR_VALUE: u32 = 3; + const CURRENT_PROCESS_VALUE: u32 = 4; fn value(self) -> u32 { match self { Self::CurrentThread => Self::CURRENT_THREAD_VALUE, + Self::CurrentProcess => Self::CURRENT_PROCESS_VALUE, + Self::Stdin => Self::STDIN_VALUE, + Self::Stdout => Self::STDOUT_VALUE, + Self::Stderr => Self::STDERR_VALUE, } } fn from_value(value: u32) -> Option { match value { Self::CURRENT_THREAD_VALUE => Some(Self::CurrentThread), + Self::CURRENT_PROCESS_VALUE => Some(Self::CurrentProcess), + Self::STDIN_VALUE => Some(Self::Stdin), + Self::STDOUT_VALUE => Some(Self::Stdout), + Self::STDERR_VALUE => Some(Self::Stderr), _ => None, } } @@ -212,6 +228,26 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ))) } + fn GetStdHandle(&mut self, which: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> { + let this = self.eval_context_mut(); + let which = this.read_scalar(which)?.to_i32()?; + + let stdin = this.eval_windows("c", "STD_INPUT_HANDLE").to_i32()?; + let stdout = this.eval_windows("c", "STD_OUTPUT_HANDLE").to_i32()?; + let stderr = this.eval_windows("c", "STD_ERROR_HANDLE").to_i32()?; + + let handle = if which == stdin { + Handle::Pseudo(PseudoHandle::Stdin) + } else if which == stdout { + Handle::Pseudo(PseudoHandle::Stdout) + } else if which == stderr { + Handle::Pseudo(PseudoHandle::Stderr) + } else { + throw_unsup_format!("Invalid argument to `GetStdHandle`: {which}") + }; + interp_ok(handle.to_scalar(this)) + } + fn CloseHandle(&mut self, handle_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); diff --git a/tests/pass/shims/fs.rs b/tests/pass/shims/fs.rs index 5f18ae7b5d..c343c54588 100644 --- a/tests/pass/shims/fs.rs +++ b/tests/pass/shims/fs.rs @@ -17,10 +17,10 @@ mod utils; fn main() { test_path_conversion(); + test_file(); + test_file_create_new(); + test_seek(); if cfg!(not(windows)) { - test_file(); - test_file_create_new(); - test_seek(); test_file_clone(); test_metadata(); test_file_set_len(); From 19acb93f41722b5cc47768fdd2b25d090b6dc45a Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sun, 1 Dec 2024 12:57:07 -0800 Subject: [PATCH 6/8] Less hacky read/write --- src/shims/windows/fs.rs | 77 +++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 30 deletions(-) diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index 902686be7f..03bea71c52 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -433,6 +433,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { throw_unsup_format!("`NtWriteFile` `Key` parameter is not null, which is unsupported"); } + // We have to put the result into io_status_block. + let io_status_information = this.project_field_named(&io_status_block, "Information")?; + let written = match handle { Handle::Pseudo(pseudo @ (PseudoHandle::Stdout | PseudoHandle::Stderr)) => { // stdout/stderr @@ -446,33 +449,39 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { io::Write::write(&mut io::stderr(), buf_cont) }; // We write at most `n` bytes, which is a `u32`, so we cannot have written more than that. - res.ok().map(|n| u32::try_from(n).unwrap()) + if let Ok(n) = res { + this.write_scalar( + Scalar::from_target_usize(n.try_into().unwrap(), this), + &io_status_information, + )?; + true + } else { + false + } } Handle::File(fd) => { let Some(desc) = this.machine.fds.get(fd) else { this.invalid_handle("NtWriteFile")? }; - let errno_layout = this.machine.layouts.u32; - let out_place = this.allocate(errno_layout, MiriMemoryKind::Machine.into())?; - desc.write(&desc, this.machine.communicate(), buf, n as usize, &out_place, this)?; - let written = this.read_scalar(&out_place)?.to_u32()?; - this.deallocate_ptr(out_place.ptr(), None, MiriMemoryKind::Machine.into())?; - Some(written) + // TODO: Windows expects this function to return its error, not set the global one + desc.write( + &desc, + this.machine.communicate(), + buf, + n as usize, + &io_status_information, + this, + )?; + let written = this.read_scalar(&io_status_information)?.to_i64()?; + written != -1 } _ => this.invalid_handle("NtWriteFile")?, }; - // We have to put the result into io_status_block. - if let Some(n) = written { - let io_status_information = - this.project_field_named(&io_status_block, "Information")?; - this.write_scalar(Scalar::from_target_usize(n.into(), this), &io_status_information)?; - } - // Return whether this was a success. >= 0 is success. // For the error code we arbitrarily pick 0xC0000185, STATUS_IO_DEVICE_ERROR. - interp_ok(Scalar::from_u32(if written.is_some() { 0 } else { 0xC0000185u32 })) + interp_ok(Scalar::from_u32(if written { 0 } else { 0xC0000185u32 })) } fn NtReadFile( @@ -528,6 +537,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { throw_unsup_format!("`NtWriteFile` `Key` parameter is not null, which is unsupported"); } + let io_status_information = this.project_field_named(&io_status_block, "Information")?; + let read = match handle { Handle::Pseudo(PseudoHandle::Stdin) => { // stdout/stderr @@ -535,33 +546,39 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let res = io::Read::read(&mut io::stdin(), &mut buf_cont); this.write_bytes_ptr(buf, buf_cont)?; // We write at most `n` bytes, which is a `u32`, so we cannot have written more than that. - res.ok().map(|n| u32::try_from(n).unwrap()) + if let Ok(n) = res { + this.write_scalar( + Scalar::from_target_usize(n.try_into().unwrap(), this), + &io_status_information, + )?; + true + } else { + false + } } Handle::File(fd) => { let Some(desc) = this.machine.fds.get(fd) else { this.invalid_handle("NtReadFile")? }; - let errno_layout = this.machine.layouts.u32; - let out_place = this.allocate(errno_layout, MiriMemoryKind::Machine.into())?; - desc.read(&desc, this.machine.communicate(), buf, n as usize, &out_place, this)?; - let read = this.read_scalar(&out_place)?.to_u32()?; - this.deallocate_ptr(out_place.ptr(), None, MiriMemoryKind::Machine.into())?; - Some(read) + // TODO: Windows expects this function to return its error, not set the global one + desc.read( + &desc, + this.machine.communicate(), + buf, + n as usize, + &io_status_information, + this, + )?; + let read = this.read_scalar(&io_status_information)?.to_i64()?; + read != -1 } _ => this.invalid_handle("NtReadFile")?, }; - // We have to put the result into io_status_block. - if let Some(n) = read { - let io_status_information = - this.project_field_named(&io_status_block, "Information")?; - this.write_scalar(Scalar::from_target_usize(n.into(), this), &io_status_information)?; - } - // Return whether this was a success. >= 0 is success. // For the error code we arbitrarily pick 0xC0000185, STATUS_IO_DEVICE_ERROR. - interp_ok(Scalar::from_u32(if read.is_some() { 0 } else { 0xC0000185u32 })) + interp_ok(Scalar::from_u32(if read { 0 } else { 0xC0000185u32 })) } fn SetFilePointerEx( From 296d806930304c03f2cba91cc8a2fed4a2f38d16 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 5 Dec 2024 20:40:40 -0800 Subject: [PATCH 7/8] Proper error handling for Windows. Requires one-off changes to Linux handling --- src/shims/files.rs | 48 ++++++++++----- src/shims/unix/fd.rs | 12 +++- src/shims/unix/fs.rs | 18 ++++-- src/shims/unix/linux_like/eventfd.rs | 42 ++++++++----- src/shims/unix/unnamed_socket.rs | 14 ++--- src/shims/windows/fs.rs | 89 ++++++++++++++++------------ 6 files changed, 140 insertions(+), 83 deletions(-) diff --git a/src/shims/files.rs b/src/shims/files.rs index f673b834be..ba0d69de52 100644 --- a/src/shims/files.rs +++ b/src/shims/files.rs @@ -1,6 +1,6 @@ use std::any::Any; use std::collections::BTreeMap; -use std::io::{IsTerminal, Read, SeekFrom, Write}; +use std::io::{ErrorKind, IsTerminal, Read, SeekFrom, Write}; use std::ops::Deref; use std::rc::{Rc, Weak}; use std::{fs, io}; @@ -25,7 +25,7 @@ pub trait FileDescription: std::fmt::Debug + Any { _len: usize, _dest: &MPlaceTy<'tcx>, _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { throw_unsup_format!("cannot read from {}", self.name()); } @@ -40,7 +40,7 @@ pub trait FileDescription: std::fmt::Debug + Any { _len: usize, _dest: &MPlaceTy<'tcx>, _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { throw_unsup_format!("cannot write to {}", self.name()); } @@ -97,7 +97,7 @@ impl FileDescription for io::Stdin { len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { let mut bytes = vec![0; len]; if !communicate_allowed { // We want isolation mode to be deterministic, so we have to disallow all reads, even stdin. @@ -106,7 +106,10 @@ impl FileDescription for io::Stdin { let result = Read::read(&mut { self }, &mut bytes); match result { Ok(read_size) => ecx.return_read_success(ptr, &bytes, read_size, dest), - Err(e) => ecx.set_last_error_and_return(e, dest), + Err(e) => { + ecx.write_int(-1, dest)?; + interp_ok(Err(e.kind())) + } } } @@ -128,7 +131,7 @@ impl FileDescription for io::Stdout { len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; // We allow writing to stderr even with isolation enabled. let result = Write::write(&mut { self }, bytes); @@ -140,7 +143,10 @@ impl FileDescription for io::Stdout { io::stdout().flush().unwrap(); match result { Ok(write_size) => ecx.return_write_success(write_size, dest), - Err(e) => ecx.set_last_error_and_return(e, dest), + Err(e) => { + ecx.write_int(-1, dest)?; + interp_ok(Err(e.kind())) + } } } @@ -162,14 +168,17 @@ impl FileDescription for io::Stderr { len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; // We allow writing to stderr even with isolation enabled. // No need to flush, stderr is not buffered. let result = Write::write(&mut { self }, bytes); match result { Ok(write_size) => ecx.return_write_success(write_size, dest), - Err(e) => ecx.set_last_error_and_return(e, dest), + Err(e) => { + ecx.write_int(-1, dest)?; + interp_ok(Err(e.kind())) + } } } @@ -195,7 +204,7 @@ impl FileDescription for NullOutput { len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { // We just don't write anything, but report to the user that we did. ecx.return_write_success(len, dest) } @@ -384,7 +393,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { bytes: &[u8], actual_read_size: usize, dest: &MPlaceTy<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { let this = self.eval_context_mut(); // If reading to `bytes` did not fail, we write those bytes to the buffer. // Crucially, if fewer than `bytes.len()` bytes were read, only write @@ -393,7 +402,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // The actual read size is always less than what got originally requested so this cannot fail. this.write_int(u64::try_from(actual_read_size).unwrap(), dest)?; - interp_ok(()) + interp_ok(Ok(())) } /// Helper to implement `FileDescription::write`: @@ -402,10 +411,21 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { &mut self, actual_write_size: usize, dest: &MPlaceTy<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { let this = self.eval_context_mut(); // The actual write size is always less than what got originally requested so this cannot fail. this.write_int(u64::try_from(actual_write_size).unwrap(), dest)?; - interp_ok(()) + interp_ok(Ok(())) + } + + fn return_io_error( + &mut self, + error: ErrorKind, + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { + let this = self.eval_context_mut(); + this.set_last_error(error)?; + this.write_int(-1, dest)?; + interp_ok(Err(error)) } } diff --git a/src/shims/unix/fd.rs b/src/shims/unix/fd.rs index e5dead1a26..e179d7a955 100644 --- a/src/shims/unix/fd.rs +++ b/src/shims/unix/fd.rs @@ -246,7 +246,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // `usize::MAX` because it is bounded by the host's `isize`. match offset { - None => fd.read(&fd, communicate, buf, count, dest, this)?, + None => + match fd.read(&fd, communicate, buf, count, dest, this)? { + Ok(_) => (), + Err(e) => this.set_last_error(e)?, + }, Some(offset) => { let Ok(offset) = u64::try_from(offset) else { return this.set_last_error_and_return(LibcError("EINVAL"), dest); @@ -286,7 +290,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; match offset { - None => fd.write(&fd, communicate, buf, count, dest, this)?, + None => + match fd.write(&fd, communicate, buf, count, dest, this)? { + Ok(_) => (), + Err(e) => this.set_last_error(e)?, + }, Some(offset) => { let Ok(offset) = u64::try_from(offset) else { return this.set_last_error_and_return(LibcError("EINVAL"), dest); diff --git a/src/shims/unix/fs.rs b/src/shims/unix/fs.rs index b41a4d2246..2fab690a6a 100644 --- a/src/shims/unix/fs.rs +++ b/src/shims/unix/fs.rs @@ -38,13 +38,16 @@ impl FileDescription for FileHandle { len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), ErrorKind>> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); let mut bytes = vec![0; len]; let result = (&mut &self.file).read(&mut bytes); match result { Ok(read_size) => ecx.return_read_success(ptr, &bytes, read_size, dest), - Err(e) => ecx.set_last_error_and_return(e, dest), + Err(e) => { + ecx.set_last_error_and_return(e.kind(), dest)?; + interp_ok(Err(e.kind())) + } } } @@ -56,13 +59,16 @@ impl FileDescription for FileHandle { len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; let result = (&mut &self.file).write(bytes); match result { Ok(write_size) => ecx.return_write_success(write_size, dest), - Err(e) => ecx.set_last_error_and_return(e, dest), + Err(e) => { + ecx.set_last_error_and_return(e.kind(), dest)?; + interp_ok(Err(e.kind())) + } } } @@ -141,7 +147,7 @@ impl UnixFileDescription for FileHandle { }; let result = f(); match result { - Ok(read_size) => ecx.return_read_success(ptr, &bytes, read_size, dest), + Ok(read_size) => ecx.return_read_success(ptr, &bytes, read_size, dest).map(|_| ()), Err(e) => ecx.set_last_error_and_return(e, dest), } } @@ -172,7 +178,7 @@ impl UnixFileDescription for FileHandle { }; let result = f(); match result { - Ok(write_size) => ecx.return_write_success(write_size, dest), + Ok(write_size) => ecx.return_write_success(write_size, dest).map(|_| ()), Err(e) => ecx.set_last_error_and_return(e, dest), } } diff --git a/src/shims/unix/linux_like/eventfd.rs b/src/shims/unix/linux_like/eventfd.rs index 4bbe417ea8..5a53faebf0 100644 --- a/src/shims/unix/linux_like/eventfd.rs +++ b/src/shims/unix/linux_like/eventfd.rs @@ -4,7 +4,9 @@ use std::io; use std::io::ErrorKind; use crate::concurrency::VClock; -use crate::shims::files::{FileDescription, FileDescriptionRef, WeakFileDescriptionRef}; +use crate::shims::files::{ + EvalContextExt as _, FileDescription, FileDescriptionRef, WeakFileDescriptionRef, +}; use crate::shims::unix::UnixFileDescription; use crate::shims::unix::linux_like::epoll::{EpollReadyEvents, EvalContextExt as _}; use crate::*; @@ -54,12 +56,12 @@ impl FileDescription for Event { len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { // We're treating the buffer as a `u64`. let ty = ecx.machine.layouts.u64; // Check the size of slice, and return error only if the size of the slice < 8. if len < ty.size.bytes_usize() { - return ecx.set_last_error_and_return(ErrorKind::InvalidInput, dest); + return ecx.return_io_error(ErrorKind::InvalidInput, dest); } // eventfd read at the size of u64. @@ -89,12 +91,12 @@ impl FileDescription for Event { len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { // We're treating the buffer as a `u64`. let ty = ecx.machine.layouts.u64; // Check the size of slice, and return error only if the size of the slice < 8. if len < ty.layout.size.bytes_usize() { - return ecx.set_last_error_and_return(ErrorKind::InvalidInput, dest); + return ecx.return_io_error(ErrorKind::InvalidInput, dest); } // Read the user-supplied value from the pointer. @@ -103,7 +105,7 @@ impl FileDescription for Event { // u64::MAX as input is invalid because the maximum value of counter is u64::MAX - 1. if num == u64::MAX { - return ecx.set_last_error_and_return(ErrorKind::InvalidInput, dest); + return ecx.return_io_error(ErrorKind::InvalidInput, dest); } // If the addition does not let the counter to exceed the maximum value, update the counter. // Else, block. @@ -198,7 +200,7 @@ fn eventfd_write<'tcx>( dest: &MPlaceTy<'tcx>, weak_eventfd: WeakFileDescriptionRef, ecx: &mut MiriInterpCx<'tcx>, -) -> InterpResult<'tcx> { +) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { let Some(eventfd_ref) = weak_eventfd.upgrade() else { throw_unsup_format!("eventfd FD got closed while blocking.") }; @@ -232,12 +234,13 @@ fn eventfd_write<'tcx>( } // Return how many bytes we consumed from the user-provided buffer. - return ecx.write_int(buf_place.layout.size.bytes(), dest); + ecx.write_int(buf_place.layout.size.bytes(), dest)?; + return interp_ok(Ok(())); } None | Some(u64::MAX) => { // We can't update the state, so we have to block. if eventfd.is_nonblock { - return ecx.set_last_error_and_return(ErrorKind::WouldBlock, dest); + return ecx.return_io_error(ErrorKind::WouldBlock, dest); } let dest = dest.clone(); @@ -256,13 +259,16 @@ fn eventfd_write<'tcx>( } @unblock = |this| { // When we get unblocked, try again. - eventfd_write(num, buf_place, &dest, weak_eventfd, this) + match eventfd_write(num, buf_place, &dest, weak_eventfd, this)? { + Ok(_) => interp_ok(()), + Err(e) => this.set_last_error(e), + } } ), ); } }; - interp_ok(()) + interp_ok(Ok(())) } /// Block thread if the current counter is 0, @@ -272,7 +278,7 @@ fn eventfd_read<'tcx>( dest: &MPlaceTy<'tcx>, weak_eventfd: WeakFileDescriptionRef, ecx: &mut MiriInterpCx<'tcx>, -) -> InterpResult<'tcx> { +) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { let Some(eventfd_ref) = weak_eventfd.upgrade() else { throw_unsup_format!("eventfd FD got closed while blocking.") }; @@ -287,7 +293,7 @@ fn eventfd_read<'tcx>( // Block when counter == 0. if counter == 0 { if eventfd.is_nonblock { - return ecx.set_last_error_and_return(ErrorKind::WouldBlock, dest); + return ecx.return_io_error(ErrorKind::WouldBlock, dest); } let dest = dest.clone(); @@ -304,7 +310,10 @@ fn eventfd_read<'tcx>( } @unblock = |this| { // When we get unblocked, try again. - eventfd_read(buf_place, &dest, weak_eventfd, this) + match eventfd_read(buf_place, &dest, weak_eventfd, this)? { + Ok(_) => interp_ok(()), + Err(e) => this.set_last_error(e), + } } ), ); @@ -330,7 +339,8 @@ fn eventfd_read<'tcx>( } // Tell userspace how many bytes we put into the buffer. - return ecx.write_int(buf_place.layout.size.bytes(), dest); + ecx.write_int(buf_place.layout.size.bytes(), dest)?; + return interp_ok(Ok(())); } - interp_ok(()) + interp_ok(Ok(())) } diff --git a/src/shims/unix/unnamed_socket.rs b/src/shims/unix/unnamed_socket.rs index 24304c0c04..b40a75cdb9 100644 --- a/src/shims/unix/unnamed_socket.rs +++ b/src/shims/unix/unnamed_socket.rs @@ -89,7 +89,7 @@ impl FileDescription for AnonSocket { len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { let mut bytes = vec![0; len]; // Always succeed on read size 0. @@ -114,7 +114,7 @@ impl FileDescription for AnonSocket { // EAGAIN or EWOULDBLOCK can be returned for socket, // POSIX.1-2001 allows either error to be returned for this case. // Since there is no ErrorKind for EAGAIN, WouldBlock is used. - return ecx.set_last_error_and_return(ErrorKind::WouldBlock, dest); + return ecx.return_io_error(ErrorKind::WouldBlock, dest); } else { // Blocking socketpair with writer and empty buffer. // FIXME: blocking is currently not supported @@ -134,7 +134,7 @@ impl FileDescription for AnonSocket { len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { // Always succeed on write size 0. // ("If count is zero and fd refers to a file other than a regular file, the results are not specified.") if len == 0 { @@ -145,7 +145,7 @@ impl FileDescription for AnonSocket { let Some(peer_fd) = self.peer_fd().upgrade() else { // If the upgrade from Weak to Rc fails, it indicates that all read ends have been // closed. - return ecx.set_last_error_and_return(ErrorKind::BrokenPipe, dest); + return ecx.return_io_error(ErrorKind::BrokenPipe, dest); }; let Some(writebuf) = &peer_fd.downcast::().unwrap().readbuf else { @@ -158,7 +158,7 @@ impl FileDescription for AnonSocket { if available_space == 0 { if self.is_nonblock { // Non-blocking socketpair with a full buffer. - return ecx.set_last_error_and_return(ErrorKind::WouldBlock, dest); + return ecx.return_io_error(ErrorKind::WouldBlock, dest); } else { // Blocking socketpair with a full buffer. throw_unsup_format!("socketpair/pipe/pipe2 write: blocking isn't supported yet"); @@ -180,7 +180,7 @@ fn anonsocket_write<'tcx>( len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, -) -> InterpResult<'tcx> { +) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { let Some(writebuf) = &peer_fd.downcast::().unwrap().readbuf else { // FIXME: This should return EBADF, but there's no nice way to do that as there's no // corresponding ErrorKind variant. @@ -215,7 +215,7 @@ fn anonsocket_read<'tcx>( ptr: Pointer, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, -) -> InterpResult<'tcx> { +) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { let Some(readbuf) = &anonsocket.readbuf else { // FIXME: This should return EBADF, but there's no nice way to do that as there's no // corresponding ErrorKind variant. diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index 03bea71c52..fa70df30cd 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -1,7 +1,7 @@ use std::borrow::Cow; use std::fs::{File, Metadata, OpenOptions}; use std::io; -use std::io::{IsTerminal, Read, Seek, SeekFrom, Write}; +use std::io::{ErrorKind, IsTerminal, Read, Seek, SeekFrom, Write}; use std::path::{Path, PathBuf}; use std::time::SystemTime; @@ -31,13 +31,16 @@ impl FileDescription for FileHandle { len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); let mut bytes = vec![0; len]; let result = (&mut &self.file).read(&mut bytes); match result { Ok(read_size) => ecx.return_read_success(ptr, &bytes, read_size, dest), - Err(e) => ecx.set_last_error_and_return(e, dest), + Err(e) => { + ecx.write_int(0, dest)?; + interp_ok(Err(e.kind())) + } } } @@ -49,13 +52,16 @@ impl FileDescription for FileHandle { len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { + ) -> InterpResult<'tcx, Result<(), io::ErrorKind>> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; let result = (&mut &self.file).write(bytes); match result { Ok(write_size) => ecx.return_write_success(write_size, dest), - Err(e) => ecx.set_last_error_and_return(e, dest), + Err(e) => { + ecx.write_int(0, dest)?; + interp_ok(Err(e.kind())) + } } } @@ -449,14 +455,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { io::Write::write(&mut io::stderr(), buf_cont) }; // We write at most `n` bytes, which is a `u32`, so we cannot have written more than that. - if let Ok(n) = res { - this.write_scalar( - Scalar::from_target_usize(n.try_into().unwrap(), this), - &io_status_information, - )?; - true - } else { - false + match res { + Ok(n) => { + this.write_scalar( + Scalar::from_target_usize(n.try_into().unwrap(), this), + &io_status_information, + )?; + Ok(()) + } + Err(e) => Err(e.kind()), } } Handle::File(fd) => { @@ -465,7 +472,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; // TODO: Windows expects this function to return its error, not set the global one - desc.write( + let err = desc.write( &desc, this.machine.communicate(), buf, @@ -473,15 +480,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { &io_status_information, this, )?; - let written = this.read_scalar(&io_status_information)?.to_i64()?; - written != -1 + err } _ => this.invalid_handle("NtWriteFile")?, }; // Return whether this was a success. >= 0 is success. - // For the error code we arbitrarily pick 0xC0000185, STATUS_IO_DEVICE_ERROR. - interp_ok(Scalar::from_u32(if written { 0 } else { 0xC0000185u32 })) + interp_ok(Scalar::from_u32(match written { + Ok(_) => 0, + Err(e) => error_kind_to_ntstatus(e), + })) } fn NtReadFile( @@ -546,14 +554,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let res = io::Read::read(&mut io::stdin(), &mut buf_cont); this.write_bytes_ptr(buf, buf_cont)?; // We write at most `n` bytes, which is a `u32`, so we cannot have written more than that. - if let Ok(n) = res { - this.write_scalar( - Scalar::from_target_usize(n.try_into().unwrap(), this), - &io_status_information, - )?; - true - } else { - false + match res { + Ok(n) => { + this.write_scalar( + Scalar::from_target_usize(n.try_into().unwrap(), this), + &io_status_information, + )?; + Ok(()) + } + Err(e) => Err(e.kind()), } } Handle::File(fd) => { @@ -562,7 +571,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; // TODO: Windows expects this function to return its error, not set the global one - desc.read( + let err = desc.read( &desc, this.machine.communicate(), buf, @@ -570,15 +579,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { &io_status_information, this, )?; - let read = this.read_scalar(&io_status_information)?.to_i64()?; - read != -1 + err } _ => this.invalid_handle("NtReadFile")?, }; // Return whether this was a success. >= 0 is success. - // For the error code we arbitrarily pick 0xC0000185, STATUS_IO_DEVICE_ERROR. - interp_ok(Scalar::from_u32(if read { 0 } else { 0xC0000185u32 })) + interp_ok(Scalar::from_u32(match read { + Ok(_) => 0, + Err(e) => error_kind_to_ntstatus(e), + })) } fn SetFilePointerEx( @@ -664,12 +674,15 @@ fn write_filetime_field<'tcx>( ) } -fn local_path(path: &str) -> Cow<'_, Path> { - if cfg!(windows) { - Cow::Borrowed(path.as_ref()) - } else { - let stripped = if path.starts_with(r"\\?\") { &path[3..] } else { path }; - let path = PathBuf::from(stripped.replace("\\", "/")); - Cow::Owned(path) +fn error_kind_to_ntstatus(e: ErrorKind) -> u32 { + match e { + // STATUS_MEDIA_WRITE_PROTECTED + ErrorKind::ReadOnlyFilesystem => 0xC00000A2, + // STATUS_FILE_INVALID + ErrorKind::InvalidInput => 0xC0000098, + // STATUS_DISK_FULL + ErrorKind::FilesystemQuotaExceeded => 0xC000007F, + // For the default error code we arbitrarily pick 0xC0000185, STATUS_IO_DEVICE_ERROR. + _ => 0xC0000185, } } From f230bb97b8963bed3fd7e78a7707d6f996eb2649 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 5 Dec 2024 20:42:46 -0800 Subject: [PATCH 8/8] Fix path reading --- src/shims/windows/fs.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index fa70df30cd..4f6d7a1b6d 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -1,8 +1,7 @@ -use std::borrow::Cow; use std::fs::{File, Metadata, OpenOptions}; use std::io; use std::io::{ErrorKind, IsTerminal, Read, Seek, SeekFrom, Write}; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::time::SystemTime; use rustc_abi::Size; @@ -157,9 +156,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.assert_target_os("windows", "CreateFileW"); this.check_no_isolation("`CreateFileW`")?; - let file_name = - String::from_utf16_lossy(&this.read_wide_str(this.read_pointer(file_name)?)?); - let file_name = local_path(&file_name); + let file_name = this.read_path_from_wide_str(this.read_pointer(file_name)?)?; let desired_access = this.read_scalar(desired_access)?.to_u32()?; let share_mode = this.read_scalar(share_mode)?.to_u32()?; let security_attributes = this.read_pointer(security_attributes)?; @@ -374,9 +371,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let this = self.eval_context_mut(); this.assert_target_os("windows", "DeleteFileW"); this.check_no_isolation("`DeleteFileW`")?; - let file_name = - String::from_utf16_lossy(&this.read_wide_str(this.read_pointer(file_name)?)?); - let file_name = Path::new(&file_name); + let file_name = this.read_path_from_wide_str(this.read_pointer(file_name)?)?; match std::fs::remove_file(file_name) { Ok(_) => interp_ok(this.eval_windows("c", "TRUE")), Err(e) => {