From 0601af0b872120fe69666e7c76305809fdd96a19 Mon Sep 17 00:00:00 2001 From: Simon Davies Date: Thu, 5 Mar 2026 21:58:00 +0000 Subject: [PATCH 1/8] feat: add SurrogateMapping enum for Windows map_file_cow support Introduce SurrogateMapping enum (SandboxMemory/ReadOnlyFile) to HostRegionBase on Windows. This allows the surrogate process pipeline to distinguish between standard sandbox memory (guard pages, PAGE_READWRITE) and file-backed read-only mappings (no guard pages, PAGE_READONLY). - Add SurrogateMapping enum to memory_region.rs - Add surrogate_mapping field to HostRegionBase - Update Hash, MemoryRegionKind::add impls to include new field - Update host_region_base() in shared_mem.rs to set SandboxMemory - Update crashdump test to include surrogate_mapping field - Add unit tests for SurrogateMapping enum behaviour - Fix Justfile fmt-apply/fmt-check/witguest-wit for Windows (PowerShell) Signed-off-by: Simon Davies --- Justfile | 14 +- src/hyperlight_host/src/mem/memory_region.rs | 177 +++++++++++++++++++ src/hyperlight_host/src/mem/shared_mem.rs | 1 + 3 files changed, 186 insertions(+), 6 deletions(-) diff --git a/Justfile b/Justfile index 9ae92795f..1a066e2da 100644 --- a/Justfile +++ b/Justfile @@ -43,10 +43,10 @@ build target=default-target: guests: build-and-move-rust-guests build-and-move-c-guests ensure-cargo-hyperlight: - command -v cargo-hyperlight >/dev/null 2>&1 || cargo install --locked cargo-hyperlight + {{ if os() == "windows" { "if (-not (Get-Command cargo-hyperlight -ErrorAction SilentlyContinue)) { cargo install --locked cargo-hyperlight }" } else { "command -v cargo-hyperlight >/dev/null 2>&1 || cargo install --locked cargo-hyperlight" } }} witguest-wit: - command -v wasm-tools >/dev/null 2>&1 || cargo install --locked wasm-tools + {{ if os() == "windows" { "if (-not (Get-Command wasm-tools -ErrorAction SilentlyContinue)) { cargo install --locked wasm-tools }" } else { "command -v wasm-tools >/dev/null 2>&1 || cargo install --locked wasm-tools" } }} cd src/tests/rust_guests/witguest && wasm-tools component wit guest.wit -w -o interface.wasm cd src/tests/rust_guests/witguest && wasm-tools component wit two_worlds.wit -w -o twoworlds.wasm @@ -287,19 +287,21 @@ check: {{ cargo-cmd }} check -p hyperlight-host --features nanvix-unstable {{ target-triple-flag }} {{ cargo-cmd }} check -p hyperlight-host --features nanvix-unstable,executable_heap {{ target-triple-flag }} -fmt-check: - rustup +nightly component list | grep -q "rustfmt.*installed" || rustup component add rustfmt --toolchain nightly +fmt-check: (ensure-nightly-fmt) cargo +nightly fmt --all -- --check cargo +nightly fmt --manifest-path src/tests/rust_guests/simpleguest/Cargo.toml -- --check cargo +nightly fmt --manifest-path src/tests/rust_guests/dummyguest/Cargo.toml -- --check cargo +nightly fmt --manifest-path src/tests/rust_guests/witguest/Cargo.toml -- --check cargo +nightly fmt --manifest-path src/hyperlight_guest_capi/Cargo.toml -- --check +[private] +ensure-nightly-fmt: + {{ if os() == "windows" { "if (-not (rustup +nightly component list | Select-String 'rustfmt.*installed')) { rustup component add rustfmt --toolchain nightly }" } else { "rustup +nightly component list | grep -q 'rustfmt.*installed' || rustup component add rustfmt --toolchain nightly" } }} + check-license-headers: ./dev/check-license-headers.sh -fmt-apply: - rustup +nightly component list | grep -q "rustfmt.*installed" || rustup component add rustfmt --toolchain nightly +fmt-apply: (ensure-nightly-fmt) cargo +nightly fmt --all cargo +nightly fmt --manifest-path src/tests/rust_guests/simpleguest/Cargo.toml cargo +nightly fmt --manifest-path src/tests/rust_guests/dummyguest/Cargo.toml diff --git a/src/hyperlight_host/src/mem/memory_region.rs b/src/hyperlight_host/src/mem/memory_region.rs index f93adb478..b29c30fbb 100644 --- a/src/hyperlight_host/src/mem/memory_region.rs +++ b/src/hyperlight_host/src/mem/memory_region.rs @@ -162,6 +162,22 @@ impl MemoryRegionKind for HostGuestMemoryRegion { base + size } } +/// Describes how a memory region should be mapped through the surrogate process +/// pipeline on Windows (WHP). +/// +/// Different mapping types require different page protections and guard page +/// behaviour when projected into the surrogate process via `MapViewOfFileNuma2`. +#[cfg(target_os = "windows")] +#[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)] +pub enum SurrogateMapping { + /// Standard sandbox shared memory: mapped with `PAGE_READWRITE` protection + /// and guard pages (`PAGE_NOACCESS`) set on the first and last pages. + SandboxMemory, + /// File-backed read-only mapping: mapped with `PAGE_READONLY` protection + /// and **no** guard pages. + ReadOnlyFile, +} + /// A [`HostRegionBase`] keeps track of not just a pointer, but also a /// file mapping into which it is pointing. This is used on WHP, /// where mapping the actual pointer into the VM actually involves @@ -178,6 +194,9 @@ pub struct HostRegionBase { /// The offset into file mapping region where this /// [`HostRegionBase`] is pointing. pub offset: usize, + /// How this region should be mapped through the surrogate process. + /// Controls page protection and guard page behaviour. + pub surrogate_mapping: SurrogateMapping, } #[cfg(target_os = "windows")] impl std::hash::Hash for HostRegionBase { @@ -189,6 +208,7 @@ impl std::hash::Hash for HostRegionBase { self.handle_base.hash(state); self.handle_size.hash(state); self.offset.hash(state); + self.surrogate_mapping.hash(state); } } #[cfg(target_os = "windows")] @@ -214,6 +234,7 @@ impl MemoryRegionKind for HostGuestMemoryRegion { handle_base: base.handle_base, handle_size: base.handle_size, offset: base.offset + size, + surrogate_mapping: base.surrogate_mapping, } } } @@ -413,3 +434,159 @@ impl From<&MemoryRegion> for kvm_bindings::kvm_userspace_memory_region { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn memory_region_flags_display() { + assert_eq!(format!("{}", MemoryRegionFlags::NONE), "NONE"); + assert_eq!(format!("{}", MemoryRegionFlags::READ), "READ"); + assert_eq!( + format!("{}", MemoryRegionFlags::READ | MemoryRegionFlags::WRITE), + "READ | WRITE" + ); + assert_eq!( + format!( + "{}", + MemoryRegionFlags::READ | MemoryRegionFlags::WRITE | MemoryRegionFlags::EXECUTE + ), + "READ | WRITE | EXECUTE" + ); + } + + #[cfg(target_os = "windows")] + mod windows_tests { + use std::collections::HashSet; + use std::hash::{DefaultHasher, Hash, Hasher}; + + use super::*; + + /// Helper to create a `HostRegionBase` with the given `SurrogateMapping`. + fn make_host_region_base(mapping: SurrogateMapping) -> HostRegionBase { + HostRegionBase { + from_handle: windows::Win32::Foundation::INVALID_HANDLE_VALUE.into(), + handle_base: 0x1000, + handle_size: 0x2000, + offset: 0x100, + surrogate_mapping: mapping, + } + } + + fn hash_of(val: &impl Hash) -> u64 { + let mut hasher = DefaultHasher::new(); + val.hash(&mut hasher); + hasher.finish() + } + + #[test] + fn surrogate_mapping_equality() { + assert_eq!( + SurrogateMapping::SandboxMemory, + SurrogateMapping::SandboxMemory + ); + assert_eq!( + SurrogateMapping::ReadOnlyFile, + SurrogateMapping::ReadOnlyFile + ); + assert_ne!( + SurrogateMapping::SandboxMemory, + SurrogateMapping::ReadOnlyFile + ); + } + + #[test] + fn surrogate_mapping_copy_clone_debug() { + let a = SurrogateMapping::ReadOnlyFile; + let b = a; // Copy + let c = a; // Also Copy (SurrogateMapping implements Copy) + assert_eq!(a, b); + assert_eq!(a, c); + // Debug should produce a non-empty string + assert!(!format!("{:?}", a).is_empty()); + } + + #[test] + fn host_region_base_different_surrogate_mapping_not_equal() { + let sandbox = make_host_region_base(SurrogateMapping::SandboxMemory); + let readonly = make_host_region_base(SurrogateMapping::ReadOnlyFile); + assert_ne!(sandbox, readonly); + } + + #[test] + fn host_region_base_different_surrogate_mapping_different_hash() { + let sandbox = make_host_region_base(SurrogateMapping::SandboxMemory); + let readonly = make_host_region_base(SurrogateMapping::ReadOnlyFile); + assert_ne!(hash_of(&sandbox), hash_of(&readonly)); + } + + #[test] + fn host_region_base_same_surrogate_mapping_equal_and_same_hash() { + let a = make_host_region_base(SurrogateMapping::SandboxMemory); + let b = make_host_region_base(SurrogateMapping::SandboxMemory); + assert_eq!(a, b); + assert_eq!(hash_of(&a), hash_of(&b)); + } + + #[test] + fn host_region_base_into_usize() { + let hrb = make_host_region_base(SurrogateMapping::SandboxMemory); + let addr: usize = hrb.into(); + assert_eq!(addr, 0x1000 + 0x100); // handle_base + offset + } + + #[test] + fn memory_region_kind_add_preserves_surrogate_mapping() { + let base = make_host_region_base(SurrogateMapping::ReadOnlyFile); + let result = ::add(base, 0x400); + + assert_eq!(result.from_handle, base.from_handle); + assert_eq!(result.handle_base, base.handle_base); + assert_eq!(result.handle_size, base.handle_size); + assert_eq!(result.offset, base.offset + 0x400); + assert_eq!(result.surrogate_mapping, SurrogateMapping::ReadOnlyFile); + } + + #[test] + fn host_region_base_works_in_hashset() { + let sandbox = make_host_region_base(SurrogateMapping::SandboxMemory); + let readonly = make_host_region_base(SurrogateMapping::ReadOnlyFile); + + let mut set = HashSet::new(); + set.insert(sandbox); + set.insert(readonly); + // Both should be present since they differ by surrogate_mapping + assert_eq!(set.len(), 2); + + // Inserting a duplicate should not increase the count + set.insert(make_host_region_base(SurrogateMapping::SandboxMemory)); + assert_eq!(set.len(), 2); + } + + #[test] + fn memory_region_with_surrogate_mapping_equality() { + let base_sandbox = make_host_region_base(SurrogateMapping::SandboxMemory); + let base_readonly = make_host_region_base(SurrogateMapping::ReadOnlyFile); + + let region_sandbox = MemoryRegion { + guest_region: 0x0..0x2000, + host_region: base_sandbox + ..::add(base_sandbox, 0x2000), + flags: MemoryRegionFlags::READ, + region_type: MemoryRegionType::Code, + }; + + let region_readonly = MemoryRegion { + guest_region: 0x0..0x2000, + host_region: base_readonly + ..::add(base_readonly, 0x2000), + flags: MemoryRegionFlags::READ, + region_type: MemoryRegionType::Code, + }; + + // Regions with different surrogate_mapping should not be equal + assert_ne!(region_sandbox, region_readonly); + } + } +} diff --git a/src/hyperlight_host/src/mem/shared_mem.rs b/src/hyperlight_host/src/mem/shared_mem.rs index 2cd3b8acc..a5306dcaf 100644 --- a/src/hyperlight_host/src/mem/shared_mem.rs +++ b/src/hyperlight_host/src/mem/shared_mem.rs @@ -759,6 +759,7 @@ pub trait SharedMemory { handle_base: self.region().ptr as usize, handle_size: self.region().size, offset: PAGE_SIZE_USIZE, + surrogate_mapping: super::memory_region::SurrogateMapping::SandboxMemory, } } From a2536422aee8838942b1d6f3d8bb25b81e01ce1e Mon Sep 17 00:00:00 2001 From: Simon Davies Date: Thu, 5 Mar 2026 22:12:56 +0000 Subject: [PATCH 2/8] feat: propagate SurrogateMapping through surrogate pipeline Pass SurrogateMapping through SurrogateProcess::map and WhpVm::map_memory so that file-backed read-only mappings (ReadOnlyFile) use PAGE_READONLY and skip guard pages, while sandbox memory (SandboxMemory) retains the existing PAGE_READWRITE + guard page behaviour. - Add mapping parameter to SurrogateProcess::map - Derive page protection and guard page logic from SurrogateMapping variant - Update WhpVm::map_memory to extract and forward surrogate_mapping - Update existing test call site in surrogate_process_manager - Add test: readonly_file_mapping_skips_guard_pages - Add test: surrogate_map_ref_counting Signed-off-by: Simon Davies Signed-off-by: Simon Davies --- .../src/hypervisor/surrogate_process.rs | 85 ++++++++---- .../hypervisor/surrogate_process_manager.rs | 124 ++++++++++++++++++ .../src/hypervisor/virtual_machine/whp.rs | 1 + 3 files changed, 181 insertions(+), 29 deletions(-) diff --git a/src/hyperlight_host/src/hypervisor/surrogate_process.rs b/src/hyperlight_host/src/hypervisor/surrogate_process.rs index f054dc996..59c650963 100644 --- a/src/hyperlight_host/src/hypervisor/surrogate_process.rs +++ b/src/hyperlight_host/src/hypervisor/surrogate_process.rs @@ -23,13 +23,14 @@ use tracing::{Span, instrument}; use windows::Win32::Foundation::HANDLE; use windows::Win32::System::Memory::{ MEMORY_MAPPED_VIEW_ADDRESS, MapViewOfFileNuma2, PAGE_NOACCESS, PAGE_PROTECTION_FLAGS, - PAGE_READWRITE, UNMAP_VIEW_OF_FILE_FLAGS, UnmapViewOfFile2, VirtualProtectEx, + PAGE_READONLY, PAGE_READWRITE, UNMAP_VIEW_OF_FILE_FLAGS, UnmapViewOfFile2, VirtualProtectEx, }; use windows::Win32::System::SystemServices::NUMA_NO_PREFERRED_NODE; use super::surrogate_process_manager::get_surrogate_process_manager; use super::wrappers::HandleWrapper; use crate::HyperlightError::WindowsAPIError; +use crate::mem::memory_region::SurrogateMapping; use crate::{Result, log_then_return}; #[derive(Debug)] @@ -57,11 +58,24 @@ impl SurrogateProcess { } } + /// Maps a file mapping handle into the surrogate process. + /// + /// The `mapping` parameter controls the page protection and guard page + /// behaviour: + /// - [`SurrogateMapping::SandboxMemory`]: uses `PAGE_READWRITE` and sets + /// guard pages (`PAGE_NOACCESS`) on the first and last pages. + /// - [`SurrogateMapping::ReadOnlyFile`]: uses `PAGE_READONLY` with no + /// guard pages. + /// + /// If `host_base` was already mapped, the existing mapping is reused + /// and the reference count is incremented (the `mapping` parameter is + /// ignored in that case). pub(super) fn map( &mut self, handle: HandleWrapper, host_base: usize, host_size: usize, + mapping: &SurrogateMapping, ) -> Result<*mut c_void> { match self.mappings.entry(host_base) { Entry::Occupied(mut oe) => { @@ -69,6 +83,12 @@ impl SurrogateProcess { Ok(oe.get().surrogate_base) } Entry::Vacant(ve) => { + // Derive the page protection from the mapping type + let page_protection = match mapping { + SurrogateMapping::SandboxMemory => PAGE_READWRITE, + SurrogateMapping::ReadOnlyFile => PAGE_READONLY, + }; + // Use MapViewOfFile2 to map memory into the surrogate process, the MapViewOfFile2 API is implemented in as an inline function in a windows header file // (see https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-mapviewoffile2#remarks) so we use the same API it uses in the header file here instead of // MapViewOfFile2 which does not exist in the rust crate (see https://github.com/microsoft/windows-rs/issues/2595) @@ -80,40 +100,47 @@ impl SurrogateProcess { None, host_size, 0, - PAGE_READWRITE.0, + page_protection.0, NUMA_NO_PREFERRED_NODE, ) }; - let mut unused_out_old_prot_flags = PAGE_PROTECTION_FLAGS(0); - // the first page of the raw_size is the guard page - let first_guard_page_start = surrogate_base.Value; - if let Err(e) = unsafe { - VirtualProtectEx( - self.process_handle.into(), - first_guard_page_start, - PAGE_SIZE_USIZE, - PAGE_NOACCESS, - &mut unused_out_old_prot_flags, - ) - } { - log_then_return!(WindowsAPIError(e.clone())); + // Only set guard pages for SandboxMemory mappings. + // File-backed read-only mappings do not need guard pages + // because the host does not write to them. + if *mapping == SurrogateMapping::SandboxMemory { + let mut unused_out_old_prot_flags = PAGE_PROTECTION_FLAGS(0); + + // the first page of the raw_size is the guard page + let first_guard_page_start = surrogate_base.Value; + if let Err(e) = unsafe { + VirtualProtectEx( + self.process_handle.into(), + first_guard_page_start, + PAGE_SIZE_USIZE, + PAGE_NOACCESS, + &mut unused_out_old_prot_flags, + ) + } { + log_then_return!(WindowsAPIError(e.clone())); + } + + // the last page of the raw_size is the guard page + let last_guard_page_start = + unsafe { first_guard_page_start.add(host_size - PAGE_SIZE_USIZE) }; + if let Err(e) = unsafe { + VirtualProtectEx( + self.process_handle.into(), + last_guard_page_start, + PAGE_SIZE_USIZE, + PAGE_NOACCESS, + &mut unused_out_old_prot_flags, + ) + } { + log_then_return!(WindowsAPIError(e.clone())); + } } - // the last page of the raw_size is the guard page - let last_guard_page_start = - unsafe { first_guard_page_start.add(host_size - PAGE_SIZE_USIZE) }; - if let Err(e) = unsafe { - VirtualProtectEx( - self.process_handle.into(), - last_guard_page_start, - PAGE_SIZE_USIZE, - PAGE_NOACCESS, - &mut unused_out_old_prot_flags, - ) - } { - log_then_return!(WindowsAPIError(e.clone())); - } ve.insert(HandleMapping { use_count: 1, surrogate_base: surrogate_base.Value, diff --git a/src/hyperlight_host/src/hypervisor/surrogate_process_manager.rs b/src/hyperlight_host/src/hypervisor/surrogate_process_manager.rs index 966ed50f4..08c60e370 100644 --- a/src/hyperlight_host/src/hypervisor/surrogate_process_manager.rs +++ b/src/hyperlight_host/src/hypervisor/surrogate_process_manager.rs @@ -459,6 +459,7 @@ mod tests { HandleWrapper::from(mem.get_mmap_file_handle()), mem.raw_ptr() as usize, mem.raw_mem_size(), + &crate::mem::memory_region::SurrogateMapping::SandboxMemory, ) .unwrap(); @@ -498,4 +499,127 @@ mod tests { assert!(success.is_err()); } } + + /// Tests that [`SurrogateMapping::ReadOnlyFile`] skips guard pages entirely. + /// + /// When mapping with `ReadOnlyFile`, the first and last pages should be + /// accessible (no `PAGE_NOACCESS` guard pages set), unlike `SandboxMemory` + /// which marks them as guard pages. + #[test] + fn readonly_file_mapping_skips_guard_pages() { + const SIZE: usize = 4096; + let mgr = get_surrogate_process_manager().unwrap(); + let mem = ExclusiveSharedMemory::new(SIZE).unwrap(); + + let mut process = mgr.get_surrogate_process().unwrap(); + let surrogate_address = process + .map( + HandleWrapper::from(mem.get_mmap_file_handle()), + mem.raw_ptr() as usize, + mem.raw_mem_size(), + &crate::mem::memory_region::SurrogateMapping::ReadOnlyFile, + ) + .unwrap(); + + let buffer = vec![0u8; SIZE]; + let bytes_read: Option<*mut usize> = None; + let process_handle: HANDLE = process.process_handle.into(); + + unsafe { + // read the first page — should succeed (no guard page for ReadOnlyFile) + let success = windows::Win32::System::Diagnostics::Debug::ReadProcessMemory( + process_handle, + surrogate_address, + buffer.as_ptr() as *mut c_void, + SIZE, + bytes_read, + ); + assert!( + success.is_ok(), + "First page should be readable with ReadOnlyFile (no guard page)" + ); + + // read the middle page — should also succeed + let success = windows::Win32::System::Diagnostics::Debug::ReadProcessMemory( + process_handle, + surrogate_address.wrapping_add(SIZE), + buffer.as_ptr() as *mut c_void, + SIZE, + bytes_read, + ); + assert!( + success.is_ok(), + "Middle page should be readable with ReadOnlyFile" + ); + + // read the last page — should succeed (no guard page for ReadOnlyFile) + let success = windows::Win32::System::Diagnostics::Debug::ReadProcessMemory( + process_handle, + surrogate_address.wrapping_add(2 * SIZE), + buffer.as_ptr() as *mut c_void, + SIZE, + bytes_read, + ); + assert!( + success.is_ok(), + "Last page should be readable with ReadOnlyFile (no guard page)" + ); + } + } + + /// Tests that the reference counting in [`SurrogateProcess::map`] works + /// correctly — repeated maps to the same `host_base` increment the count + /// and return the same surrogate address, regardless of the mapping type + /// passed on subsequent calls. + #[test] + fn surrogate_map_ref_counting() { + let mgr = get_surrogate_process_manager().unwrap(); + let mem = ExclusiveSharedMemory::new(4096).unwrap(); + + let mut process = mgr.get_surrogate_process().unwrap(); + let handle = HandleWrapper::from(mem.get_mmap_file_handle()); + let host_base = mem.raw_ptr() as usize; + let host_size = mem.raw_mem_size(); + + // First map — creates the mapping + let addr1 = process + .map( + handle, + host_base, + host_size, + &crate::mem::memory_region::SurrogateMapping::SandboxMemory, + ) + .unwrap(); + + // Second map — should reuse (ref count incremented) + let addr2 = process + .map( + handle, + host_base, + host_size, + &crate::mem::memory_region::SurrogateMapping::SandboxMemory, + ) + .unwrap(); + + assert_eq!( + addr1, addr2, + "Repeated map should return the same surrogate address" + ); + + // First unmap — decrements ref count but should NOT actually unmap + process.unmap(host_base); + + // The mapping should still be present (ref count was 2, now 1) + assert!( + process.mappings.contains_key(&host_base), + "Mapping should still exist after first unmap (ref count > 0)" + ); + + // Second unmap — ref count hits 0, actually unmaps + process.unmap(host_base); + assert!( + !process.mappings.contains_key(&host_base), + "Mapping should be removed after ref count reaches 0" + ); + } } diff --git a/src/hyperlight_host/src/hypervisor/virtual_machine/whp.rs b/src/hyperlight_host/src/hypervisor/virtual_machine/whp.rs index be81f7f72..de6fa7497 100644 --- a/src/hyperlight_host/src/hypervisor/virtual_machine/whp.rs +++ b/src/hyperlight_host/src/hypervisor/virtual_machine/whp.rs @@ -144,6 +144,7 @@ impl VirtualMachine for WhpVm { region.host_region.start.from_handle, region.host_region.start.handle_base, region.host_region.start.handle_size, + ®ion.host_region.start.surrogate_mapping, ) .map_err(|e| MapMemoryError::SurrogateProcess(e.to_string()))?; let surrogate_addr = surrogate_base.wrapping_add(region.host_region.start.offset); From 9de630074e6d5e583f258638896fb5a33011b333 Mon Sep 17 00:00:00 2001 From: Simon Davies Date: Thu, 5 Mar 2026 22:45:44 +0000 Subject: [PATCH 3/8] feat: add OwnedFileMapping RAII struct for Windows handle cleanup Introduce OwnedFileMapping to track host-side file mapping resources (MapViewOfFile view + CreateFileMappingW handle) with RAII cleanup. Add file_mappings Vec to MultiUseSandbox (Windows-only, positioned after vm field for correct drop order). - OwnedFileMapping::Drop calls UnmapViewOfFile then CloseHandle - unsafe impl Send + Sync (raw pointer only used during Drop) - Add test: owned_file_mapping_drop_releases_handles Signed-off-by: Simon Davies Signed-off-by: Simon Davies --- .../src/sandbox/initialized_multi_use.rs | 132 ++++++++++++++++++ 1 file changed, 132 insertions(+) diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index 82da52912..d7ae38bf8 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -30,6 +30,10 @@ use hyperlight_common::flatbuffer_wrappers::function_types::{ }; use hyperlight_common::flatbuffer_wrappers::util::estimate_flatbuffer_capacity; use tracing::{Span, instrument}; +#[cfg(target_os = "windows")] +use windows::Win32::Foundation::CloseHandle; +#[cfg(target_os = "windows")] +use windows::Win32::System::Memory::{MEMORY_MAPPED_VIEW_ADDRESS, UnmapViewOfFile}; use super::Callable; use super::host_funcs::FunctionRegistry; @@ -38,6 +42,8 @@ use crate::HyperlightError::{self, SnapshotSandboxMismatch}; use crate::func::{ParameterTuple, SupportedReturnType}; use crate::hypervisor::InterruptHandle; use crate::hypervisor::hyperlight_vm::{HyperlightVm, HyperlightVmError}; +#[cfg(target_os = "windows")] +use crate::hypervisor::wrappers::HandleWrapper; use crate::mem::memory_region::MemoryRegion; #[cfg(unix)] use crate::mem::memory_region::{MemoryRegionFlags, MemoryRegionType}; @@ -48,6 +54,53 @@ use crate::metrics::{ }; use crate::{Result, log_then_return}; +/// Tracks a host-side file mapping created by [`MultiUseSandbox::map_file_cow`] +/// on Windows, providing RAII cleanup of the underlying Windows handles. +/// +/// When dropped, releases both the `MapViewOfFile` view and the +/// `CreateFileMappingW` handle, preventing resource leaks. +#[cfg(target_os = "windows")] +#[allow(dead_code)] // Used in Step 4 (map_file_cow) and Step 5 (restore cleanup) +struct OwnedFileMapping { + /// Guest base address used to identify which region this mapping + /// belongs to (for cleanup during [`MultiUseSandbox::restore`]). + guest_base: usize, + /// Host-side base pointer returned by `MapViewOfFile`. + view_base: *mut core::ffi::c_void, + /// File mapping handle returned by `CreateFileMappingW`. + mapping_handle: HandleWrapper, +} + +#[cfg(target_os = "windows")] +impl Drop for OwnedFileMapping { + fn drop(&mut self) { + // Unmap the view first, then close the file mapping handle. + // Order matters: the view holds a reference to the mapping. + unsafe { + if let Err(e) = UnmapViewOfFile(MEMORY_MAPPED_VIEW_ADDRESS { + Value: self.view_base, + }) { + tracing::error!("Failed to unmap file view at {:?}: {:?}", self.view_base, e); + } + if let Err(e) = CloseHandle(self.mapping_handle.into()) { + tracing::error!( + "Failed to close file mapping handle {:?}: {:?}", + self.mapping_handle, + e + ); + } + } + } +} + +// SAFETY: The raw pointer `view_base` is only accessed during `Drop` +// (to call `UnmapViewOfFile`), which happens on the owning thread. +// The `HandleWrapper` is already `Send + Sync`. +#[cfg(target_os = "windows")] +unsafe impl Send for OwnedFileMapping {} +#[cfg(target_os = "windows")] +unsafe impl Sync for OwnedFileMapping {} + /// A fully initialized sandbox that can execute guest functions multiple times. /// /// Guest functions can be called repeatedly while maintaining state between calls. @@ -99,6 +152,12 @@ pub struct MultiUseSandbox { /// If the current state of the sandbox has been captured in a snapshot, /// that snapshot is stored here. snapshot: Option>, + /// Tracks host-side file mappings created by [`Self::map_file_cow`] for + /// RAII cleanup. Declared **after** `vm` so that Rust's field drop order + /// releases hypervisor mappings before closing host-side file handles. + #[cfg(target_os = "windows")] + #[allow(dead_code)] // Used in Step 4 (map_file_cow) and Step 5 (restore cleanup) + file_mappings: Vec, } impl MultiUseSandbox { @@ -123,6 +182,8 @@ impl MultiUseSandbox { #[cfg(gdb)] dbg_mem_access_fn, snapshot: None, + #[cfg(target_os = "windows")] + file_mappings: Vec::new(), } } @@ -1539,4 +1600,75 @@ mod tests { let start = code_gva + 4096 - 100; assert_gva_read_matches(&mut sbox, start, 200); } + + /// Tests that [`OwnedFileMapping`] properly releases Windows handles on drop. + /// + /// Creates a temp file, opens a `CreateFileMappingW` + `MapViewOfFile`, + /// wraps in `OwnedFileMapping`, drops it, and verifies the file can be + /// deleted (no leaked handles holding it open). + #[test] + #[cfg(target_os = "windows")] + fn owned_file_mapping_drop_releases_handles() { + use std::io::Write; + use std::os::windows::io::AsRawHandle; + + use windows::Win32::Foundation::HANDLE; + use windows::Win32::System::Memory::{ + CreateFileMappingW, FILE_MAP_READ, MapViewOfFile, PAGE_READONLY, + }; + + use super::OwnedFileMapping; + use crate::hypervisor::wrappers::HandleWrapper; + + // Create a temp file with some content + let temp_dir = std::env::temp_dir(); + let temp_path = temp_dir.join("hyperlight_test_owned_file_mapping.bin"); + // Clean up from any previous failed run + let _ = std::fs::remove_file(&temp_path); + + let mapping_handle; + let view_base; + + { + // Create and write to the file, then keep it open for mapping + let file = std::fs::OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(true) + .open(&temp_path) + .unwrap(); + file.set_len(4096).unwrap(); + (&file).write_all(&[0xAA; 4096]).unwrap(); + + let file_handle = HANDLE(file.as_raw_handle()); + + mapping_handle = unsafe { + CreateFileMappingW(file_handle, None, PAGE_READONLY, 0, 0, None) + .expect("CreateFileMappingW failed") + }; + + // File is dropped here (fd closed), but the mapping keeps the file open + } + + { + let view = unsafe { MapViewOfFile(mapping_handle, FILE_MAP_READ, 0, 0, 0) }; + assert!( + !view.Value.is_null(), + "MapViewOfFile should return a non-null pointer" + ); + view_base = view.Value; + + let _owned = OwnedFileMapping { + guest_base: 0x1000, + view_base, + mapping_handle: HandleWrapper::from(mapping_handle), + }; + // _owned is dropped here — should call UnmapViewOfFile + CloseHandle + } + + // After drop, the file should no longer be held open by our handles + std::fs::remove_file(&temp_path) + .expect("File should be deletable after OwnedFileMapping is dropped"); + } } From 290d8a20cdac9d95a6f5a27968ee6c696151417a Mon Sep 17 00:00:00 2001 From: Simon Davies Date: Fri, 6 Mar 2026 00:15:21 +0000 Subject: [PATCH 4/8] feat: implement map_file_cow on Windows Replace the Windows stub with a full implementation that maps a file into the guest address space via the surrogate process pipeline: CreateFileMappingW -> MapViewOfFile -> SurrogateProcess::map -> WHvMapGpaRange2. Key details: - Opens file read-only, creates PAGE_READONLY mapping - Uses SurrogateMapping::ReadOnlyFile (no guard pages, PAGE_READONLY) - Guest gets READ|EXECUTE access via WHvMapGpaRange2 flags - Tracks host resources in OwnedFileMapping for RAII cleanup - Error path cleans up MapViewOfFile + CloseHandle on failure - CreateFileMappingW uses 0,0 for size (file's actual size, Windows rounds to page boundaries internally) Tests (264 passed, 0 failed): - test_map_file_cow_basic: map file, read from guest, verify content - test_map_file_cow_read_only_enforcement: write triggers violation - test_map_file_cow_poisoned: PoisonedSandbox check + restore - test_map_file_cow_multi_vm_same_file: two sandboxes, same file - test_map_file_cow_multi_vm_threaded: 5 threads, concurrent mapping - test_map_file_cow_cleanup_no_handle_leak: file deletable after drop Signed-off-by: Simon Davies Signed-off-by: Simon Davies --- .../src/sandbox/initialized_multi_use.rs | 356 +++++++++++++++++- 1 file changed, 349 insertions(+), 7 deletions(-) diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index d7ae38bf8..cd234d3fb 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -33,7 +33,10 @@ use tracing::{Span, instrument}; #[cfg(target_os = "windows")] use windows::Win32::Foundation::CloseHandle; #[cfg(target_os = "windows")] -use windows::Win32::System::Memory::{MEMORY_MAPPED_VIEW_ADDRESS, UnmapViewOfFile}; +use windows::Win32::System::Memory::{ + CreateFileMappingW, FILE_MAP_READ, MEMORY_MAPPED_VIEW_ADDRESS, MapViewOfFile, PAGE_READONLY, + UnmapViewOfFile, +}; use super::Callable; use super::host_funcs::FunctionRegistry; @@ -44,9 +47,9 @@ use crate::hypervisor::InterruptHandle; use crate::hypervisor::hyperlight_vm::{HyperlightVm, HyperlightVmError}; #[cfg(target_os = "windows")] use crate::hypervisor::wrappers::HandleWrapper; -use crate::mem::memory_region::MemoryRegion; -#[cfg(unix)] -use crate::mem::memory_region::{MemoryRegionFlags, MemoryRegionType}; +#[cfg(target_os = "windows")] +use crate::mem::memory_region::{HostRegionBase, MemoryRegionKind, SurrogateMapping}; +use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags, MemoryRegionType}; use crate::mem::mgr::SandboxMemoryManager; use crate::mem::shared_mem::HostSharedMemory; use crate::metrics::{ @@ -60,7 +63,7 @@ use crate::{Result, log_then_return}; /// When dropped, releases both the `MapViewOfFile` view and the /// `CreateFileMappingW` handle, preventing resource leaks. #[cfg(target_os = "windows")] -#[allow(dead_code)] // Used in Step 4 (map_file_cow) and Step 5 (restore cleanup) +#[allow(dead_code)] // guest_base used in Step 5 (restore cleanup) struct OwnedFileMapping { /// Guest base address used to identify which region this mapping /// belongs to (for cleanup during [`MultiUseSandbox::restore`]). @@ -156,7 +159,6 @@ pub struct MultiUseSandbox { /// RAII cleanup. Declared **after** `vm` so that Rust's field drop order /// releases hypervisor mappings before closing host-side file handles. #[cfg(target_os = "windows")] - #[allow(dead_code)] // Used in Step 4 (map_file_cow) and Step 5 (restore cleanup) file_mappings: Vec, } @@ -619,7 +621,88 @@ impl MultiUseSandbox { return Err(crate::HyperlightError::PoisonedSandbox); } #[cfg(windows)] - log_then_return!("mmap'ing a file into the guest is not yet supported on Windows"); + { + use std::os::windows::io::AsRawHandle; + + use windows::Win32::Foundation::HANDLE; + + let file = std::fs::File::options().read(true).open(_fp)?; + let file_size = file.metadata()?.len(); + let page_size = page_size::get(); + let size = (file_size as usize).div_ceil(page_size) * page_size; + + let file_handle = HANDLE(file.as_raw_handle()); + + // Create a read-only file mapping object backed by the actual file. + // Pass 0,0 for size to use the file's actual size — Windows will + // NOT extend a read-only file, so requesting page-aligned size + // would fail for files smaller than one page. MapViewOfFile and + // the surrogate process will round up to page boundaries internally. + let mapping_handle = + unsafe { CreateFileMappingW(file_handle, None, PAGE_READONLY, 0, 0, None) } + .map_err(|e| { + HyperlightError::Error(format!("CreateFileMappingW failed: {e}")) + })?; + + // Map a read-only view into the host process. + // Passing 0 for dwNumberOfBytesToMap maps the entire file; the OS + // rounds up to the next page boundary and zero-fills the remainder. + let view = unsafe { MapViewOfFile(mapping_handle, FILE_MAP_READ, 0, 0, 0) }; + if view.Value.is_null() { + // Clean up the mapping handle before returning + unsafe { + let _ = CloseHandle(mapping_handle); + } + log_then_return!( + "MapViewOfFile failed: {:?}", + std::io::Error::last_os_error() + ); + } + + let host_base = HostRegionBase { + from_handle: HandleWrapper::from(mapping_handle), + handle_base: view.Value as usize, + handle_size: size, + offset: 0, + surrogate_mapping: SurrogateMapping::ReadOnlyFile, + }; + + let host_end = + ::add( + host_base, size, + ); + + let region = MemoryRegion { + host_region: host_base..host_end, + guest_region: _guest_base as usize.._guest_base as usize + size, + flags: MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE, + region_type: MemoryRegionType::Heap, + }; + + // Reset snapshot since we are mutating the sandbox state + self.snapshot = None; + + if let Err(err) = unsafe { self.vm.map_region(®ion) } + .map_err(HyperlightVmError::MapRegion) + .map_err(HyperlightError::HyperlightVmError) + { + // Clean up host-side resources on failure + unsafe { + let _ = UnmapViewOfFile(view); + let _ = CloseHandle(mapping_handle); + } + return Err(err); + } + + self.mem_mgr.mapped_rgns += 1; + self.file_mappings.push(OwnedFileMapping { + guest_base: _guest_base as usize, + view_base: view.Value, + mapping_handle: HandleWrapper::from(mapping_handle), + }); + + Ok(size as u64) + } #[cfg(unix)] unsafe { let file = std::fs::File::options().read(true).write(true).open(_fp)?; @@ -1671,4 +1754,263 @@ mod tests { std::fs::remove_file(&temp_path) .expect("File should be deletable after OwnedFileMapping is dropped"); } + + /// Helper: create a temp file with known content, padded to be + /// at least page-aligned (4096 bytes). Returns the path and the + /// *original* content bytes (before padding). + fn create_test_file(name: &str, content: &[u8]) -> (std::path::PathBuf, Vec) { + use std::io::Write; + + let page_size = 4096usize; + let padded_len = content.len().max(page_size).div_ceil(page_size) * page_size; + let mut padded = vec![0u8; padded_len]; + padded[..content.len()].copy_from_slice(content); + + let temp_dir = std::env::temp_dir(); + let path = temp_dir.join(name); + let _ = std::fs::remove_file(&path); // clean up from previous runs + let mut f = std::fs::File::create(&path).unwrap(); + f.write_all(&padded).unwrap(); + (path, content.to_vec()) + } + + /// Tests the basic `map_file_cow` flow: map a file, read its content + /// from the guest, and verify it matches. + #[test] + fn test_map_file_cow_basic() { + let expected = b"hello world from map_file_cow"; + let (path, expected_bytes) = + create_test_file("hyperlight_test_map_file_cow_basic.bin", expected); + + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + let guest_base: u64 = 0x1_0000_0000; + let mapped_size = sbox.map_file_cow(&path, guest_base).unwrap(); + assert!(mapped_size > 0, "mapped_size should be positive"); + assert!( + mapped_size >= expected.len() as u64, + "mapped_size should be >= file content length" + ); + + // Read the content back from the guest + let actual: Vec = sbox + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, true), + ) + .unwrap(); + + assert_eq!( + actual, expected_bytes, + "Guest should read back the exact file content" + ); + + // Clean up + let _ = std::fs::remove_file(&path); + } + + /// Tests that `map_file_cow` enforces read-only access: writing to + /// the mapped region from the guest should cause a MemoryAccessViolation. + #[test] + fn test_map_file_cow_read_only_enforcement() { + let content = &[0xBB; 4096]; + let (path, _) = create_test_file("hyperlight_test_map_file_cow_readonly.bin", content); + + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + let guest_base: u64 = 0x1_0000_0000; + sbox.map_file_cow(&path, guest_base).unwrap(); + + // Writing to the mapped region should fail with MemoryAccessViolation + let err = sbox + .call::("WriteMappedBuffer", (guest_base, content.len() as u64)) + .unwrap_err(); + + match err { + HyperlightError::MemoryAccessViolation(addr, ..) if addr == guest_base => {} + _ => panic!( + "Expected MemoryAccessViolation at guest_base, got: {:?}", + err + ), + }; + + // Clean up + let _ = std::fs::remove_file(&path); + } + + /// Tests that `map_file_cow` returns `PoisonedSandbox` when the + /// sandbox is poisoned. + #[test] + fn test_map_file_cow_poisoned() { + let (path, _) = create_test_file("hyperlight_test_map_file_cow_poison.bin", &[0xCC; 4096]); + + let mut sbox: MultiUseSandbox = { + let path = simple_guest_as_string().unwrap(); + let u_sbox = UninitializedSandbox::new(GuestBinary::FilePath(path), None).unwrap(); + u_sbox.evolve() + } + .unwrap(); + let snapshot = sbox.snapshot().unwrap(); + + // Poison the sandbox + let _ = sbox + .call::<()>("guest_panic", "hello".to_string()) + .unwrap_err(); + assert!(sbox.poisoned()); + + // map_file_cow should fail with PoisonedSandbox + let err = sbox.map_file_cow(&path, 0x1_0000_0000).unwrap_err(); + assert!(matches!(err, HyperlightError::PoisonedSandbox)); + + // Restore and verify map_file_cow works again + sbox.restore(snapshot).unwrap(); + assert!(!sbox.poisoned()); + let result = sbox.map_file_cow(&path, 0x1_0000_0000); + assert!(result.is_ok()); + + let _ = std::fs::remove_file(&path); + } + + /// Tests that two separate sandboxes can map the same file + /// simultaneously and both read it correctly. + #[test] + fn test_map_file_cow_multi_vm_same_file() { + let expected = b"shared file content across VMs"; + let (path, expected_bytes) = + create_test_file("hyperlight_test_map_file_cow_multi_vm.bin", expected); + + let guest_base: u64 = 0x1_0000_0000; + + let mut sbox1 = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + let mut sbox2 = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + // Map the same file into both sandboxes + sbox1.map_file_cow(&path, guest_base).unwrap(); + sbox2.map_file_cow(&path, guest_base).unwrap(); + + // Both should read the correct content + let actual1: Vec = sbox1 + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, true), + ) + .unwrap(); + let actual2: Vec = sbox2 + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, true), + ) + .unwrap(); + + assert_eq!( + actual1, expected_bytes, + "Sandbox 1 should read correct content" + ); + assert_eq!( + actual2, expected_bytes, + "Sandbox 2 should read correct content" + ); + + let _ = std::fs::remove_file(&path); + } + + /// Tests that multiple threads can each create a sandbox, map the + /// same file, read it, and drop without errors. + #[test] + fn test_map_file_cow_multi_vm_threaded() { + let expected = b"threaded file mapping test data"; + let (path, expected_bytes) = + create_test_file("hyperlight_test_map_file_cow_threaded.bin", expected); + + const NUM_THREADS: usize = 5; + let path = Arc::new(path); + let expected_bytes = Arc::new(expected_bytes); + let barrier = Arc::new(Barrier::new(NUM_THREADS)); + let mut handles = vec![]; + + for _ in 0..NUM_THREADS { + let path = path.clone(); + let expected_bytes = expected_bytes.clone(); + let barrier = barrier.clone(); + + handles.push(thread::spawn(move || { + barrier.wait(); + + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + let guest_base: u64 = 0x1_0000_0000; + sbox.map_file_cow(&path, guest_base).unwrap(); + + let actual: Vec = sbox + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, true), + ) + .unwrap(); + + assert_eq!(actual, *expected_bytes); + })); + } + + for h in handles { + h.join().unwrap(); + } + + let _ = std::fs::remove_file(&*path); + } + + /// Tests that file cleanup works after dropping a sandbox that used + /// `map_file_cow` — the file should be deletable (no leaked handles). + #[test] + #[cfg(target_os = "windows")] + fn test_map_file_cow_cleanup_no_handle_leak() { + let (path, _) = create_test_file("hyperlight_test_map_file_cow_cleanup.bin", &[0xDD; 4096]); + + { + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + sbox.map_file_cow(&path, 0x1_0000_0000).unwrap(); + // sandbox dropped here + } + + std::fs::remove_file(&path) + .expect("File should be deletable after sandbox with map_file_cow is dropped"); + } } From ebe9ee0c8092f72c7640d334b9228bdf04df4364 Mon Sep 17 00:00:00 2001 From: Simon Davies Date: Fri, 6 Mar 2026 00:26:24 +0000 Subject: [PATCH 5/8] feat: wire map_file_cow cleanup into restore() Clean up host-side file mapping resources (OwnedFileMapping) when restore() unmaps regions. For each unmapped region, remove the corresponding OwnedFileMapping entry, whose Drop impl calls UnmapViewOfFile + CloseHandle. Also remove the dead_code allow now that guest_base is read. Tests (266 passed, 0 failed): - test_map_file_cow_snapshot_restore: map, snapshot, restore, verify data readable from snapshot memory - test_map_file_cow_snapshot_remapping_cycle: snapshot1 (empty) -> map file -> snapshot2 -> restore1 (unmapped) -> restore2 (folded) Signed-off-by: Simon Davies Signed-off-by: Simon Davies --- .../src/sandbox/initialized_multi_use.rs | 124 +++++++++++++++++- 1 file changed, 123 insertions(+), 1 deletion(-) diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index cd234d3fb..bacc3a250 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -63,7 +63,6 @@ use crate::{Result, log_then_return}; /// When dropped, releases both the `MapViewOfFile` view and the /// `CreateFileMappingW` handle, preventing resource leaks. #[cfg(target_os = "windows")] -#[allow(dead_code)] // guest_base used in Step 5 (restore cleanup) struct OwnedFileMapping { /// Guest base address used to identify which region this mapping /// belongs to (for cleanup during [`MultiUseSandbox::restore`]). @@ -392,6 +391,13 @@ impl MultiUseSandbox { self.vm .unmap_region(region) .map_err(HyperlightVmError::UnmapRegion)?; + + // Clean up host-side file mapping resources for regions that + // were created by map_file_cow. The OwnedFileMapping::Drop + // will call UnmapViewOfFile + CloseHandle. + #[cfg(target_os = "windows")] + self.file_mappings + .retain(|fm| fm.guest_base != region.guest_region.start); } for region in regions_to_map { @@ -2013,4 +2019,120 @@ mod tests { std::fs::remove_file(&path) .expect("File should be deletable after sandbox with map_file_cow is dropped"); } + + /// Tests snapshot/restore cycle with map_file_cow: + /// snapshot₁ (no file) → map file → snapshot₂ → restore₁ (unmapped) + /// → restore₂ (data folded into snapshot). + #[test] + fn test_map_file_cow_snapshot_remapping_cycle() { + let expected = b"snapshot remapping cycle test!"; + let (path, expected_bytes) = + create_test_file("hyperlight_test_map_file_cow_snapshot_remap.bin", expected); + + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + let guest_base: u64 = 0x1_0000_0000; + + // 1. snapshot₁ — no file mapped + let snapshot1 = sbox.snapshot().unwrap(); + + // 2. Map the file + sbox.map_file_cow(&path, guest_base).unwrap(); + + // Verify we can read it + let actual: Vec = sbox + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, true), + ) + .unwrap(); + assert_eq!(actual, expected_bytes); + + // 3. snapshot₂ — file mapped (data folded into snapshot) + let snapshot2 = sbox.snapshot().unwrap(); + + // 4. Restore to snapshot₁ — file should be unmapped + sbox.restore(snapshot1.clone()).unwrap(); + let is_mapped: bool = sbox.call("CheckMapped", (guest_base,)).unwrap(); + assert!( + !is_mapped, + "Region should be unmapped after restoring to snapshot₁" + ); + + // 5. Restore to snapshot₂ — data should still be readable + // (folded into snapshot memory, not the original file mapping) + sbox.restore(snapshot2).unwrap(); + let is_mapped: bool = sbox.call("CheckMapped", (guest_base,)).unwrap(); + assert!( + is_mapped, + "Region should be mapped after restoring to snapshot₂" + ); + let actual2: Vec = sbox + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, false), + ) + .unwrap(); + assert_eq!( + actual2, expected_bytes, + "Data should be intact after snapshot₂ restore" + ); + + let _ = std::fs::remove_file(&path); + } + + /// Tests that snapshot correctly captures map_file_cow data and + /// restore brings it back. + #[test] + fn test_map_file_cow_snapshot_restore() { + let expected = b"snapshot restore basic test!!"; + let (path, expected_bytes) = + create_test_file("hyperlight_test_map_file_cow_snap_restore.bin", expected); + + let mut sbox = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_string().expect("Guest Binary Missing")), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + let guest_base: u64 = 0x1_0000_0000; + sbox.map_file_cow(&path, guest_base).unwrap(); + + // Read the content to verify mapping works + let actual: Vec = sbox + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, true), + ) + .unwrap(); + assert_eq!(actual, expected_bytes); + + // Take snapshot — folds file data into snapshot memory + let snapshot = sbox.snapshot().unwrap(); + + // Restore — the file-backed region is unmapped but data is in snapshot + sbox.restore(snapshot).unwrap(); + + // Data should still be readable from snapshot memory + let actual2: Vec = sbox + .call( + "ReadMappedBuffer", + (guest_base, expected_bytes.len() as u64, false), + ) + .unwrap(); + assert_eq!( + actual2, expected_bytes, + "Data should be readable after restore from snapshot" + ); + + let _ = std::fs::remove_file(&path); + } } From dc6a6986decece29d5207587f9e10667d09c6de8 Mon Sep 17 00:00:00 2001 From: Simon Davies Date: Fri, 6 Mar 2026 14:15:37 +0000 Subject: [PATCH 6/8] fix: address code review findings H1: Add null check after MapViewOfFileNuma2 in SurrogateProcess::map H2: Store SurrogateMapping in HandleMapping, add debug_assert_eq on reuse M1: Clean up surrogate mapping on VirtualProtectEx failure (pre-existing) M2: Use checked_sub for ref count, log error on underflow M5: Early return with clear error for empty (0-byte) files L1: Fix incorrect Send/Sync safety comment on OwnedFileMapping L3: Rename _fp/_guest_base to file_path/guest_base L4: Use usize::try_from(file_size) instead of silent truncation N2: Use page_size::get() in test helper instead of magic 4096 N3: Change SurrogateMapping and surrogate_mapping field to pub(crate) N4: Replace low-value derive trait test with meaningful variant test Also: Change MemoryRegionType::Heap to Code for file mappings, add tracing::error in release-mode vacant unmap path. All 266 tests pass, 0 failures. Signed-off-by: Simon Davies --- .../src/hypervisor/surrogate_process.rs | 35 ++++++++++++++- src/hyperlight_host/src/mem/memory_region.rs | 22 +++++----- .../src/sandbox/initialized_multi_use.rs | 43 +++++++++++++------ 3 files changed, 74 insertions(+), 26 deletions(-) diff --git a/src/hyperlight_host/src/hypervisor/surrogate_process.rs b/src/hyperlight_host/src/hypervisor/surrogate_process.rs index 59c650963..3493269b9 100644 --- a/src/hyperlight_host/src/hypervisor/surrogate_process.rs +++ b/src/hyperlight_host/src/hypervisor/surrogate_process.rs @@ -37,6 +37,9 @@ use crate::{Result, log_then_return}; pub(crate) struct HandleMapping { pub(crate) use_count: u64, pub(crate) surrogate_base: *mut c_void, + /// The mapping type used when this entry was first created. + /// Used for debug assertions to catch conflicting re-maps. + pub(crate) mapping_type: SurrogateMapping, } /// Contains details of a surrogate process to be used by a Sandbox for providing memory to a HyperV VM on Windows. @@ -79,6 +82,14 @@ impl SurrogateProcess { ) -> Result<*mut c_void> { match self.mappings.entry(host_base) { Entry::Occupied(mut oe) => { + debug_assert_eq!( + oe.get().mapping_type, + *mapping, + "Conflicting SurrogateMapping for host_base {host_base:#x}: \ + existing={:?}, requested={:?}", + oe.get().mapping_type, + mapping + ); oe.get_mut().use_count += 1; Ok(oe.get().surrogate_base) } @@ -105,6 +116,13 @@ impl SurrogateProcess { ) }; + if surrogate_base.Value.is_null() { + log_then_return!( + "MapViewOfFileNuma2 failed: {:?}", + std::io::Error::last_os_error() + ); + } + // Only set guard pages for SandboxMemory mappings. // File-backed read-only mappings do not need guard pages // because the host does not write to them. @@ -122,6 +140,7 @@ impl SurrogateProcess { &mut unused_out_old_prot_flags, ) } { + self.unmap_helper(surrogate_base.Value); log_then_return!(WindowsAPIError(e.clone())); } @@ -137,6 +156,7 @@ impl SurrogateProcess { &mut unused_out_old_prot_flags, ) } { + self.unmap_helper(surrogate_base.Value); log_then_return!(WindowsAPIError(e.clone())); } } @@ -144,6 +164,7 @@ impl SurrogateProcess { ve.insert(HandleMapping { use_count: 1, surrogate_base: surrogate_base.Value, + mapping_type: *mapping, }); Ok(surrogate_base.Value) } @@ -153,15 +174,25 @@ impl SurrogateProcess { pub(super) fn unmap(&mut self, host_base: usize) { match self.mappings.entry(host_base) { Entry::Occupied(mut oe) => { - oe.get_mut().use_count -= 1; + oe.get_mut().use_count = oe.get().use_count.checked_sub(1).unwrap_or_else(|| { + tracing::error!( + "Surrogate unmap ref count underflow for host_base {:#x}", + host_base + ); + 0 + }); if oe.get().use_count == 0 { let entry = oe.remove(); self.unmap_helper(entry.surrogate_base); } } Entry::Vacant(_) => { + tracing::error!( + "Attempted to unmap from surrogate a region at host_base {:#x} that was never mapped", + host_base + ); #[cfg(debug_assertions)] - panic!("Attempted to unmap from surrogate a region that was never mapped") + panic!("Attempted to unmap from surrogate a region that was never mapped"); } } } diff --git a/src/hyperlight_host/src/mem/memory_region.rs b/src/hyperlight_host/src/mem/memory_region.rs index b29c30fbb..0ea8b617d 100644 --- a/src/hyperlight_host/src/mem/memory_region.rs +++ b/src/hyperlight_host/src/mem/memory_region.rs @@ -169,7 +169,7 @@ impl MemoryRegionKind for HostGuestMemoryRegion { /// behaviour when projected into the surrogate process via `MapViewOfFileNuma2`. #[cfg(target_os = "windows")] #[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)] -pub enum SurrogateMapping { +pub(crate) enum SurrogateMapping { /// Standard sandbox shared memory: mapped with `PAGE_READWRITE` protection /// and guard pages (`PAGE_NOACCESS`) set on the first and last pages. SandboxMemory, @@ -196,7 +196,7 @@ pub struct HostRegionBase { pub offset: usize, /// How this region should be mapped through the surrogate process. /// Controls page protection and guard page behaviour. - pub surrogate_mapping: SurrogateMapping, + pub(crate) surrogate_mapping: SurrogateMapping, } #[cfg(target_os = "windows")] impl std::hash::Hash for HostRegionBase { @@ -497,14 +497,16 @@ mod tests { } #[test] - fn surrogate_mapping_copy_clone_debug() { - let a = SurrogateMapping::ReadOnlyFile; - let b = a; // Copy - let c = a; // Also Copy (SurrogateMapping implements Copy) - assert_eq!(a, b); - assert_eq!(a, c); - // Debug should produce a non-empty string - assert!(!format!("{:?}", a).is_empty()); + fn surrogate_mapping_variants_are_distinct() { + // Verify the two variants are distinct values that can be + // used to key different behaviour in the surrogate pipeline + let sandbox = SurrogateMapping::SandboxMemory; + let readonly = SurrogateMapping::ReadOnlyFile; + assert_ne!(sandbox, readonly); + + // Verify Copy semantics work (enum is Copy + Eq) + let copy = sandbox; + assert_eq!(sandbox, copy); } #[test] diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index bacc3a250..501decb21 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -95,9 +95,10 @@ impl Drop for OwnedFileMapping { } } -// SAFETY: The raw pointer `view_base` is only accessed during `Drop` -// (to call `UnmapViewOfFile`), which happens on the owning thread. -// The `HandleWrapper` is already `Send + Sync`. +// SAFETY: `view_base` is a pointer to a Windows memory-mapped view created by +// `MapViewOfFile`. Both `UnmapViewOfFile` and `CloseHandle` are thread-safe +// Win32 APIs that operate on kernel objects, so they can safely be called +// from any thread. The `HandleWrapper` is already `Send + Sync`. #[cfg(target_os = "windows")] unsafe impl Send for OwnedFileMapping {} #[cfg(target_os = "windows")] @@ -621,8 +622,8 @@ impl MultiUseSandbox { /// /// This method will return [`crate::HyperlightError::PoisonedSandbox`] if the sandbox /// is currently poisoned. Use [`restore()`](Self::restore) to recover from a poisoned state. - #[instrument(err(Debug), skip(self, _fp, _guest_base), parent = Span::current())] - pub fn map_file_cow(&mut self, _fp: &Path, _guest_base: u64) -> Result { + #[instrument(err(Debug), skip(self, file_path, guest_base), parent = Span::current())] + pub fn map_file_cow(&mut self, file_path: &Path, guest_base: u64) -> Result { if self.poisoned { return Err(crate::HyperlightError::PoisonedSandbox); } @@ -632,10 +633,18 @@ impl MultiUseSandbox { use windows::Win32::Foundation::HANDLE; - let file = std::fs::File::options().read(true).open(_fp)?; + let file = std::fs::File::options().read(true).open(file_path)?; let file_size = file.metadata()?.len(); + if file_size == 0 { + log_then_return!("map_file_cow: cannot map an empty file: {:?}", file_path); + } let page_size = page_size::get(); - let size = (file_size as usize).div_ceil(page_size) * page_size; + let size = usize::try_from(file_size).map_err(|_| { + HyperlightError::Error(format!( + "File size {file_size} exceeds addressable range on this platform" + )) + })?; + let size = size.div_ceil(page_size) * page_size; let file_handle = HANDLE(file.as_raw_handle()); @@ -680,9 +689,9 @@ impl MultiUseSandbox { let region = MemoryRegion { host_region: host_base..host_end, - guest_region: _guest_base as usize.._guest_base as usize + size, + guest_region: guest_base as usize..guest_base as usize + size, flags: MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE, - region_type: MemoryRegionType::Heap, + region_type: MemoryRegionType::Code, }; // Reset snapshot since we are mutating the sandbox state @@ -702,7 +711,7 @@ impl MultiUseSandbox { self.mem_mgr.mapped_rgns += 1; self.file_mappings.push(OwnedFileMapping { - guest_base: _guest_base as usize, + guest_base: guest_base as usize, view_base: view.Value, mapping_handle: HandleWrapper::from(mapping_handle), }); @@ -711,8 +720,14 @@ impl MultiUseSandbox { } #[cfg(unix)] unsafe { - let file = std::fs::File::options().read(true).write(true).open(_fp)?; + let file = std::fs::File::options() + .read(true) + .write(true) + .open(file_path)?; let file_size = file.metadata()?.st_size(); + if file_size == 0 { + log_then_return!("map_file_cow: cannot map an empty file: {:?}", file_path); + } let page_size = page_size::get(); let size = (file_size as usize).div_ceil(page_size) * page_size; let base = libc::mmap( @@ -729,9 +744,9 @@ impl MultiUseSandbox { if let Err(err) = self.map_region(&MemoryRegion { host_region: base as usize..base.wrapping_add(size) as usize, - guest_region: _guest_base as usize.._guest_base as usize + size, + guest_region: guest_base as usize..guest_base as usize + size, flags: MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE, - region_type: MemoryRegionType::Heap, + region_type: MemoryRegionType::Code, }) { libc::munmap(base, size); return Err(err); @@ -1767,7 +1782,7 @@ mod tests { fn create_test_file(name: &str, content: &[u8]) -> (std::path::PathBuf, Vec) { use std::io::Write; - let page_size = 4096usize; + let page_size = page_size::get(); let padded_len = content.len().max(page_size).div_ceil(page_size) * page_size; let mut padded = vec![0u8; padded_len]; padded[..content.len()].copy_from_slice(content); From c5f8f5c051a019d79d3bdfd494dfbbeaa9e1c775 Mon Sep 17 00:00:00 2001 From: Simon Davies Date: Tue, 10 Mar 2026 14:48:10 +0000 Subject: [PATCH 7/8] fix: add MappedFile region type, downgrade MemoryRegionType to pub(crate), fix clippy - Add MemoryRegionType::MappedFile variant for map_file_cow regions - Downgrade MemoryRegionType from pub to pub(crate) (not part of public API) - Use MappedFile consistently on both Windows and Linux in map_file_cow - Replace disallowed debug_assert_eq! with tracing::warn! in surrogate_process - Fix Justfile merge conflict Signed-off-by: Simon Davies --- .../src/hypervisor/surrogate_process.rs | 16 ++++++++-------- src/hyperlight_host/src/mem/memory_region.rs | 12 ++++++++++-- .../src/sandbox/initialized_multi_use.rs | 4 ++-- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/hyperlight_host/src/hypervisor/surrogate_process.rs b/src/hyperlight_host/src/hypervisor/surrogate_process.rs index 3493269b9..6c439dbf6 100644 --- a/src/hyperlight_host/src/hypervisor/surrogate_process.rs +++ b/src/hyperlight_host/src/hypervisor/surrogate_process.rs @@ -82,14 +82,14 @@ impl SurrogateProcess { ) -> Result<*mut c_void> { match self.mappings.entry(host_base) { Entry::Occupied(mut oe) => { - debug_assert_eq!( - oe.get().mapping_type, - *mapping, - "Conflicting SurrogateMapping for host_base {host_base:#x}: \ - existing={:?}, requested={:?}", - oe.get().mapping_type, - mapping - ); + if oe.get().mapping_type != *mapping { + tracing::warn!( + "Conflicting SurrogateMapping for host_base {host_base:#x}: \ + existing={:?}, requested={:?}", + oe.get().mapping_type, + mapping + ); + } oe.get_mut().use_count += 1; Ok(oe.get().surrogate_base) } diff --git a/src/hyperlight_host/src/mem/memory_region.rs b/src/hyperlight_host/src/mem/memory_region.rs index 0ea8b617d..9010ff85b 100644 --- a/src/hyperlight_host/src/mem/memory_region.rs +++ b/src/hyperlight_host/src/mem/memory_region.rs @@ -112,10 +112,13 @@ impl TryFrom for MemoryRegionFlags { } } -// only used for debugging +// NOTE: In the future, all host-side knowledge about memory region types +// should collapse down to Snapshot vs Scratch (see shared_mem.rs). +// Until then, these variants help distinguish regions for diagnostics +// and crash dumps. Not part of the public API. #[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)] /// The type of memory region -pub enum MemoryRegionType { +pub(crate) enum MemoryRegionType { /// The region contains the guest's code Code, /// The region contains the guest's init data @@ -128,6 +131,11 @@ pub enum MemoryRegionType { Scratch, /// The snapshot region Snapshot, + /// An externally-mapped file (via [`MultiUseSandbox::map_file_cow`]). + /// These regions are backed by file handles (Windows) or mmap + /// (Linux) and are read-only + executable. They are cleaned up + /// during restore/drop — not part of the guest's own allocator. + MappedFile, } /// A trait that distinguishes between different kinds of memory region representations. diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index 501decb21..bfd8e9de2 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -691,7 +691,7 @@ impl MultiUseSandbox { host_region: host_base..host_end, guest_region: guest_base as usize..guest_base as usize + size, flags: MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE, - region_type: MemoryRegionType::Code, + region_type: MemoryRegionType::MappedFile, }; // Reset snapshot since we are mutating the sandbox state @@ -746,7 +746,7 @@ impl MultiUseSandbox { host_region: base as usize..base.wrapping_add(size) as usize, guest_region: guest_base as usize..guest_base as usize + size, flags: MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE, - region_type: MemoryRegionType::Code, + region_type: MemoryRegionType::MappedFile, }) { libc::munmap(base, size); return Err(err); From 253fc42f23ed785f108dc70de61b93c18a13af64 Mon Sep 17 00:00:00 2001 From: Simon Davies Date: Tue, 10 Mar 2026 17:05:42 +0000 Subject: [PATCH 8/8] Fix clippy Signed-off-by: Simon Davies --- src/hyperlight_host/src/mem/memory_region.rs | 2 +- src/hyperlight_host/src/sandbox/initialized_multi_use.rs | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/hyperlight_host/src/mem/memory_region.rs b/src/hyperlight_host/src/mem/memory_region.rs index 9010ff85b..2bfcfdf49 100644 --- a/src/hyperlight_host/src/mem/memory_region.rs +++ b/src/hyperlight_host/src/mem/memory_region.rs @@ -272,7 +272,7 @@ pub struct MemoryRegion_ { /// memory access flags for the given region pub flags: MemoryRegionFlags, /// the type of memory region - pub region_type: MemoryRegionType, + pub(crate) region_type: MemoryRegionType, } /// A memory region that tracks both host and guest addresses. diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index bfd8e9de2..aae4b6dd9 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -84,10 +84,11 @@ impl Drop for OwnedFileMapping { }) { tracing::error!("Failed to unmap file view at {:?}: {:?}", self.view_base, e); } - if let Err(e) = CloseHandle(self.mapping_handle.into()) { + let mapping_handle = self.mapping_handle.into(); + if let Err(e) = CloseHandle(mapping_handle) { tracing::error!( "Failed to close file mapping handle {:?}: {:?}", - self.mapping_handle, + mapping_handle, e ); } @@ -394,7 +395,7 @@ impl MultiUseSandbox { .map_err(HyperlightVmError::UnmapRegion)?; // Clean up host-side file mapping resources for regions that - // were created by map_file_cow. The OwnedFileMapping::Drop + // were created by map_file_cow. The OwnedFileMapping::drop // will call UnmapViewOfFile + CloseHandle. #[cfg(target_os = "windows")] self.file_mappings