Review Feedback

Signed-off-by: Simon Davies <simongdavies@users.noreply.github.com>

Author: simongdavies

src/hyperlight_host/src/hypervisor/virtual_machine/whp.rs

@@ -20,9 +20,10 @@ use std::os::raw::c_void;
 use tracing::Span;
 #[cfg(feature = "trace_guest")]
 use tracing_opentelemetry::OpenTelemetrySpanExt;
-use windows::Win32::Foundation::{FreeLibrary, HANDLE};
+use windows::Win32::Foundation::{CloseHandle, FreeLibrary, HANDLE};
 use windows::Win32::System::Hypervisor::*;
 use windows::Win32::System::LibraryLoader::*;
+use windows::Win32::System::Memory::{MEMORY_MAPPED_VIEW_ADDRESS, UnmapViewOfFile};
 use windows::core::s;
 use windows_result::HRESULT;
 
@@ -40,7 +41,8 @@ use crate::hypervisor::virtual_machine::{
     CreateVmError, HypervisorError, MapMemoryError, RegisterError, RunVcpuError, UnmapMemoryError,
     VirtualMachine, VmExit, XSAVE_MIN_SIZE,
 };
-use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags};
+use crate::hypervisor::wrappers::HandleWrapper;
+use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags, MemoryRegionType};
 #[cfg(feature = "trace_guest")]
 use crate::sandbox::trace::TraceContext as SandboxTraceContext;
 
@@ -65,19 +67,72 @@ pub(crate) fn is_hypervisor_present() -> bool {
     }
 }
 
+/// Tracks a host-side file mapping created by [`MultiUseSandbox::map_file_cow`],
+/// providing RAII cleanup of the underlying Windows handles.
+///
+/// When dropped, releases both the `MapViewOfFile` view and the
+/// `CreateFileMappingW` handle, preventing resource leaks.
+struct OwnedFileMapping {
+    /// Host-side base pointer returned by `MapViewOfFile`.
+    view_base: *mut core::ffi::c_void,
+    /// File mapping handle returned by `CreateFileMappingW`.
+    mapping_handle: HandleWrapper,
+}
+
+impl std::fmt::Debug for OwnedFileMapping {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("OwnedFileMapping")
+            .field("view_base", &self.view_base)
+            .field("mapping_handle", &self.mapping_handle)
+            .finish()
+    }
+}
+
+impl Drop for OwnedFileMapping {
+    fn drop(&mut self) {
+        // Unmap the view first, then close the file mapping handle.
+        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: `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`.
+unsafe impl Send for OwnedFileMapping {}
+unsafe impl Sync for OwnedFileMapping {}
+
 /// A Windows Hypervisor Platform implementation of a single-vcpu VM
 #[derive(Debug)]
 pub(crate) struct WhpVm {
     partition: WHV_PARTITION_HANDLE,
     // Surrogate process for memory mapping
     surrogate_process: SurrogateProcess,
+    /// Tracks host-side file mappings created by `map_file_cow` for
+    /// RAII cleanup. Keyed by host handle_base address.
+    file_mappings: Vec<OwnedFileMapping>,
 }
 
 // Safety: `WhpVm` is !Send because it holds `SurrogateProcess` which contains a raw pointer
 // `allocated_address` (*mut c_void). This pointer represents a memory mapped view address
 // in the surrogate process. It is never dereferenced, only used for address arithmetic and
 // resource management (unmapping). This is a system resource that is not bound to the creating
 // thread and can be safely transferred between threads.
+// `OwnedFileMapping` contains a raw pointer (`view_base`) that is also a kernel resource
+// handle, safe to use from any thread.
 unsafe impl Send for WhpVm {}
 
 impl WhpVm {
@@ -109,6 +164,7 @@ impl WhpVm {
         Ok(WhpVm {
             partition,
             surrogate_process,
+            file_mappings: Vec::new(),
         })
     }
 
@@ -144,7 +200,7 @@ impl VirtualMachine for WhpVm {
                 region.host_region.start.from_handle,
                 region.host_region.start.handle_base,
                 region.host_region.start.handle_size,
-                &region.host_region.start.surrogate_mapping,
+                &region.region_type.surrogate_mapping(),
             )
             .map_err(|e| MapMemoryError::SurrogateProcess(e.to_string()))?;
         let surrogate_addr = surrogate_base.wrapping_add(region.host_region.start.offset);
@@ -194,6 +250,16 @@ impl VirtualMachine for WhpVm {
             )));
         }
 
