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 <simongdavies@users.noreply.github.com>

Author: simongdavies

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,13 +156,15 @@ impl SurrogateProcess {
                             &mut unused_out_old_prot_flags,
                         )
                     } {
+                        self.unmap_helper(surrogate_base.Value);
                         log_then_return!(WindowsAPIError(e.clone()));
                     }
                 }
 
                 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");
             }
         }
     }

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]

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<u64> {
+    #[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<u64> {
         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<u8>) {
         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);