From 9b61c05ff0897e1ab0f3d8708540fa0b0dc6015f Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sun, 1 Dec 2024 19:27:32 -0800 Subject: [PATCH 01/22] 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 02/22] 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 03/22] 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 04/22] 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 9a76a85f67c1b69c05fefe1d15cd9d6ebac30819 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 5 Dec 2024 19:59:44 -0800 Subject: [PATCH 05/22] Most review comments - still missing shim tests --- src/shims/windows/fs.rs | 51 ++++++++++++++++++++----------------- src/shims/windows/handle.rs | 36 ++++++++++++++++++-------- 2 files changed, 52 insertions(+), 35 deletions(-) diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index 03c6d289e7..4522cde8c8 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::{IsTerminal, Read, Seek, SeekFrom, Write}; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::time::SystemTime; use rustc_abi::Size; @@ -116,6 +115,14 @@ impl FileDescription for DirHandle { fn metadata<'tcx>(&self) -> InterpResult<'tcx, io::Result> { interp_ok(self.path.metadata()) } + + fn close<'tcx>( + self: Box, + _communicate_allowed: bool, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, io::Result<()>> { + interp_ok(Ok(())) + } } #[derive(Debug)] @@ -131,6 +138,14 @@ impl FileDescription for MetadataHandle { fn metadata<'tcx>(&self) -> InterpResult<'tcx, io::Result> { interp_ok(self.path.metadata()) } + + fn close<'tcx>( + self: Box, + _communicate_allowed: bool, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, io::Result<()>> { + interp_ok(Ok(())) + } } impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} @@ -151,9 +166,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)?; @@ -267,19 +280,19 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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)) + let fd_num = fh.insert_new(DirHandle { path: file_name.into() }); + Ok(Handle::File(fd_num)) } 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)) + let fd_num = fh.insert_new(MetadataHandle { path: file_name.into() }); + Ok(Handle::File(fd_num)) } 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) + let fd_num = fh.insert_new(FileHandle { file, writable: desired_write }); + Handle::File(fd_num) }) }; @@ -308,13 +321,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.windows_ty_layout("BY_HANDLE_FILE_INFORMATION"), )?; - let fd = if let Handle::File(fd) = file { - fd + let fd_num = if let Handle::File(fd_num) = file { + fd_num } else { this.invalid_handle("GetFileInformationByHandle")? }; - let Some(desc) = this.machine.fds.get(fd) else { + let Some(desc) = this.machine.fds.get(fd_num) else { this.invalid_handle("GetFileInformationByHandle")? }; @@ -391,13 +404,3 @@ 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) - } -} diff --git a/src/shims/windows/handle.rs b/src/shims/windows/handle.rs index 8d684c9495..836ef11872 100644 --- a/src/shims/windows/handle.rs +++ b/src/shims/windows/handle.rs @@ -6,6 +6,9 @@ use rustc_abi::HasDataLayout; use crate::concurrency::thread::ThreadNotFound; use crate::*; +/// Internal type of a file-descriptor - this is what [`FdTable`](shims::files::FdTable) expects +type FdNum = i32; + #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] pub enum PseudoHandle { CurrentThread, @@ -17,7 +20,7 @@ pub enum Handle { Null, Pseudo(PseudoHandle), Thread(ThreadId), - File(i32), + File(FdNum), Invalid, } @@ -51,6 +54,8 @@ impl Handle { const PSEUDO_DISCRIMINANT: u32 = 1; const THREAD_DISCRIMINANT: u32 = 2; const FILE_DISCRIMINANT: u32 = 3; + // Chosen to ensure Handle::Invalid encodes to -1. Update this value if there are ever more than + // 8 discriminants. const INVALID_DISCRIMINANT: u32 = 7; fn discriminant(self) -> u32 { @@ -70,20 +75,25 @@ impl Handle { Self::Thread(thread) => thread.to_u32(), #[expect(clippy::cast_sign_loss)] Self::File(fd) => fd as u32, + // INVALID_HANDLE_VALUE is -1. This fact is explicitly declared or implied in several + // pages of Windows documentation. + // 1: https://learn.microsoft.com/en-us/dotnet/api/microsoft.win32.safehandles.safefilehandle?view=net-9.0 + // 2: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle?view=msvc-170 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 + // 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 for more detail on + // INVALID_HANDLE_VALUE. let variant_count = variant_count::(); - // however, std's ilog2 is floor(log2(x)) + // However, std's ilog2 is floor(log2(x)) let floor_log2 = variant_count.ilog2(); - // we need to add one for non powers of two to compensate for the difference + // We need to add one for non powers of two to compensate for the difference. #[expect(clippy::arithmetic_side_effects)] // cannot overflow if variant_count.is_power_of_two() { floor_log2 } else { floor_log2 + 1 } } @@ -105,7 +115,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 @@ -118,7 +128,11 @@ impl Handle { 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::FILE_DISCRIMINANT => { + // This cast preserves all bits. + assert_eq!(size_of_val(&data), size_of::()); + Some(Self::File(data as FdNum)) + } Self::INVALID_DISCRIMINANT => Some(Self::Invalid), _ => None, } @@ -221,9 +235,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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)?; + Handle::File(fd_num) => + if let Some(fd) = this.machine.fds.remove(fd_num) { + let err = fd.close(this.machine.communicate(), this)?; if let Err(e) = err { this.set_last_error(e)?; this.eval_windows("c", "FALSE") From cc6eae53d661dde499addd12e387b0eb3ef3024e Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 5 Dec 2024 20:04:38 -0800 Subject: [PATCH 06/22] Don't leak miri implementation details --- src/shims/windows/foreign_items.rs | 6 +++--- src/shims/windows/fs.rs | 2 +- src/shims/windows/handle.rs | 12 +++++------- src/shims/windows/thread.rs | 2 +- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/shims/windows/foreign_items.rs b/src/shims/windows/foreign_items.rs index 7e7cb69e8a..3af2d78bca 100644 --- a/src/shims/windows/foreign_items.rs +++ b/src/shims/windows/foreign_items.rs @@ -542,7 +542,7 @@ 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_handle(handle)?; + let handle = this.read_handle(handle, "SetThreadDescription")?; let name = this.read_wide_str(this.read_pointer(name)?)?; let thread = match handle { @@ -558,7 +558,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let [handle, name_ptr] = this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; - let handle = this.read_handle(handle)?; + let handle = this.read_handle(handle, "GetThreadDescription")?; let name_ptr = this.deref_pointer(name_ptr)?; // the pointer where we should store the ptr to the name let thread = match handle { @@ -674,7 +674,7 @@ 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_handle(handle)?; + let handle = this.read_handle(handle, "GetModuleFileNameW")?; let filename = this.read_pointer(filename)?; let size = this.read_scalar(size)?.to_u32()?; diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index 4522cde8c8..077be337cc 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -315,7 +315,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.assert_target_os("windows", "GetFileInformationByHandle"); this.check_no_isolation("`GetFileInformationByHandle`")?; - let file = this.read_handle(file)?; + let file = this.read_handle(file, "GetFileInformationByHandle")?; let file_information = this.deref_pointer_as( file_information, this.windows_ty_layout("BY_HANDLE_FILE_INFORMATION"), diff --git a/src/shims/windows/handle.rs b/src/shims/windows/handle.rs index 836ef11872..433cb09ecf 100644 --- a/src/shims/windows/handle.rs +++ b/src/shims/windows/handle.rs @@ -1,5 +1,4 @@ use std::mem::variant_count; -use std::panic::Location; use rustc_abi::HasDataLayout; @@ -201,21 +200,20 @@ 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> { + fn read_handle(&self, handle: &OpTy<'tcx>, function_name: &str) -> 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 {}", + "invalid handle {} passed to {function_name}", handle.to_target_isize(this)?, - Location::caller(), ))), Err(HandleError::ThreadNotFound(_)) => throw_machine_stop!(TerminationInfo::Abort(format!( - "invalid thread ID: {}", - Location::caller() + "invalid thread ID {} passed to {function_name}", + handle.to_target_isize(this)?, ))), } } @@ -229,7 +227,7 @@ 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_handle(handle_op)?; + let handle = this.read_handle(handle_op, "CloseHandle")?; let ret = match handle { Handle::Thread(thread) => { this.detach_thread(thread, /*allow_terminated_joined*/ true)?; diff --git a/src/shims/windows/thread.rs b/src/shims/windows/thread.rs index 2c4d5c2297..ff83befb88 100644 --- a/src/shims/windows/thread.rs +++ b/src/shims/windows/thread.rs @@ -62,7 +62,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let handle = this.read_handle(handle_op)?; + let handle = this.read_handle(handle_op, "WaitForSingleObject")?; let timeout = this.read_scalar(timeout_op)?.to_u32()?; let thread = match handle { From 36531697a99920314e4861b5b25bcbfa30b29cfa Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 5 Dec 2024 20:44:25 -0800 Subject: [PATCH 07/22] Fix clippy --- src/shims/windows/fs.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index 077be337cc..e8155f36ca 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -280,13 +280,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let handle = if is_dir && exists { let fh = &mut this.machine.fds; - let fd_num = fh.insert_new(DirHandle { path: file_name.into() }); + let fd_num = fh.insert_new(DirHandle { path: file_name }); Ok(Handle::File(fd_num)) } 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_num = fh.insert_new(MetadataHandle { path: file_name.into() }); + let fd_num = fh.insert_new(MetadataHandle { path: file_name }); Ok(Handle::File(fd_num)) } else { options.open(file_name).map(|file| { From a64dbd19213f21a98d6c1b9895111e59e533a8af Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Fri, 6 Dec 2024 13:02:29 -0800 Subject: [PATCH 08/22] Test file creation and metadata shims directly --- tests/pass/shims/windows-fs.rs | 131 +++++++++++++++++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 tests/pass/shims/windows-fs.rs diff --git a/tests/pass/shims/windows-fs.rs b/tests/pass/shims/windows-fs.rs new file mode 100644 index 0000000000..22576102d2 --- /dev/null +++ b/tests/pass/shims/windows-fs.rs @@ -0,0 +1,131 @@ +//@only-target: windows # this directly tests windows-only functions +//@compile-flags: -Zmiri-disable-isolation +#![allow(nonstandard_style)] + +use std::os::windows::ffi::OsStrExt; +use std::ptr; + +#[path = "../../utils/mod.rs"] +mod utils; + +// Windows API definitions. +type HANDLE = isize; +type BOOL = i32; +type DWORD = u32; +type LPCWSTR = *const u16; + +const GENERIC_READ: u32 = 2147483648u32; +const GENERIC_WRITE: u32 = 1073741824u32; +pub const FILE_SHARE_NONE: u32 = 0u32; +pub const FILE_SHARE_READ: u32 = 1u32; +pub const FILE_SHARE_WRITE: u32 = 2u32; +pub const OPEN_ALWAYS: u32 = 4u32; +pub const OPEN_EXISTING: u32 = 3u32; +pub const CREATE_NEW: u32 = 1u32; +pub const FILE_ATTRIBUTE_DIRECTORY: u32 = 16u32; +pub const FILE_ATTRIBUTE_NORMAL: u32 = 128u32; +pub const FILE_FLAG_BACKUP_SEMANTICS: u32 = 0x02000000u32; + +#[repr(C)] +struct FILETIME { + dwLowDateTime: DWORD, + dwHighDateTime: DWORD, +} + +#[repr(C)] +struct BY_HANDLE_FILE_INFORMATION { + dwFileAttributes: DWORD, + ftCreationTime: FILETIME, + ftLastAccessTime: FILETIME, + ftLastWriteTime: FILETIME, + dwVolumeSerialNumber: DWORD, + nFileSizeHigh: DWORD, + nFileSizeLow: DWORD, + nNumberOfLinks: DWORD, + nFileIndexHigh: DWORD, + nFileIndexLow: DWORD, +} + +#[repr(C)] +struct SECURITY_ATTRIBUTES { + nLength: DWORD, + lpSecurityDescriptor: *mut std::ffi::c_void, + bInheritHandle: BOOL, +} + +extern "system" { + fn CreateFileW( + file_name: LPCWSTR, + dwDesiredAccess: DWORD, + dwShareMode: DWORD, + lpSecurityAttributes: *mut SECURITY_ATTRIBUTES, + dwCreationDisposition: DWORD, + dwFlagsAndAttributes: DWORD, + hTemplateFile: HANDLE, + ) -> HANDLE; + fn GetFileInformationByHandle( + handle: HANDLE, + file_info: *mut BY_HANDLE_FILE_INFORMATION, + ) -> BOOL; + fn CloseHandle(handle: HANDLE) -> BOOL; + fn GetLastError() -> DWORD; +} + +fn main() { + unsafe { + test_create_dir_file(); + test_create_normal_file(); + } +} + +unsafe fn test_create_dir_file() { + let temp = utils::tmp(); + let mut raw_path = temp.as_os_str().encode_wide().collect::>(); + // encode_wide doesn't add a null-terminator + raw_path.push(0); + raw_path.push(0); + let handle = CreateFileW( + raw_path.as_ptr(), + GENERIC_READ, + FILE_SHARE_READ | FILE_SHARE_WRITE, + ptr::null_mut(), + OPEN_EXISTING, + FILE_FLAG_BACKUP_SEMANTICS, + 0, + ); + assert_ne!(handle, -1, "CreateNewW Failed: {}", GetLastError()); + let mut info = std::mem::zeroed::(); + if GetFileInformationByHandle(handle, &mut info) == 0 { + panic!("Failed to get file information") + }; + assert!(info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0); + if CloseHandle(handle) == 0 { + panic!("Failed to close file") + }; +} + +unsafe fn test_create_normal_file() { + let temp = utils::tmp().join("test.txt"); + let mut raw_path = temp.as_os_str().encode_wide().collect::>(); + // encode_wide doesn't add a null-terminator + raw_path.push(0); + raw_path.push(0); + let handle = CreateFileW( + raw_path.as_ptr(), + GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, + ptr::null_mut(), + CREATE_NEW, + 0, + 0, + ); + assert_ne!(handle, -1, "CreateNewW Failed: {}", GetLastError()); + let mut info = std::mem::zeroed::(); + if GetFileInformationByHandle(handle, &mut info) == 0 { + panic!("Failed to get file information: {}", GetLastError()) + }; + assert!(info.dwFileAttributes & FILE_ATTRIBUTE_NORMAL != 0); + if CloseHandle(handle) == 0 { + panic!("Failed to close file") + }; +} From 274d90f0d706d1d0b336dcd65f5e39c6c9b251f2 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sat, 7 Dec 2024 18:28:41 -0800 Subject: [PATCH 09/22] Move windows-fs to pass-dep and use windows_sys --- tests/{pass => pass-dep}/shims/windows-fs.rs | 68 ++------------------ 1 file changed, 6 insertions(+), 62 deletions(-) rename tests/{pass => pass-dep}/shims/windows-fs.rs (55%) diff --git a/tests/pass/shims/windows-fs.rs b/tests/pass-dep/shims/windows-fs.rs similarity index 55% rename from tests/pass/shims/windows-fs.rs rename to tests/pass-dep/shims/windows-fs.rs index 22576102d2..ad278887d6 100644 --- a/tests/pass/shims/windows-fs.rs +++ b/tests/pass-dep/shims/windows-fs.rs @@ -8,68 +8,12 @@ use std::ptr; #[path = "../../utils/mod.rs"] mod utils; -// Windows API definitions. -type HANDLE = isize; -type BOOL = i32; -type DWORD = u32; -type LPCWSTR = *const u16; - -const GENERIC_READ: u32 = 2147483648u32; -const GENERIC_WRITE: u32 = 1073741824u32; -pub const FILE_SHARE_NONE: u32 = 0u32; -pub const FILE_SHARE_READ: u32 = 1u32; -pub const FILE_SHARE_WRITE: u32 = 2u32; -pub const OPEN_ALWAYS: u32 = 4u32; -pub const OPEN_EXISTING: u32 = 3u32; -pub const CREATE_NEW: u32 = 1u32; -pub const FILE_ATTRIBUTE_DIRECTORY: u32 = 16u32; -pub const FILE_ATTRIBUTE_NORMAL: u32 = 128u32; -pub const FILE_FLAG_BACKUP_SEMANTICS: u32 = 0x02000000u32; - -#[repr(C)] -struct FILETIME { - dwLowDateTime: DWORD, - dwHighDateTime: DWORD, -} - -#[repr(C)] -struct BY_HANDLE_FILE_INFORMATION { - dwFileAttributes: DWORD, - ftCreationTime: FILETIME, - ftLastAccessTime: FILETIME, - ftLastWriteTime: FILETIME, - dwVolumeSerialNumber: DWORD, - nFileSizeHigh: DWORD, - nFileSizeLow: DWORD, - nNumberOfLinks: DWORD, - nFileIndexHigh: DWORD, - nFileIndexLow: DWORD, -} - -#[repr(C)] -struct SECURITY_ATTRIBUTES { - nLength: DWORD, - lpSecurityDescriptor: *mut std::ffi::c_void, - bInheritHandle: BOOL, -} - -extern "system" { - fn CreateFileW( - file_name: LPCWSTR, - dwDesiredAccess: DWORD, - dwShareMode: DWORD, - lpSecurityAttributes: *mut SECURITY_ATTRIBUTES, - dwCreationDisposition: DWORD, - dwFlagsAndAttributes: DWORD, - hTemplateFile: HANDLE, - ) -> HANDLE; - fn GetFileInformationByHandle( - handle: HANDLE, - file_info: *mut BY_HANDLE_FILE_INFORMATION, - ) -> BOOL; - fn CloseHandle(handle: HANDLE) -> BOOL; - fn GetLastError() -> DWORD; -} +use windows_sys::Win32::Foundation::{CloseHandle, GENERIC_READ, GENERIC_WRITE, GetLastError}; +use windows_sys::Win32::Storage::FileSystem::{ + BY_HANDLE_FILE_INFORMATION, CREATE_NEW, CreateFileW, FILE_ATTRIBUTE_DIRECTORY, + FILE_ATTRIBUTE_NORMAL, FILE_FLAG_BACKUP_SEMANTICS, FILE_SHARE_READ, FILE_SHARE_WRITE, + GetFileInformationByHandle, OPEN_EXISTING, +}; fn main() { unsafe { From 51273f5222c6e78a32d6079ef68fbea39729827b Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sat, 7 Dec 2024 18:30:22 -0800 Subject: [PATCH 10/22] Move FdNum to shims/files.rs, use it in FdTable definitions --- src/shims/files.rs | 17 ++++++++++++----- src/shims/windows/handle.rs | 4 +--- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/shims/files.rs b/src/shims/files.rs index f673b834be..30fdd15902 100644 --- a/src/shims/files.rs +++ b/src/shims/files.rs @@ -275,6 +275,9 @@ impl VisitProvenance for WeakFileDescriptionRef { } } +/// Internal type of a file-descriptor - this is what [`FdTable`] expects +pub type FdNum = i32; + /// A unique id for file descriptions. While we could use the address, considering that /// is definitely unique, the address would expose interpreter internal state when used /// for sorting things. So instead we generate a unique id per file description is the name @@ -325,12 +328,16 @@ impl FdTable { self.insert(fd_ref) } - pub fn insert(&mut self, fd_ref: FileDescriptionRef) -> i32 { + pub fn insert(&mut self, fd_ref: FileDescriptionRef) -> FdNum { self.insert_with_min_num(fd_ref, 0) } /// Insert a file description, giving it a file descriptor that is at least `min_fd_num`. - pub fn insert_with_min_num(&mut self, file_handle: FileDescriptionRef, min_fd_num: i32) -> i32 { + pub fn insert_with_min_num( + &mut self, + file_handle: FileDescriptionRef, + min_fd_num: FdNum, + ) -> FdNum { // Find the lowest unused FD, starting from min_fd. If the first such unused FD is in // between used FDs, the find_map combinator will return it. If the first such unused FD // is after all other used FDs, the find_map combinator will return None, and we will use @@ -356,16 +363,16 @@ impl FdTable { new_fd_num } - pub fn get(&self, fd_num: i32) -> Option { + pub fn get(&self, fd_num: FdNum) -> Option { let fd = self.fds.get(&fd_num)?; Some(fd.clone()) } - pub fn remove(&mut self, fd_num: i32) -> Option { + pub fn remove(&mut self, fd_num: FdNum) -> Option { self.fds.remove(&fd_num) } - pub fn is_fd_num(&self, fd_num: i32) -> bool { + pub fn is_fd_num(&self, fd_num: FdNum) -> bool { self.fds.contains_key(&fd_num) } } diff --git a/src/shims/windows/handle.rs b/src/shims/windows/handle.rs index 433cb09ecf..f45e1a29c7 100644 --- a/src/shims/windows/handle.rs +++ b/src/shims/windows/handle.rs @@ -3,11 +3,9 @@ use std::mem::variant_count; use rustc_abi::HasDataLayout; use crate::concurrency::thread::ThreadNotFound; +use crate::shims::files::FdNum; use crate::*; -/// Internal type of a file-descriptor - this is what [`FdTable`](shims::files::FdTable) expects -type FdNum = i32; - #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] pub enum PseudoHandle { CurrentThread, From aa6bbf6cfc80ae8887fb1b5ee39f80499274903e Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sat, 7 Dec 2024 20:22:58 -0800 Subject: [PATCH 11/22] Slightly improve flag handling - parse and validate in one place --- src/shims/windows/fs.rs | 91 +++++++----------------------- tests/pass-dep/shims/windows-fs.rs | 8 +-- 2 files changed, 25 insertions(+), 74 deletions(-) diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index e8155f36ca..229d3e7013 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -22,51 +22,6 @@ impl FileDescription for FileHandle { "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, @@ -177,37 +132,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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 @@ -215,6 +149,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let file_flag_open_reparse_point = this.eval_windows_u32("c", "FILE_FLAG_OPEN_REPARSE_POINT"); + if share_mode != (file_share_delete | file_share_read | file_share_write) { + 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 flags_and_attributes = match flags_and_attributes { 0 => file_attribute_normal, _ => flags_and_attributes, @@ -238,8 +179,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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(); @@ -249,13 +188,21 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } let mut options = OpenOptions::new(); - if desired_read { + if desired_access & generic_read != 0 { + desired_access &= !generic_read; options.read(true); } - if desired_write { + if desired_access & generic_write != 0 { + desired_access &= !generic_write; options.write(true); } + if desired_access != 0 { + throw_unsup_format!( + "CreateFileW: Unsupported bits set for access mode: {desired_access:#x}" + ); + } + if creation_disposition == create_always { if file_name.exists() { this.set_last_error(IoError::WindowsError("ERROR_ALREADY_EXISTS"))?; @@ -276,6 +223,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Nothing } else if creation_disposition == truncate_existing { options.truncate(true); + } else { + throw_unsup_format!( + "CreateFileW: Unsupported creation disposition: {creation_disposition}" + ); } let handle = if is_dir && exists { diff --git a/tests/pass-dep/shims/windows-fs.rs b/tests/pass-dep/shims/windows-fs.rs index ad278887d6..2c1cc386dc 100644 --- a/tests/pass-dep/shims/windows-fs.rs +++ b/tests/pass-dep/shims/windows-fs.rs @@ -11,8 +11,8 @@ mod utils; use windows_sys::Win32::Foundation::{CloseHandle, GENERIC_READ, GENERIC_WRITE, GetLastError}; use windows_sys::Win32::Storage::FileSystem::{ BY_HANDLE_FILE_INFORMATION, CREATE_NEW, CreateFileW, FILE_ATTRIBUTE_DIRECTORY, - FILE_ATTRIBUTE_NORMAL, FILE_FLAG_BACKUP_SEMANTICS, FILE_SHARE_READ, FILE_SHARE_WRITE, - GetFileInformationByHandle, OPEN_EXISTING, + FILE_ATTRIBUTE_NORMAL, FILE_FLAG_BACKUP_SEMANTICS, FILE_SHARE_DELETE, FILE_SHARE_READ, + FILE_SHARE_WRITE, GetFileInformationByHandle, OPEN_EXISTING, }; fn main() { @@ -31,7 +31,7 @@ unsafe fn test_create_dir_file() { let handle = CreateFileW( raw_path.as_ptr(), GENERIC_READ, - FILE_SHARE_READ | FILE_SHARE_WRITE, + FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, ptr::null_mut(), OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, @@ -57,7 +57,7 @@ unsafe fn test_create_normal_file() { let handle = CreateFileW( raw_path.as_ptr(), GENERIC_READ | GENERIC_WRITE, - FILE_SHARE_READ | FILE_SHARE_WRITE, + FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, ptr::null_mut(), CREATE_NEW, 0, From 938430f49f97afd90d7d0e314ea4e8abdd5bbe7e Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sat, 7 Dec 2024 20:29:43 -0800 Subject: [PATCH 12/22] Fixup imports, compile --- src/shims/windows/fs.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index 229d3e7013..a2ac9fc51a 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -1,12 +1,10 @@ use std::fs::{File, Metadata, OpenOptions}; use std::io; -use std::io::{IsTerminal, Read, Seek, SeekFrom, Write}; +use std::io::IsTerminal; use std::path::PathBuf; use std::time::SystemTime; -use rustc_abi::Size; - -use crate::shims::files::{EvalContextExt as _, FileDescription, FileDescriptionRef}; +use crate::shims::files::{FileDescription, FileDescriptionRef}; use crate::shims::time::system_time_to_duration; use crate::shims::windows::handle::{EvalContextExt as _, Handle}; use crate::*; @@ -122,7 +120,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.check_no_isolation("`CreateFileW`")?; 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 mut 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()?; @@ -187,12 +185,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { return interp_ok(Handle::Invalid); } + let desired_read = desired_access & generic_read != 0; + let desired_write = desired_access & generic_write != 0; + let mut options = OpenOptions::new(); - if desired_access & generic_read != 0 { + if desired_read { desired_access &= !generic_read; options.read(true); } - if desired_access & generic_write != 0 { + if desired_write { desired_access &= !generic_write; options.write(true); } @@ -233,7 +234,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let fh = &mut this.machine.fds; let fd_num = fh.insert_new(DirHandle { path: file_name }); Ok(Handle::File(fd_num)) - } else if creation_disposition == open_existing && desired_access == 0 { + } else if creation_disposition == open_existing && !(desired_read || desired_write) { // Windows supports handles with no permissions. These allow things such as reading // metadata, but not file content. let fh = &mut this.machine.fds; From 3c2ed8abbd3ba4d753fb51b718d691a94bb60476 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sat, 7 Dec 2024 20:43:52 -0800 Subject: [PATCH 13/22] Make metadata handle store the metadata, instead of just a path. Add test for this case. --- src/shims/windows/fs.rs | 12 +++++---- tests/pass-dep/shims/windows-fs.rs | 41 ++++++++++++++++++++++-------- 2 files changed, 38 insertions(+), 15 deletions(-) diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index a2ac9fc51a..2764aac18f 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -4,7 +4,7 @@ use std::io::IsTerminal; use std::path::PathBuf; use std::time::SystemTime; -use crate::shims::files::{FileDescription, FileDescriptionRef}; +use crate::shims::files::FileDescription; use crate::shims::time::system_time_to_duration; use crate::shims::windows::handle::{EvalContextExt as _, Handle}; use crate::*; @@ -80,7 +80,7 @@ impl FileDescription for DirHandle { #[derive(Debug)] pub struct MetadataHandle { - pub(crate) path: PathBuf, + pub(crate) meta: Metadata, } impl FileDescription for MetadataHandle { @@ -89,7 +89,7 @@ impl FileDescription for MetadataHandle { } fn metadata<'tcx>(&self) -> InterpResult<'tcx, io::Result> { - interp_ok(self.path.metadata()) + interp_ok(Ok(self.meta.clone())) } fn close<'tcx>( @@ -238,8 +238,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // 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_num = fh.insert_new(MetadataHandle { path: file_name }); - Ok(Handle::File(fd_num)) + file_name.metadata().map(|meta| { + let fd_num = fh.insert_new(MetadataHandle { meta }); + Handle::File(fd_num) + }) } else { options.open(file_name).map(|file| { let fh = &mut this.machine.fds; diff --git a/tests/pass-dep/shims/windows-fs.rs b/tests/pass-dep/shims/windows-fs.rs index 2c1cc386dc..07f1aa41a0 100644 --- a/tests/pass-dep/shims/windows-fs.rs +++ b/tests/pass-dep/shims/windows-fs.rs @@ -3,6 +3,7 @@ #![allow(nonstandard_style)] use std::os::windows::ffi::OsStrExt; +use std::path::Path; use std::ptr; #[path = "../../utils/mod.rs"] @@ -24,10 +25,7 @@ fn main() { unsafe fn test_create_dir_file() { let temp = utils::tmp(); - let mut raw_path = temp.as_os_str().encode_wide().collect::>(); - // encode_wide doesn't add a null-terminator - raw_path.push(0); - raw_path.push(0); + let raw_path = to_wide_cstr(&temp); let handle = CreateFileW( raw_path.as_ptr(), GENERIC_READ, @@ -37,7 +35,7 @@ unsafe fn test_create_dir_file() { FILE_FLAG_BACKUP_SEMANTICS, 0, ); - assert_ne!(handle, -1, "CreateNewW Failed: {}", GetLastError()); + assert_ne!(handle, -1, "CreateFileW Failed: {}", GetLastError()); let mut info = std::mem::zeroed::(); if GetFileInformationByHandle(handle, &mut info) == 0 { panic!("Failed to get file information") @@ -50,10 +48,7 @@ unsafe fn test_create_dir_file() { unsafe fn test_create_normal_file() { let temp = utils::tmp().join("test.txt"); - let mut raw_path = temp.as_os_str().encode_wide().collect::>(); - // encode_wide doesn't add a null-terminator - raw_path.push(0); - raw_path.push(0); + let raw_path = to_wide_cstr(&temp); let handle = CreateFileW( raw_path.as_ptr(), GENERIC_READ | GENERIC_WRITE, @@ -63,7 +58,7 @@ unsafe fn test_create_normal_file() { 0, 0, ); - assert_ne!(handle, -1, "CreateNewW Failed: {}", GetLastError()); + assert_ne!(handle, -1, "CreateFileW Failed: {}", GetLastError()); let mut info = std::mem::zeroed::(); if GetFileInformationByHandle(handle, &mut info) == 0 { panic!("Failed to get file information: {}", GetLastError()) @@ -72,4 +67,30 @@ unsafe fn test_create_normal_file() { if CloseHandle(handle) == 0 { panic!("Failed to close file") }; + + // Test metadata-only handle + let handle = CreateFileW( + raw_path.as_ptr(), + 0, + FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, + ptr::null_mut(), + OPEN_EXISTING, + 0, + 0, + ); + assert_ne!(handle, -1, "CreateFileW Failed: {}", GetLastError()); + let mut info = std::mem::zeroed::(); + if GetFileInformationByHandle(handle, &mut info) == 0 { + panic!("Failed to get file information: {}", GetLastError()) + }; + assert!(info.dwFileAttributes & FILE_ATTRIBUTE_NORMAL != 0); + if CloseHandle(handle) == 0 { + panic!("Failed to close file") + }; +} + +fn to_wide_cstr(path: &Path) -> Vec { + let mut raw_path = path.as_os_str().encode_wide().collect::>(); + raw_path.extend([0, 0]); + raw_path } From ebfc7681f37753e02b39c103968babc4730622a6 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sat, 7 Dec 2024 20:48:09 -0800 Subject: [PATCH 14/22] Improve extract_windows_epoch impl --- src/shims/windows/fs.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index 2764aac18f..83d6d93c5e 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -332,11 +332,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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; + // (seconds in a year * (years between 1970 and 1601)) + (89 leap days in that span) * 10 million (nanoseconds/second / 100) + const TIME_TO_EPOCH: u64 = (3600 * 24 * 365 * (1970 - 1601) + (3600 * 24 * 89)) * 10_000_000; match time.ok() { Some(time) => { let duration = system_time_to_duration(&time)?; + // 10 million is the number of 100ns periods per second. 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); From d989984247fb616f8b7e1608937ba48a6e4a472f Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sat, 7 Dec 2024 20:51:57 -0800 Subject: [PATCH 15/22] Improve extract_windows_epoch impl comments --- src/shims/windows/fs.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index 83d6d93c5e..2d235b6fa6 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -332,8 +332,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn extract_windows_epoch<'tcx>( time: io::Result, ) -> InterpResult<'tcx, Option<(u32, u32)>> { - // (seconds in a year * (years between 1970 and 1601)) + (89 leap days in that span) * 10 million (nanoseconds/second / 100) - const TIME_TO_EPOCH: u64 = (3600 * 24 * 365 * (1970 - 1601) + (3600 * 24 * 89)) * 10_000_000; + // (seconds in a year * years between 1970 and 1601) + (89 leap days in that span) + const SECONDS_TO_EPOCH: u64 = 3600 * 24 * 365 * (1970 - 1601) + 3600 * 24 * 89; + // seconds between unix and windows epochs * 10 million (nanoseconds/second / 100) + const TIME_TO_EPOCH: u64 = SECONDS_TO_EPOCH * 10_000_000; match time.ok() { Some(time) => { let duration = system_time_to_duration(&time)?; From e5ada7618b3a76cc6b41c7bad6be187e9fae9e3f Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sun, 8 Dec 2024 15:06:18 -0800 Subject: [PATCH 16/22] Add invalid handle encoding test --- src/shims/windows/handle.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/shims/windows/handle.rs b/src/shims/windows/handle.rs index f45e1a29c7..f2a114ffb7 100644 --- a/src/shims/windows/handle.rs +++ b/src/shims/windows/handle.rs @@ -249,3 +249,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { interp_ok(ret) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_invalid_encoding() { + // Ensure the invalid handle encodes to `u32::MAX`/`INVALID_HANDLE_VALUE`. + assert_eq!(Handle::Invalid.to_packed(), 0xFFFFFFFF) + } +} From 52c167603237e82abf3680f3e117be264e42703e Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sun, 8 Dec 2024 15:46:16 -0800 Subject: [PATCH 17/22] Add tests for CREATE_ALWAYS and OPEN_ALWAYS error behavior. Add comments to code about various quirks --- src/shims/windows/fs.rs | 33 ++++++++++-- tests/pass-dep/shims/windows-fs.rs | 80 ++++++++++++++++++++++++++++-- 2 files changed, 106 insertions(+), 7 deletions(-) diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index 2d235b6fa6..f217135a21 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -177,7 +177,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { throw_unsup_format!("CreateFileW: Template files are not supported"); } - let exists = file_name.exists(); let is_dir = file_name.is_dir(); if flags_and_attributes == file_attribute_normal && is_dir { @@ -204,20 +203,46 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ); } + // This is racy, but there doesn't appear to be an std API that both succeeds if a file + // exists but tells us it isn't new. Either we accept racing one way or another, or we + // use an iffy heuristic like file creation time. This implementation prefers to fail in the + // direction of erroring more often. + let exists = file_name.exists(); + if creation_disposition == create_always { - if file_name.exists() { + // Per the documentation: + // If the specified file exists and is writable, the function truncates the file, the + // function succeeds, and last-error code is set to ERROR_ALREADY_EXISTS. + // If the specified file does not exist and is a valid path, a new file is created, the + // function succeeds, and the last-error code is set to zero. + // https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew + if exists { this.set_last_error(IoError::WindowsError("ERROR_ALREADY_EXISTS"))?; + } else { + this.set_last_error(IoError::Raw(Scalar::from_u32(0)))?; } options.create(true); options.truncate(true); } else if creation_disposition == create_new { options.create_new(true); + // Per `create_new` documentation: + // If the specified file does not exist and is a valid path to a writable location, the + // function creates a file and the last-error code is set to zero. + // https://doc.rust-lang.org/std/fs/struct.OpenOptions.html#method.create_new if !desired_write { options.append(true); } } else if creation_disposition == open_always { - if file_name.exists() { + // Per the documentation: + // If the specified file exists, the function succeeds and the last-error code is set + // to ERROR_ALREADY_EXISTS. + // If the specified file does not exist and is a valid path to a writable location, the + // function creates a file and the last-error code is set to zero. + // https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew + if exists { this.set_last_error(IoError::WindowsError("ERROR_ALREADY_EXISTS"))?; + } else { + this.set_last_error(IoError::Raw(Scalar::from_u32(0)))?; } options.create(true); } else if creation_disposition == open_existing { @@ -230,7 +255,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ); } - let handle = if is_dir && exists { + let handle = if is_dir { let fh = &mut this.machine.fds; let fd_num = fh.insert_new(DirHandle { path: file_name }); Ok(Handle::File(fd_num)) diff --git a/tests/pass-dep/shims/windows-fs.rs b/tests/pass-dep/shims/windows-fs.rs index 07f1aa41a0..c91c2de8c2 100644 --- a/tests/pass-dep/shims/windows-fs.rs +++ b/tests/pass-dep/shims/windows-fs.rs @@ -9,17 +9,21 @@ use std::ptr; #[path = "../../utils/mod.rs"] mod utils; -use windows_sys::Win32::Foundation::{CloseHandle, GENERIC_READ, GENERIC_WRITE, GetLastError}; +use windows_sys::Win32::Foundation::{ + CloseHandle, ERROR_ALREADY_EXISTS, GENERIC_READ, GENERIC_WRITE, GetLastError, +}; use windows_sys::Win32::Storage::FileSystem::{ - BY_HANDLE_FILE_INFORMATION, CREATE_NEW, CreateFileW, FILE_ATTRIBUTE_DIRECTORY, + BY_HANDLE_FILE_INFORMATION, CREATE_ALWAYS, CREATE_NEW, CreateFileW, FILE_ATTRIBUTE_DIRECTORY, FILE_ATTRIBUTE_NORMAL, FILE_FLAG_BACKUP_SEMANTICS, FILE_SHARE_DELETE, FILE_SHARE_READ, - FILE_SHARE_WRITE, GetFileInformationByHandle, OPEN_EXISTING, + FILE_SHARE_WRITE, GetFileInformationByHandle, OPEN_ALWAYS, OPEN_EXISTING, }; fn main() { unsafe { test_create_dir_file(); test_create_normal_file(); + test_create_always_twice(); + test_open_always_twice(); } } @@ -89,6 +93,76 @@ unsafe fn test_create_normal_file() { }; } +/// Tests that CREATE_ALWAYS sets the error value correctly based on whether the file already exists +unsafe fn test_create_always_twice() { + let temp = utils::tmp().join("test_create_always.txt"); + let raw_path = to_wide_cstr(&temp); + let handle = CreateFileW( + raw_path.as_ptr(), + GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, + ptr::null_mut(), + CREATE_ALWAYS, + 0, + 0, + ); + assert_ne!(handle, -1, "CreateFileW Failed: {}", GetLastError()); + assert_eq!(GetLastError(), 0); + if CloseHandle(handle) == 0 { + panic!("Failed to close file") + }; + + let handle = CreateFileW( + raw_path.as_ptr(), + GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, + ptr::null_mut(), + CREATE_ALWAYS, + 0, + 0, + ); + assert_ne!(handle, -1, "CreateFileW Failed: {}", GetLastError()); + assert_eq!(GetLastError(), ERROR_ALREADY_EXISTS); + if CloseHandle(handle) == 0 { + panic!("Failed to close file") + }; +} + +/// Tests that OPEN_ALWAYS sets the error value correctly based on whether the file already exists +unsafe fn test_open_always_twice() { + let temp = utils::tmp().join("test_open_always.txt"); + let raw_path = to_wide_cstr(&temp); + let handle = CreateFileW( + raw_path.as_ptr(), + GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, + ptr::null_mut(), + OPEN_ALWAYS, + 0, + 0, + ); + assert_ne!(handle, -1, "CreateFileW Failed: {}", GetLastError()); + assert_eq!(GetLastError(), 0); + if CloseHandle(handle) == 0 { + panic!("Failed to close file") + }; + + let handle = CreateFileW( + raw_path.as_ptr(), + GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, + ptr::null_mut(), + OPEN_ALWAYS, + 0, + 0, + ); + assert_ne!(handle, -1, "CreateFileW Failed: {}", GetLastError()); + assert_eq!(GetLastError(), ERROR_ALREADY_EXISTS); + if CloseHandle(handle) == 0 { + panic!("Failed to close file") + }; +} + fn to_wide_cstr(path: &Path) -> Vec { let mut raw_path = path.as_os_str().encode_wide().collect::>(); raw_path.extend([0, 0]); From 5ac99dad08a4241a8148869c76a81938eb3c9109 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sun, 8 Dec 2024 15:58:58 -0800 Subject: [PATCH 18/22] Extract Windows epoch helpers from GetSystemTimeAsFileTime and use them in file APIs --- src/shims/time.rs | 36 ++++++++++++++++++++++++++---------- src/shims/windows/fs.rs | 21 +++++++-------------- 2 files changed, 33 insertions(+), 24 deletions(-) diff --git a/src/shims/time.rs b/src/shims/time.rs index 6436823b0f..f58527ebc0 100644 --- a/src/shims/time.rs +++ b/src/shims/time.rs @@ -218,16 +218,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let filetime = this.deref_pointer_as(LPFILETIME_op, this.windows_ty_layout("FILETIME"))?; - let NANOS_PER_SEC = this.eval_windows_u64("time", "NANOS_PER_SEC"); - let INTERVALS_PER_SEC = this.eval_windows_u64("time", "INTERVALS_PER_SEC"); - let INTERVALS_TO_UNIX_EPOCH = this.eval_windows_u64("time", "INTERVALS_TO_UNIX_EPOCH"); - let NANOS_PER_INTERVAL = NANOS_PER_SEC / INTERVALS_PER_SEC; - let SECONDS_TO_UNIX_EPOCH = INTERVALS_TO_UNIX_EPOCH / INTERVALS_PER_SEC; - - let duration = system_time_to_duration(&SystemTime::now())? - + Duration::from_secs(SECONDS_TO_UNIX_EPOCH); - let duration_ticks = u64::try_from(duration.as_nanos() / u128::from(NANOS_PER_INTERVAL)) - .map_err(|_| err_unsup_format!("programs running more than 2^64 Windows ticks after the Windows epoch are not supported"))?; + let duration = this.system_time_since_windows_epoch(&SystemTime::now())?; + let duration_ticks = this.windows_ticks_for(duration)?; let dwLowDateTime = u32::try_from(duration_ticks & 0x00000000FFFFFFFF).unwrap(); let dwHighDateTime = u32::try_from((duration_ticks & 0xFFFFFFFF00000000) >> 32).unwrap(); @@ -276,6 +268,30 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { interp_ok(Scalar::from_i32(-1)) // Return non-zero on success } + #[allow(non_snake_case, clippy::arithmetic_side_effects)] + fn system_time_since_windows_epoch(&self, time: &SystemTime) -> InterpResult<'tcx, Duration> { + let this = self.eval_context_ref(); + + let INTERVALS_PER_SEC = this.eval_windows_u64("time", "INTERVALS_PER_SEC"); + let INTERVALS_TO_UNIX_EPOCH = this.eval_windows_u64("time", "INTERVALS_TO_UNIX_EPOCH"); + let SECONDS_TO_UNIX_EPOCH = INTERVALS_TO_UNIX_EPOCH / INTERVALS_PER_SEC; + + interp_ok(system_time_to_duration(time)? + Duration::from_secs(SECONDS_TO_UNIX_EPOCH)) + } + + #[allow(non_snake_case, clippy::arithmetic_side_effects)] + fn windows_ticks_for(&self, duration: Duration) -> InterpResult<'tcx, u64> { + let this = self.eval_context_ref(); + + let NANOS_PER_SEC = this.eval_windows_u64("time", "NANOS_PER_SEC"); + let INTERVALS_PER_SEC = this.eval_windows_u64("time", "INTERVALS_PER_SEC"); + let NANOS_PER_INTERVAL = NANOS_PER_SEC / INTERVALS_PER_SEC; + + let ticks = u64::try_from(duration.as_nanos() / u128::from(NANOS_PER_INTERVAL)) + .map_err(|_| err_unsup_format!("programs running more than 2^64 Windows ticks after the Windows epoch are not supported"))?; + interp_ok(ticks) + } + fn mach_absolute_time(&self) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_ref(); diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index f217135a21..076d95cf2a 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -5,7 +5,6 @@ use std::path::PathBuf; use std::time::SystemTime; use crate::shims::files::FileDescription; -use crate::shims::time::system_time_to_duration; use crate::shims::windows::handle::{EvalContextExt as _, Handle}; use crate::*; @@ -329,9 +328,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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)); + let created = extract_windows_epoch(this, metadata.created())?.unwrap_or((0, 0)); + let accessed = extract_windows_epoch(this, metadata.accessed())?.unwrap_or((0, 0)); + let written = extract_windows_epoch(this, metadata.modified())?.unwrap_or((0, 0)); this.write_int_fields_named(&[("dwFileAttributes", attributes.into())], &file_information)?; write_filetime_field(this, &file_information, "ftCreationTime", created)?; @@ -355,21 +354,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { /// Windows FILETIME is measured in 100-nanosecs since 1601 fn extract_windows_epoch<'tcx>( + ecx: &MiriInterpCx<'tcx>, time: io::Result, ) -> InterpResult<'tcx, Option<(u32, u32)>> { - // (seconds in a year * years between 1970 and 1601) + (89 leap days in that span) - const SECONDS_TO_EPOCH: u64 = 3600 * 24 * 365 * (1970 - 1601) + 3600 * 24 * 89; - // seconds between unix and windows epochs * 10 million (nanoseconds/second / 100) - const TIME_TO_EPOCH: u64 = SECONDS_TO_EPOCH * 10_000_000; match time.ok() { Some(time) => { - let duration = system_time_to_duration(&time)?; - // 10 million is the number of 100ns periods per second. - 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); + let duration = ecx.system_time_since_windows_epoch(&time)?; + let duration_ticks = ecx.windows_ticks_for(duration)?; #[allow(clippy::cast_possible_truncation)] - interp_ok(Some((total as u32, (total >> 32) as u32))) + interp_ok(Some((duration_ticks as u32, (duration_ticks >> 32) as u32))) } None => interp_ok(None), } From ef5ab7fad60932e875b91cbe5e3434ce59fa51b6 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Mon, 9 Dec 2024 12:56:52 -0800 Subject: [PATCH 19/22] Merge FileHandle implementation between Unix and Windows --- src/shims/files.rs | 104 ++++++++++++++++++++++++++- src/shims/unix/fd.rs | 6 +- src/shims/unix/fs.rs | 102 +------------------------- src/shims/unix/linux_like/epoll.rs | 4 +- src/shims/unix/linux_like/eventfd.rs | 2 +- src/shims/unix/unnamed_socket.rs | 2 +- src/shims/windows/fs.rs | 51 +------------ 7 files changed, 114 insertions(+), 157 deletions(-) diff --git a/src/shims/files.rs b/src/shims/files.rs index 30fdd15902..28d8ed4208 100644 --- a/src/shims/files.rs +++ b/src/shims/files.rs @@ -1,6 +1,7 @@ use std::any::Any; use std::collections::BTreeMap; -use std::io::{IsTerminal, Read, SeekFrom, Write}; +use std::fs::{File, Metadata}; +use std::io::{IsTerminal, Read, Seek, SeekFrom, Write}; use std::ops::Deref; use std::rc::{Rc, Weak}; use std::{fs, io}; @@ -72,7 +73,7 @@ pub trait FileDescription: std::fmt::Debug + Any { false } - fn as_unix(&self) -> &dyn UnixFileDescription { + fn as_unix<'tcx>(&self, _ecx: &MiriInterpCx<'tcx>) -> &dyn UnixFileDescription { panic!("Not a unix file descriptor: {}", self.name()); } } @@ -178,6 +179,105 @@ impl FileDescription for io::Stderr { } } +#[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() + } + + fn as_unix<'tcx>(&self, ecx: &MiriInterpCx<'tcx>) -> &dyn UnixFileDescription { + assert!( + ecx.target_os_is_unix(), + "unix file operations are only available for unix targets" + ); + self + } +} + /// Like /dev/null #[derive(Debug)] pub struct NullOutput; diff --git a/src/shims/unix/fd.rs b/src/shims/unix/fd.rs index e5dead1a26..27e94bade8 100644 --- a/src/shims/unix/fd.rs +++ b/src/shims/unix/fd.rs @@ -121,7 +121,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { throw_unsup_format!("unsupported flags {:#x}", op); }; - let result = fd.as_unix().flock(this.machine.communicate(), parsed_op)?; + let result = fd.as_unix(this).flock(this.machine.communicate(), parsed_op)?; drop(fd); // return `0` if flock is successful let result = result.map(|()| 0i32); @@ -251,7 +251,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let Ok(offset) = u64::try_from(offset) else { return this.set_last_error_and_return(LibcError("EINVAL"), dest); }; - fd.as_unix().pread(communicate, offset, buf, count, dest, this)? + fd.as_unix(this).pread(communicate, offset, buf, count, dest, this)? } }; interp_ok(()) @@ -291,7 +291,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let Ok(offset) = u64::try_from(offset) else { return this.set_last_error_and_return(LibcError("EINVAL"), dest); }; - fd.as_unix().pwrite(communicate, buf, count, offset, dest, this)? + fd.as_unix(this).pwrite(communicate, buf, count, offset, dest, this)? } }; interp_ok(()) diff --git a/src/shims/unix/fs.rs b/src/shims/unix/fs.rs index b41a4d2246..6edd394bb6 100644 --- a/src/shims/unix/fs.rs +++ b/src/shims/unix/fs.rs @@ -2,10 +2,9 @@ use std::borrow::Cow; use std::fs::{ - DirBuilder, File, FileType, Metadata, OpenOptions, ReadDir, read_dir, remove_dir, remove_file, - rename, + DirBuilder, File, FileType, OpenOptions, ReadDir, read_dir, remove_dir, remove_file, rename, }; -use std::io::{self, ErrorKind, IsTerminal, Read, Seek, SeekFrom, Write}; +use std::io::{self, ErrorKind, Read, Seek, SeekFrom, Write}; use std::path::{Path, PathBuf}; use std::time::SystemTime; @@ -14,106 +13,11 @@ use rustc_data_structures::fx::FxHashMap; use self::shims::time::system_time_to_duration; use crate::helpers::check_min_arg_count; -use crate::shims::files::{EvalContextExt as _, FileDescription, FileDescriptionRef}; +use crate::shims::files::{EvalContextExt as _, FileHandle}; use crate::shims::os_str::bytes_to_os_str; use crate::shims::unix::fd::{FlockOp, UnixFileDescription}; use crate::*; -#[derive(Debug)] -struct FileHandle { - file: File, - 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() - } - - fn as_unix(&self) -> &dyn UnixFileDescription { - self - } -} - impl UnixFileDescription for FileHandle { fn pread<'tcx>( &self, diff --git a/src/shims/unix/linux_like/epoll.rs b/src/shims/unix/linux_like/epoll.rs index 5b240351c2..27c6ed185f 100644 --- a/src/shims/unix/linux_like/epoll.rs +++ b/src/shims/unix/linux_like/epoll.rs @@ -153,7 +153,7 @@ impl FileDescription for Epoll { interp_ok(Ok(())) } - fn as_unix(&self) -> &dyn UnixFileDescription { + fn as_unix<'tcx>(&self, _ecx: &MiriInterpCx<'tcx>) -> &dyn UnixFileDescription { self } } @@ -601,7 +601,7 @@ fn check_and_update_one_event_interest<'tcx>( ecx: &MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, bool> { // Get the bitmask of ready events for a file description. - let ready_events_bitmask = fd_ref.as_unix().get_epoll_ready_events()?.get_event_bitmask(ecx); + let ready_events_bitmask = fd_ref.as_unix(ecx).get_epoll_ready_events()?.get_event_bitmask(ecx); let epoll_event_interest = interest.borrow(); // This checks if any of the events specified in epoll_event_interest.events // match those in ready_events. diff --git a/src/shims/unix/linux_like/eventfd.rs b/src/shims/unix/linux_like/eventfd.rs index 4bbe417ea8..2d41380581 100644 --- a/src/shims/unix/linux_like/eventfd.rs +++ b/src/shims/unix/linux_like/eventfd.rs @@ -111,7 +111,7 @@ impl FileDescription for Event { eventfd_write(num, buf_place, dest, weak_eventfd, ecx) } - fn as_unix(&self) -> &dyn UnixFileDescription { + fn as_unix<'tcx>(&self, _ecx: &MiriInterpCx<'tcx>) -> &dyn UnixFileDescription { self } } diff --git a/src/shims/unix/unnamed_socket.rs b/src/shims/unix/unnamed_socket.rs index 24304c0c04..87357e8863 100644 --- a/src/shims/unix/unnamed_socket.rs +++ b/src/shims/unix/unnamed_socket.rs @@ -167,7 +167,7 @@ impl FileDescription for AnonSocket { anonsocket_write(available_space, &peer_fd, ptr, len, dest, ecx) } - fn as_unix(&self) -> &dyn UnixFileDescription { + fn as_unix<'tcx>(&self, _ecx: &MiriInterpCx<'tcx>) -> &dyn UnixFileDescription { self } } diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index 076d95cf2a..2e26c13451 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -1,59 +1,12 @@ -use std::fs::{File, Metadata, OpenOptions}; +use std::fs::{Metadata, OpenOptions}; use std::io; -use std::io::IsTerminal; use std::path::PathBuf; use std::time::SystemTime; -use crate::shims::files::FileDescription; +use crate::shims::files::{FileDescription, FileHandle}; 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 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, From 6ee99aaf032b0d433e23b568043dfb8d98141cfd Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 12 Dec 2024 22:45:39 -0800 Subject: [PATCH 20/22] Use u32::MAX constant --- src/shims/windows/handle.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shims/windows/handle.rs b/src/shims/windows/handle.rs index f2a114ffb7..98a1d67cdf 100644 --- a/src/shims/windows/handle.rs +++ b/src/shims/windows/handle.rs @@ -257,6 +257,6 @@ mod tests { #[test] fn test_invalid_encoding() { // Ensure the invalid handle encodes to `u32::MAX`/`INVALID_HANDLE_VALUE`. - assert_eq!(Handle::Invalid.to_packed(), 0xFFFFFFFF) + assert_eq!(Handle::Invalid.to_packed(), u32::MAX) } } From a61daf25f6d61c87d52cf8ceda281d190c362916 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sun, 15 Dec 2024 19:27:43 -0800 Subject: [PATCH 21/22] Some fs improvements --- Cargo.toml | 1 + src/shims/windows/fs.rs | 204 +++++++++++++++++++++++++--------------- 2 files changed, 128 insertions(+), 77 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d1cd364034..60ccec672a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,6 +26,7 @@ measureme = "11" chrono = { version = "0.4.38", default-features = false } chrono-tz = "0.10" directories = "5" +bitflags = "2.6" # Copied from `compiler/rustc/Cargo.toml`. # But only for some targets, it fails for others. Rustc configures this in its CI, but we can't diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index 2e26c13451..9b80e066da 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -1,8 +1,11 @@ use std::fs::{Metadata, OpenOptions}; use std::io; +use std::io::ErrorKind; use std::path::PathBuf; use std::time::SystemTime; +use bitflags::bitflags; + use crate::shims::files::{FileDescription, FileHandle}; use crate::shims::windows::handle::{EvalContextExt as _, Handle}; use crate::*; @@ -30,6 +33,9 @@ impl FileDescription for DirHandle { } } +/// Windows supports handles without any read/write/delete permissions - these handles can get +/// metadata, but little else. We represent that by storing the metadata from the time the handle +/// was opened. #[derive(Debug)] pub struct MetadataHandle { pub(crate) meta: Metadata, @@ -53,6 +59,88 @@ impl FileDescription for MetadataHandle { } } +#[derive(Copy, Clone, Debug, PartialEq)] +enum CreationDisposition { + CreateAlways, + CreateNew, + OpenAlways, + OpenExisting, + TruncateExisting, +} + +impl CreationDisposition { + fn new<'tcx>( + value: u32, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, CreationDisposition> { + let create_always = ecx.eval_windows_u32("c", "CREATE_ALWAYS"); + let create_new = ecx.eval_windows_u32("c", "CREATE_NEW"); + let open_always = ecx.eval_windows_u32("c", "OPEN_ALWAYS"); + let open_existing = ecx.eval_windows_u32("c", "OPEN_EXISTING"); + let truncate_existing = ecx.eval_windows_u32("c", "TRUNCATE_EXISTING"); + + let out = if value == create_always { + CreationDisposition::CreateAlways + } else if value == create_new { + CreationDisposition::CreateNew + } else if value == open_always { + CreationDisposition::OpenAlways + } else if value == open_existing { + CreationDisposition::OpenExisting + } else if value == truncate_existing { + CreationDisposition::TruncateExisting + } else { + throw_unsup_format!("CreateFileW: Unsupported creation disposition: {value}"); + }; + interp_ok(out) + } +} + +bitflags! { + #[derive(PartialEq)] + struct FileAttributes: u32 { + const ZERO = 0; + const NORMAL = 1 << 0; + /// This must be passed to allow getting directory handles. If not passed, we error on trying + /// to open directories + const BACKUP_SEMANTICS = 1 << 1; + const OPEN_REPARSE = 1 << 2; + } +} + +impl FileAttributes { + fn new<'tcx>( + mut value: u32, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, Result> { + let file_attribute_normal = ecx.eval_windows_u32("c", "FILE_ATTRIBUTE_NORMAL"); + let file_flag_backup_semantics = ecx.eval_windows_u32("c", "FILE_FLAG_BACKUP_SEMANTICS"); + let file_flag_open_reparse_point = + ecx.eval_windows_u32("c", "FILE_FLAG_OPEN_REPARSE_POINT"); + + let mut out = FileAttributes::ZERO; + if value & file_flag_backup_semantics != 0 { + value &= !file_flag_backup_semantics; + out |= FileAttributes::BACKUP_SEMANTICS; + } else if value & file_flag_open_reparse_point != 0 { + value &= !file_flag_open_reparse_point; + out |= FileAttributes::OPEN_REPARSE; + } else if value & file_attribute_normal { + value &= !file_attribute_normal; + out |= FileAttributes::NORMAL; + } + + if value != 0 { + throw_unsup_format!("CreateFileW: Unsupported flags_and_attributes: {value}"); + } + + if out == FileAttributes::ZERO { + out = FileAttributes::NORMAL; + } + interp_ok(Ok(out)) + } +} + impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} #[allow(non_snake_case)] pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { @@ -67,6 +155,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { template_file: &OpTy<'tcx>, // HANDLE ) -> InterpResult<'tcx, Handle> { // ^ Returns HANDLE + use CreationDisposition::*; + let this = self.eval_context_mut(); this.assert_target_os("windows", "CreateFileW"); this.check_no_isolation("`CreateFileW`")?; @@ -86,18 +176,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let file_share_read = this.eval_windows_u32("c", "FILE_SHARE_READ"); let file_share_write = this.eval_windows_u32("c", "FILE_SHARE_WRITE"); - 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"); - - 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 creation_disposition = CreationDisposition::new(creation_disposition, this)?; + let attributes = FileAttributes::new(flags_and_attributes, this)?; if share_mode != (file_share_delete | file_share_read | file_share_write) { throw_unsup_format!("CreateFileW: Unsupported share mode: {share_mode}"); @@ -106,22 +186,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { throw_unsup_format!("CreateFileW: Security attributes are not supported"); } - 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 - { + if attributes & FileAttributes::OPEN_REPARSE != 0 && creation_disposition == CreateAlways { throw_machine_stop!(TerminationInfo::Abort("Invalid CreateFileW argument combination: FILE_FLAG_OPEN_REPARSE_POINT with CREATE_ALWAYS".to_string())); } @@ -131,7 +196,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let is_dir = file_name.is_dir(); - if flags_and_attributes == file_attribute_normal && is_dir { + if !attributes.contains(FileAttributes::BACKUP_SEMANTICS) && is_dir { this.set_last_error(IoError::WindowsError("ERROR_ACCESS_DENIED"))?; return interp_ok(Handle::Invalid); } @@ -155,63 +220,48 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ); } - // This is racy, but there doesn't appear to be an std API that both succeeds if a file - // exists but tells us it isn't new. Either we accept racing one way or another, or we - // use an iffy heuristic like file creation time. This implementation prefers to fail in the - // direction of erroring more often. - let exists = file_name.exists(); - - if creation_disposition == create_always { - // Per the documentation: - // If the specified file exists and is writable, the function truncates the file, the - // function succeeds, and last-error code is set to ERROR_ALREADY_EXISTS. - // If the specified file does not exist and is a valid path, a new file is created, the - // function succeeds, and the last-error code is set to zero. - // https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew - if exists { - this.set_last_error(IoError::WindowsError("ERROR_ALREADY_EXISTS"))?; - } else { - this.set_last_error(IoError::Raw(Scalar::from_u32(0)))?; + match creation_disposition { + CreateNew | OpenAlways => { + // This is racy, but there doesn't appear to be an std API that both succeeds if a + // file exists but tells us it isn't new. Either we accept racing one way or another, + // or we use an iffy heuristic like file creation time. This implementation prefers + // to fail in the direction of erroring more often. + // Per the documentation: + // If the specified file exists and is writable, the function truncates the file, + // the function succeeds, and last-error code is set to ERROR_ALREADY_EXISTS. + // If the specified file does not exist and is a valid path, a new file is created, + // the function succeeds, and the last-error code is set to zero. + // https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew + if file_name.exists() { + this.set_last_error(IoError::WindowsError("ERROR_ALREADY_EXISTS"))?; + } else { + this.set_last_error(IoError::Raw(Scalar::from_u32(0)))?; + } + options.create(true); + if creation_disposition == CreateNew { + options.truncate(true); + } } - options.create(true); - options.truncate(true); - } else if creation_disposition == create_new { - options.create_new(true); - // Per `create_new` documentation: - // If the specified file does not exist and is a valid path to a writable location, the - // function creates a file and the last-error code is set to zero. - // https://doc.rust-lang.org/std/fs/struct.OpenOptions.html#method.create_new - if !desired_write { - options.append(true); + CreateAlways => { + options.create_new(true); + // Per `create_new` documentation: + // The file must be opened with write or append access in order to create a new file. + // https://doc.rust-lang.org/std/fs/struct.OpenOptions.html#method.create_new + if !desired_write { + options.append(true); + } } - } else if creation_disposition == open_always { - // Per the documentation: - // If the specified file exists, the function succeeds and the last-error code is set - // to ERROR_ALREADY_EXISTS. - // If the specified file does not exist and is a valid path to a writable location, the - // function creates a file and the last-error code is set to zero. - // https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew - if exists { - this.set_last_error(IoError::WindowsError("ERROR_ALREADY_EXISTS"))?; - } else { - this.set_last_error(IoError::Raw(Scalar::from_u32(0)))?; + OpenExisting => (), // Nothing + TruncateExisting => { + options.truncate(true); } - options.create(true); - } else if creation_disposition == open_existing { - // Nothing - } else if creation_disposition == truncate_existing { - options.truncate(true); - } else { - throw_unsup_format!( - "CreateFileW: Unsupported creation disposition: {creation_disposition}" - ); } let handle = if is_dir { let fh = &mut this.machine.fds; let fd_num = fh.insert_new(DirHandle { path: file_name }); Ok(Handle::File(fd_num)) - } else if creation_disposition == open_existing && !(desired_read || desired_write) { + } else if creation_disposition == OpenExisting && !(desired_read || desired_write) { // Windows supports handles with no permissions. These allow things such as reading // metadata, but not file content. let fh = &mut this.machine.fds; From 92f41ce06355c764db1a7ecbc92971c648eca295 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Sun, 15 Dec 2024 19:28:14 -0800 Subject: [PATCH 22/22] Use FdNum more places --- src/shims/files.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shims/files.rs b/src/shims/files.rs index 28d8ed4208..f9c634201d 100644 --- a/src/shims/files.rs +++ b/src/shims/files.rs @@ -388,7 +388,7 @@ pub struct FdId(usize); /// The file descriptor table #[derive(Debug)] pub struct FdTable { - pub fds: BTreeMap, + pub fds: BTreeMap, /// Unique identifier for file description, used to differentiate between various file description. next_file_description_id: FdId, } @@ -423,7 +423,7 @@ impl FdTable { } /// Insert a new file description to the FdTable. - pub fn insert_new(&mut self, fd: impl FileDescription) -> i32 { + pub fn insert_new(&mut self, fd: impl FileDescription) -> FdNum { let fd_ref = self.new_ref(fd); self.insert(fd_ref) }