+        // Track host-side file mappings for RAII cleanup.
+        // MappedFile regions have host views + handles that need
+        // UnmapViewOfFile + CloseHandle when the region is unmapped.
+        if region.region_type == MemoryRegionType::MappedFile {
+            self.file_mappings.push(OwnedFileMapping {
+                view_base: region.host_region.start.handle_base as *mut core::ffi::c_void,
+                mapping_handle: region.host_region.start.from_handle,
+            });
+        }
+
         Ok(())
     }
 
@@ -211,6 +277,15 @@ impl VirtualMachine for WhpVm {
         }
         self.surrogate_process
             .unmap(region.host_region.start.handle_base);
+
+        // Clean up host-side file mapping resources for MappedFile regions.
+        // OwnedFileMapping::Drop calls UnmapViewOfFile + CloseHandle.
+        if region.region_type == MemoryRegionType::MappedFile {
+            let handle_base = region.host_region.start.handle_base;
+            self.file_mappings
+                .retain(|fm| fm.view_base as usize != handle_base);
+        }
+
         Ok(())
     }
 

src/hyperlight_host/src/mem/memory_region.rs

@@ -138,6 +138,21 @@ pub(crate) enum MemoryRegionType {
     MappedFile,
 }
 
+#[cfg(target_os = "windows")]
+impl MemoryRegionType {
+    /// Derives the [`SurrogateMapping`] from this region type.
+    ///
+    /// `MappedFile` regions use read-only file-backed mappings with no
+    /// guard pages; all other region types use the standard sandbox
+    /// shared memory mapping with guard pages.
+    pub(crate) fn surrogate_mapping(&self) -> SurrogateMapping {
+        match self {
+            MemoryRegionType::MappedFile => SurrogateMapping::ReadOnlyFile,
+            _ => SurrogateMapping::SandboxMemory,
+        }
+    }
+}
+
 /// A trait that distinguishes between different kinds of memory region representations.
 ///
 /// This trait is used to parameterize [`MemoryRegion_`]
@@ -202,9 +217,6 @@ 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(crate) surrogate_mapping: SurrogateMapping,
 }
 #[cfg(target_os = "windows")]
 impl std::hash::Hash for HostRegionBase {
@@ -216,7 +228,6 @@ 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")]
@@ -242,7 +253,6 @@ impl MemoryRegionKind for HostGuestMemoryRegion {
             handle_base: base.handle_base,
             handle_size: base.handle_size,
             offset: base.offset + size,
-            surrogate_mapping: base.surrogate_mapping,
         }
     }
 }
@@ -442,161 +452,3 @@ 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_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]
-        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 = <HostGuestMemoryRegion as MemoryRegionKind>::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
-                    ..<HostGuestMemoryRegion as MemoryRegionKind>::add(base_sandbox, 0x2000),
-                flags: MemoryRegionFlags::READ,
-                region_type: MemoryRegionType::Code,
-            };
-
-            let region_readonly = MemoryRegion {
-                guest_region: 0x0..0x2000,
-                host_region: base_readonly
-                    ..<HostGuestMemoryRegion as MemoryRegionKind>::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);
-        }
-    }
-}

src/hyperlight_host/src/mem/shared_mem.rs

@@ -774,7 +774,6 @@ 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,
         }
     }