From 04f3a9066eb227940b59af5050aac7f0aaa79e64 Mon Sep 17 00:00:00 2001 From: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Date: Thu, 13 Aug 2026 12:35:47 -0700 Subject: [PATCH 1/5] borrow memory manager instead of cloning, make manager non-clone Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> --- Justfile | 2 +- src/hyperlight_host/src/hypervisor/gdb/mod.rs | 253 +++++++++--------- .../src/hypervisor/hyperlight_vm/aarch64.rs | 22 +- .../src/hypervisor/hyperlight_vm/mod.rs | 9 +- .../src/hypervisor/hyperlight_vm/x86_64.rs | 84 ++---- src/hyperlight_host/src/hypervisor/mod.rs | 5 - src/hyperlight_host/src/mem/mgr.rs | 1 - .../src/sandbox/initialized_multi_use.rs | 40 +-- .../src/sandbox/uninitialized_evolve.rs | 19 +- 9 files changed, 161 insertions(+), 274 deletions(-) diff --git a/Justfile b/Justfile index f69d88c41..77289c68f 100644 --- a/Justfile +++ b/Justfile @@ -289,7 +289,7 @@ test-no-surrogate target=default-target: # runs tests that exercise gdb debugging test-rust-gdb-debugging target=default-target features="": {{ cargo-cmd }} test --profile={{ if target == "debug" { "dev" } else { target } }} {{ target-triple-flag }} --example guest-debugging {{ if features =="" {'--features gdb'} else { "--features gdb," + features } }} - {{ cargo-cmd }} test --profile={{ if target == "debug" { "dev" } else { target } }} {{ target-triple-flag }} {{ if features =="" {'--features gdb'} else { "--features gdb," + features } }} -- test_gdb + {{ cargo-cmd }} test --profile={{ if target == "debug" { "dev" } else { target } }} {{ target-triple-flag }} -p hyperlight-host --lib {{ if features =="" {'--features gdb'} else { "--features gdb," + features } }} -- hypervisor::gdb:: # rust test for crashdump test-rust-crashdump target=default-target features="": diff --git a/src/hyperlight_host/src/hypervisor/gdb/mod.rs b/src/hyperlight_host/src/hypervisor/gdb/mod.rs index 5f82be0c3..131dd350f 100644 --- a/src/hyperlight_host/src/hypervisor/gdb/mod.rs +++ b/src/hyperlight_host/src/hypervisor/gdb/mod.rs @@ -20,7 +20,7 @@ mod x86_64_target; use std::io::{self, ErrorKind}; use std::net::TcpListener; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use std::thread; use crossbeam_channel::{Receiver, Sender, TryRecvError}; @@ -78,12 +78,10 @@ impl From for TargetError { } } -/// This abstracts the memory access functions that debugging needs from a sandbox -pub(crate) struct DebugMemoryAccess { - /// Memory manager that provides access to the guest memory - pub(crate) dbg_mem_access_fn: Arc>>, - /// Guest mapped memory regions - pub(crate) guest_mmap_regions: Vec, +/// A borrowed view of the sandbox memory visible to GDB. +pub(crate) struct DebugMemoryView<'a> { + mem_mgr: &'a SandboxMemoryManager, + guest_mmap_regions: Vec, } /// Errors that can occur during debug memory access operations @@ -91,15 +89,27 @@ pub(crate) struct DebugMemoryAccess { pub enum DebugMemoryAccessError { #[error("Failed to copy memory: {0}")] CopyFailed(Box), - #[error("Failed to acquire lock at {0}:{1} - {2}")] - LockFailed(&'static str, u32, String), #[error("Failed to translate guest address {0:#x}")] TranslateGuestAddress(u64), #[error("Failed to write to read-only region")] WriteToReadOnly, } -impl DebugMemoryAccess { +impl<'a> DebugMemoryView<'a> { + pub(crate) fn new( + mem_mgr: &'a SandboxMemoryManager, + guest_mmap_regions: Vec, + ) -> Self { + Self { + mem_mgr, + guest_mmap_regions, + } + } + + pub(crate) fn code_section_offset(&self) -> u64 { + self.mem_mgr.layout.get_guest_code_address() as u64 + } + /// Reads memory from the guest's address space with a maximum length of a PAGE_SIZE /// /// # Arguments @@ -113,15 +123,11 @@ impl DebugMemoryAccess { data: &mut [u8], gpa: u64, ) -> std::result::Result<(), DebugMemoryAccessError> { - let mgr = self - .dbg_mem_access_fn - .try_lock() - .map_err(|e| DebugMemoryAccessError::LockFailed(file!(), line!(), e.to_string()))?; - - mgr.layout + self.mem_mgr + .layout .resolve_gpa(gpa, &self.guest_mmap_regions) .ok_or(DebugMemoryAccessError::TranslateGuestAddress(gpa))? - .with_memories(&mgr.shared_mem, &mgr.scratch_mem) + .with_memories(&self.mem_mgr.shared_mem, &self.mem_mgr.scratch_mem) .copy_to_slice(data) .map_err(|e| DebugMemoryAccessError::CopyFailed(Box::new(e))) } @@ -139,12 +145,8 @@ impl DebugMemoryAccess { data: &[u8], gpa: u64, ) -> std::result::Result<(), DebugMemoryAccessError> { - let mgr = self - .dbg_mem_access_fn - .try_lock() - .map_err(|e| DebugMemoryAccessError::LockFailed(file!(), line!(), e.to_string()))?; - - let resolved = mgr + let resolved = self + .mem_mgr .layout .resolve_gpa(gpa, &self.guest_mmap_regions) .ok_or(DebugMemoryAccessError::TranslateGuestAddress(gpa))?; @@ -153,11 +155,13 @@ impl DebugMemoryAccess { // process) if the address is in the scratch region match resolved.base { #[cfg(unshared_snapshot_mem)] - BaseGpaRegion::Snapshot(()) => mgr + BaseGpaRegion::Snapshot(()) => self + .mem_mgr .shared_mem .copy_from_slice(data, resolved.offset) .map_err(|e| DebugMemoryAccessError::CopyFailed(Box::new(e))), - BaseGpaRegion::Scratch(()) => mgr + BaseGpaRegion::Scratch(()) => self + .mem_mgr .scratch_mem .copy_from_slice(data, resolved.offset) .map_err(|e| DebugMemoryAccessError::CopyFailed(Box::new(e))), @@ -373,7 +377,6 @@ mod tests { mod mem_access_tests { use std::os::fd::AsRawFd; use std::os::linux::fs::MetadataExt; - use std::sync::{Arc, Mutex}; use hyperlight_testing::dummy_guest_as_pathbuf; @@ -387,117 +390,119 @@ mod tests { #[cfg(target_os = "linux")] const BASE_VIRT: usize = 0x10000000 + SandboxMemoryLayout::BASE_ADDRESS; - /// Dummy memory region to test memory access - /// This maps a file into memory and uses it as guest memory - fn get_mem_access() -> crate::Result { - let filename = dummy_guest_as_pathbuf(); - - let file = std::fs::File::options() - .read(true) - .write(true) - .open(&filename)?; - let file_size = file.metadata()?.st_size(); - let page_size = page_size::get(); - let size = (file_size as usize).div_ceil(page_size) * page_size; - let mapped_mem = unsafe { - libc::mmap( - std::ptr::null_mut(), - size, - libc::PROT_READ | libc::PROT_WRITE | libc::PROT_EXEC, - libc::MAP_PRIVATE, - file.as_raw_fd(), - 0, - ) - }; - if mapped_mem == libc::MAP_FAILED { - log_then_return!("mmap error: {:?}", std::io::Error::last_os_error()); + struct TestMemory { + mem_mgr: SandboxMemoryManager, + mmap_region: MemoryRegion, + } + + impl TestMemory { + fn new() -> crate::Result { + let filename = dummy_guest_as_pathbuf(); + let file = std::fs::File::options() + .read(true) + .write(true) + .open(&filename)?; + let file_size = file.metadata()?.st_size(); + let page_size = page_size::get(); + let size = (file_size as usize).div_ceil(page_size) * page_size; + + let sandbox = UninitializedSandbox::new(GuestBinary::FilePath(filename), None)?; + let (mem_mgr, _) = sandbox.mgr.build()?; + + // SAFETY: `file` is open for this call, and `size` is the page-aligned + // file size. `MAP_FAILED` is checked below. + let mapped_mem = unsafe { + libc::mmap( + std::ptr::null_mut(), + size, + libc::PROT_READ | libc::PROT_WRITE | libc::PROT_EXEC, + libc::MAP_PRIVATE, + file.as_raw_fd(), + 0, + ) + }; + if mapped_mem == libc::MAP_FAILED { + log_then_return!("mmap error: {:?}", std::io::Error::last_os_error()); + } + + Ok(Self { + mem_mgr, + mmap_region: MemoryRegion { + host_region: mapped_mem as usize..mapped_mem.wrapping_add(size) as usize, + guest_region: BASE_VIRT..BASE_VIRT + size, + flags: MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE, + region_type: MemoryRegionType::Heap, + }, + }) } - // Create a sandbox memory manager with the mapped memory region - let sandbox = UninitializedSandbox::new(GuestBinary::FilePath(filename.clone()), None) - .inspect_err(|_| unsafe { - libc::munmap(mapped_mem, size); - })?; - let (mem_mgr, _) = sandbox.mgr.build()?; - - // Create the memory access struct - let mem_access = DebugMemoryAccess { - dbg_mem_access_fn: Arc::new(Mutex::new(mem_mgr)), - guest_mmap_regions: vec![MemoryRegion { - host_region: mapped_mem as usize..mapped_mem.wrapping_add(size) as usize, - guest_region: BASE_VIRT..BASE_VIRT + size, - flags: MemoryRegionFlags::READ | MemoryRegionFlags::EXECUTE, - region_type: MemoryRegionType::Heap, - }], - }; - - Ok(mem_access) - } + fn access(&self) -> DebugMemoryView<'_> { + DebugMemoryView::new(&self.mem_mgr, vec![self.mmap_region.clone()]) + } - /// Gets a slice to the mapped memory region to be able to modify it - /// - /// NOTE: By returning a mutable slice from a mutable reference, we ensure - /// that the memory is not deallocated while the slice is in use. - unsafe fn get_mmap_slice(mem_access: &mut DebugMemoryAccess) -> &mut [u8] { - unsafe { - std::slice::from_raw_parts_mut( - mem_access.guest_mmap_regions[0].host_region.start as *mut u8, - mem_access.guest_mmap_regions[0].host_region.end - - mem_access.guest_mmap_regions[0].host_region.start, - ) + fn mmap_slice(&mut self) -> &mut [u8] { + // SAFETY: `mmap_region` describes the live mapping owned by `self`. + // The mutable borrow prevents overlapping slices from this method. + unsafe { + std::slice::from_raw_parts_mut( + self.mmap_region.host_region.start as *mut u8, + self.mmap_region.host_region.len(), + ) + } } } - /// Drops the mapped memory region - fn drop_mem_access(mem_access: DebugMemoryAccess) { - let mapped_mem = - mem_access.guest_mmap_regions[0].host_region.start as *mut libc::c_void; - let size = mem_access.guest_mmap_regions[0].host_region.end - - mem_access.guest_mmap_regions[0].host_region.start; - - unsafe { - libc::munmap(mapped_mem, size); + impl Drop for TestMemory { + fn drop(&mut self) { + // SAFETY: this is the mapping returned by `mmap` in `new`, with + // the same base and length, and `Drop` unmaps it exactly once. + unsafe { + libc::munmap( + self.mmap_region.host_region.start as *mut libc::c_void, + self.mmap_region.host_region.len(), + ); + } } } #[test] fn test_mem_access_read_single_byte() -> crate::Result<()> { - let mut mem_access = get_mem_access()?; + let mut memory = TestMemory::new()?; let offset = 2000; // Modify the memory directly to have a known value to read { - let slice = unsafe { get_mmap_slice(&mut mem_access) }; + let slice = memory.mmap_slice(); slice[offset] = 0xAA; } let mut read_data = [0u8; 1]; - mem_access + memory + .access() .read(&mut read_data, (BASE_VIRT + offset) as u64) .unwrap(); assert_eq!(read_data[0], 0xAA); - drop_mem_access(mem_access); - Ok(()) } #[test] fn test_mem_access_read_multiple_bytes() -> crate::Result<()> { - let mut mem_access = get_mem_access()?; + let mut memory = TestMemory::new()?; let offset = 20; // Modify the memory directly to have a known value to read { - let slice = unsafe { get_mmap_slice(&mut mem_access) }; + let slice = memory.mmap_slice(); for i in 0..16 { slice[offset + i] = i as u8; } } let mut read_data = [0u8; 16]; - mem_access + memory + .access() .read(&mut read_data, (BASE_VIRT + offset) as u64) .unwrap(); @@ -505,51 +510,49 @@ mod tests { read_data, [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15] ); - drop_mem_access(mem_access); Ok(()) } #[test] fn test_mem_access_write_single_byte() -> crate::Result<()> { - let mut mem_access = get_mem_access()?; - let offset = 3000; - { - let slice = unsafe { get_mmap_slice(&mut mem_access) }; - slice[offset] = 0xBB; - } + let memory = TestMemory::new()?; + let scratch_gpa = memory.mem_mgr.layout.get_first_free_scratch_gpa(); let write_data = [0xCCu8; 1]; - mem_access - .write(&write_data, (BASE_VIRT + offset) as u64) - .unwrap(); + memory.access().write(&write_data, scratch_gpa).unwrap(); - let slice = unsafe { get_mmap_slice(&mut mem_access) }; - assert_eq!(slice[offset], write_data[0]); - drop_mem_access(mem_access); + let mut actual = [0u8; 1]; + memory.access().read(&mut actual, scratch_gpa).unwrap(); + assert_eq!(actual, write_data); Ok(()) } #[test] fn test_mem_access_write_multiple_bytes() -> crate::Result<()> { - let mut mem_access = get_mem_access()?; - let offset = 56; - { - let slice = unsafe { get_mmap_slice(&mut mem_access) }; - for i in 0..16 { - slice[offset + i] = i as u8; - } - } + let memory = TestMemory::new()?; + let scratch_gpa = memory.mem_mgr.layout.get_first_free_scratch_gpa(); let write_data = [0xAAu8; 16]; - mem_access - .write(&write_data, (BASE_VIRT + offset) as u64) - .unwrap(); + memory.access().write(&write_data, scratch_gpa).unwrap(); + + let mut actual = [0u8; 16]; + memory.access().read(&mut actual, scratch_gpa).unwrap(); + assert_eq!(actual, write_data); + + Ok(()) + } + + #[test] + fn test_mem_access_write_rejects_mmap_region() -> crate::Result<()> { + let memory = TestMemory::new()?; - let slice = unsafe { get_mmap_slice(&mut mem_access) }; - assert_eq!(slice[offset..offset + 16], write_data); - drop_mem_access(mem_access); + let error = memory + .access() + .write(&[0xAA], BASE_VIRT as u64) + .unwrap_err(); + assert!(matches!(error, DebugMemoryAccessError::WriteToReadOnly)); Ok(()) } } diff --git a/src/hyperlight_host/src/hypervisor/hyperlight_vm/aarch64.rs b/src/hyperlight_host/src/hypervisor/hyperlight_vm/aarch64.rs index 770959c14..6ac13815a 100644 --- a/src/hyperlight_host/src/hypervisor/hyperlight_vm/aarch64.rs +++ b/src/hyperlight_host/src/hypervisor/hyperlight_vm/aarch64.rs @@ -115,9 +115,6 @@ impl HyperlightVm { mem_mgr: &mut SandboxMemoryManager, host_funcs: &Arc>, guest_max_log_level: Option, - #[cfg(gdb)] dbg_mem_access_fn: Arc< - std::sync::Mutex>, - >, ) -> Result<(), InitializeError> { let NextAction::Initialise(initialise) = self.next_action else { return Ok(()); @@ -136,13 +133,8 @@ impl HyperlightVm { }; self.vm.set_regs(®s)?; - self.run( - mem_mgr, - host_funcs, - #[cfg(gdb)] - dbg_mem_access_fn, - ) - .map_err(InitializeError::Run)?; + self.run(mem_mgr, host_funcs) + .map_err(InitializeError::Run)?; let regs = self.vm.regs()?; if !regs.sp.is_multiple_of(16) { @@ -158,9 +150,6 @@ impl HyperlightVm { &mut self, mem_mgr: &mut SandboxMemoryManager, host_funcs: &Arc>, - #[cfg(gdb)] dbg_mem_access_fn: Arc< - std::sync::Mutex>, - >, ) -> Result<(), DispatchGuestCallError> { let NextAction::Call(dispatch_func_addr) = self.next_action else { return Err(DispatchGuestCallError::Uninitialized); @@ -182,12 +171,7 @@ impl HyperlightVm { .set_fpu(&CommonFpu::default()) .map_err(DispatchGuestCallError::SetupRegs)?; let result = self - .run( - mem_mgr, - host_funcs, - #[cfg(gdb)] - dbg_mem_access_fn, - ) + .run(mem_mgr, host_funcs) .map_err(DispatchGuestCallError::Run); self.pending_tlb_flush = false; result diff --git a/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs b/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs index d5411d80e..93368a335 100644 --- a/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs +++ b/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs @@ -598,7 +598,6 @@ impl HyperlightVm { &mut self, mem_mgr: &mut SandboxMemoryManager, host_funcs: &Arc>, - #[cfg(gdb)] dbg_mem_access_fn: Arc>>, ) -> std::result::Result<(), RunVmError> { // Keeps the trace context and open spans #[cfg(feature = "trace_guest")] @@ -694,7 +693,7 @@ impl HyperlightVm { self.one_shot_entry_bp = None; } } - if let Err(e) = self.handle_debug(dbg_mem_access_fn.clone(), stop_reason) { + if let Err(e) = self.handle_debug(mem_mgr, stop_reason) { break Err(e.into()); } } @@ -759,9 +758,7 @@ impl HyperlightVm { #[cfg(gdb)] { self.interrupt_handle.clear_debug_interrupt(); - if let Err(e) = - self.handle_debug(dbg_mem_access_fn.clone(), VcpuStopReason::Interrupt) - { + if let Err(e) = self.handle_debug(mem_mgr, VcpuStopReason::Interrupt) { break Err(e.into()); } } @@ -796,7 +793,7 @@ impl HyperlightVm { // Disregard return value as we want to return the error #[cfg(gdb)] if self.gdb_conn.is_some() { - self.handle_debug(dbg_mem_access_fn.clone(), VcpuStopReason::Crash)? + self.handle_debug(mem_mgr, VcpuStopReason::Crash)? } Err(e) } diff --git a/src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs b/src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs index 49abad98a..1227d60ec 100644 --- a/src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs +++ b/src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs @@ -221,7 +221,6 @@ impl HyperlightVm { mem_mgr: &mut SandboxMemoryManager, host_funcs: &Arc>, guest_max_log_level: Option, - #[cfg(gdb)] dbg_mem_access_fn: Arc>>, ) -> std::result::Result<(), InitializeError> { let NextAction::Initialise(initialise) = self.next_action else { return Ok(()); @@ -248,13 +247,8 @@ impl HyperlightVm { }; self.vm.set_regs(®s)?; - self.run( - mem_mgr, - host_funcs, - #[cfg(gdb)] - dbg_mem_access_fn, - ) - .map_err(InitializeError::Run)?; + self.run(mem_mgr, host_funcs) + .map_err(InitializeError::Run)?; let regs = self.vm.regs()?; // todo(portability): this is architecture-specific @@ -317,7 +311,6 @@ impl HyperlightVm { &mut self, mem_mgr: &mut SandboxMemoryManager, host_funcs: &Arc>, - #[cfg(gdb)] dbg_mem_access_fn: Arc>>, ) -> std::result::Result<(), DispatchGuestCallError> { let NextAction::Call(dispatch_func_addr) = self.next_action else { return Err(DispatchGuestCallError::Uninitialized); @@ -352,12 +345,7 @@ impl HyperlightVm { .map_err(DispatchGuestCallError::SetupRegs)?; let result = self - .run( - mem_mgr, - host_funcs, - #[cfg(gdb)] - dbg_mem_access_fn, - ) + .run(mem_mgr, host_funcs) .map_err(DispatchGuestCallError::Run); // Clear the TLB flush flag only after run() returns. The guest @@ -418,24 +406,19 @@ impl HyperlightVm { #[cfg(gdb)] pub(super) fn handle_debug( &mut self, - dbg_mem_access_fn: Arc>>, + mem_mgr: &SandboxMemoryManager, stop_reason: VcpuStopReason, ) -> std::result::Result<(), HandleDebugError> { use debug::ProcessDebugRequestError; - use crate::hypervisor::gdb::DebugMemoryAccess; + use crate::hypervisor::gdb::DebugMemoryView; if self.gdb_conn.is_none() { return Err(HandleDebugError::DebugNotEnabled); } - let mem_access = DebugMemoryAccess { - // TODO: dbg_mem_access_fn could be out of sync with the - // actual snapshot/scratch regions, if a snapshot restore - // has caused either of those to change. - dbg_mem_access_fn, - guest_mmap_regions: self.get_mapped_regions().cloned().collect(), - }; + let mem_access = + DebugMemoryView::new(mem_mgr, self.get_mapped_regions().cloned().collect()); match stop_reason { // If the vCPU stopped because of a crash, we need to handle it differently @@ -650,7 +633,7 @@ pub(super) mod debug { use super::HyperlightVm; use crate::hypervisor::gdb::arch::{SW_BP, SW_BP_SIZE}; use crate::hypervisor::gdb::{ - DebugError, DebugMemoryAccess, DebugMemoryAccessError, DebugMsg, DebugResponse, + DebugError, DebugMemoryAccessError, DebugMemoryView, DebugMsg, DebugResponse, }; use crate::hypervisor::virtual_machine::VmError; @@ -659,8 +642,6 @@ pub(super) mod debug { pub enum ProcessDebugRequestError { #[error("Debug is not enabled")] DebugNotEnabled, - #[error("Failed to acquire lock at {0}:{1}")] - TryLockError(&'static str, u32), #[error("VM operation error: {0}")] Vm(#[from] VmError), #[error("Debug operation error: {0}")] @@ -677,7 +658,7 @@ pub(super) mod debug { pub(crate) fn process_dbg_request( &mut self, req: DebugMsg, - mem_access: &DebugMemoryAccess, + mem_access: &DebugMemoryView<'_>, ) -> std::result::Result { if self.gdb_conn.is_some() { match req { @@ -717,16 +698,9 @@ pub(super) mod debug { Ok(DebugResponse::DisableDebug) } - DebugMsg::GetCodeSectionOffset => { - let offset = mem_access - .dbg_mem_access_fn - .try_lock() - .map_err(|_| ProcessDebugRequestError::TryLockError(file!(), line!()))? - .layout - .get_guest_code_address(); - - Ok(DebugResponse::GetCodeSectionOffset(offset as u64)) - } + DebugMsg::GetCodeSectionOffset => Ok(DebugResponse::GetCodeSectionOffset( + mem_access.code_section_offset(), + )), DebugMsg::ReadAddr(addr, len) => { let mut data = vec![0u8; len]; @@ -826,7 +800,7 @@ pub(super) mod debug { &mut self, mut gva: u64, mut data: &mut [u8], - mem_access: &DebugMemoryAccess, + mem_access: &DebugMemoryView<'_>, ) -> std::result::Result<(), ProcessDebugRequestError> { let data_len = data.len(); tracing::debug!("Read addr: {:X} len: {:X}", gva, data_len); @@ -854,7 +828,7 @@ pub(super) mod debug { &mut self, mut gva: u64, mut data: &[u8], - mem_access: &DebugMemoryAccess, + mem_access: &DebugMemoryView<'_>, ) -> std::result::Result<(), ProcessDebugRequestError> { let data_len = data.len(); tracing::debug!("Write addr: {:X} len: {:X}", gva, data_len); @@ -883,7 +857,7 @@ pub(super) mod debug { fn add_sw_breakpoint( &mut self, gva: u64, - mem_access: &DebugMemoryAccess, + mem_access: &DebugMemoryView<'_>, ) -> std::result::Result<(), ProcessDebugRequestError> { // Check if breakpoint already exists if self.sw_breakpoints.contains_key(&gva) { @@ -904,7 +878,7 @@ pub(super) mod debug { fn remove_sw_breakpoint( &mut self, gva: u64, - mem_access: &DebugMemoryAccess, + mem_access: &DebugMemoryView<'_>, ) -> std::result::Result<(), ProcessDebugRequestError> { if let Some(saved_data) = self.sw_breakpoints.remove(&gva) { // Restore saved data to the guest's memory @@ -948,8 +922,6 @@ mod tests { vm: HyperlightVm, hshm: SandboxMemoryManager, host_funcs: Arc>, - #[cfg(gdb)] - dbg_mem_access_hdl: Arc>>, } // ========================================================================== @@ -1574,28 +1546,15 @@ mod tests { let seed = rand::rng().random::(); let peb_addr = RawPtr::from(u64::try_from(peb_address).unwrap()); - #[cfg(gdb)] - let dbg_mem_access_hdl = Arc::new(Mutex::new(hshm.clone())); - let host_funcs = Arc::new(Mutex::new(FunctionRegistry::default())); - vm.initialise( - peb_addr, - seed, - &mut hshm, - &host_funcs, - None, - #[cfg(gdb)] - dbg_mem_access_hdl.clone(), - ) - .unwrap(); + vm.initialise(peb_addr, seed, &mut hshm, &host_funcs, None) + .unwrap(); TestVmContext { vm, hshm, host_funcs, - #[cfg(gdb)] - dbg_mem_access_hdl, } } @@ -2184,12 +2143,7 @@ mod tests { fn run(&mut self) { self.ctx .vm - .run( - &mut self.ctx.hshm, - &self.ctx.host_funcs, - #[cfg(gdb)] - self.ctx.dbg_mem_access_hdl.clone(), - ) + .run(&mut self.ctx.hshm, &self.ctx.host_funcs) .unwrap(); } diff --git a/src/hyperlight_host/src/hypervisor/mod.rs b/src/hyperlight_host/src/hypervisor/mod.rs index e8f5a3d79..51c658423 100644 --- a/src/hyperlight_host/src/hypervisor/mod.rs +++ b/src/hyperlight_host/src/hypervisor/mod.rs @@ -506,9 +506,6 @@ pub(crate) mod tests { let host_funcs = Arc::new(Mutex::new(FunctionRegistry::default())); let guest_max_log_level = Some(tracing_core::LevelFilter::ERROR); - #[cfg(gdb)] - let dbg_mem_access_fn = Arc::new(Mutex::new(mem_mgr.clone())); - // Test the initialise method vm.initialise( peb_addr, @@ -516,8 +513,6 @@ pub(crate) mod tests { &mut mem_mgr, &host_funcs, guest_max_log_level, - #[cfg(gdb)] - dbg_mem_access_fn, ) .unwrap(); diff --git a/src/hyperlight_host/src/mem/mgr.rs b/src/hyperlight_host/src/mem/mgr.rs index c93f1cac1..8e4558770 100644 --- a/src/hyperlight_host/src/mem/mgr.rs +++ b/src/hyperlight_host/src/mem/mgr.rs @@ -132,7 +132,6 @@ impl ReadonlySharedMemory { pub(crate) use unused_hack::SnapshotSharedMemory; /// A struct that is responsible for laying out and managing the memory /// for a given `Sandbox`. -#[derive(Clone)] pub(crate) struct SandboxMemoryManager { /// Shared memory for the Sandbox pub(crate) shared_mem: SnapshotSharedMemory, diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index 3ced8ab31..f455dffa5 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -86,8 +86,6 @@ pub struct MultiUseSandbox { pub(crate) host_funcs: Arc>, pub(crate) mem_mgr: SandboxMemoryManager, vm: HyperlightVm, - #[cfg(gdb)] - dbg_mem_access_fn: Arc>>, /// If the current state of the sandbox has been captured in a snapshot, /// that snapshot is stored here. pub(crate) snapshot: Option>, @@ -120,15 +118,12 @@ impl MultiUseSandbox { host_funcs: Arc>, mgr: SandboxMemoryManager, vm: HyperlightVm, - #[cfg(gdb)] dbg_mem_access_fn: Arc>>, ) -> MultiUseSandbox { Self { poisoned: false, host_funcs, mem_mgr: mgr, vm, - #[cfg(gdb)] - dbg_mem_access_fn, snapshot: None, pt_root_finder: None, } @@ -281,20 +276,9 @@ impl MultiUseSandbox { }; let peb_addr = RawPtr::from(u64::try_from(hshm.layout.peb_address())?); - #[cfg(gdb)] - let dbg_mem_access_hdl = Arc::new(Mutex::new(hshm.clone())); - // noop for NextAction::Call - vm.initialise( - peb_addr, - seed, - &mut hshm, - &host_funcs, - None, - #[cfg(gdb)] - dbg_mem_access_hdl, - ) - .map_err(crate::hypervisor::hyperlight_vm::HyperlightVmError::Initialize)?; + vm.initialise(peb_addr, seed, &mut hshm, &host_funcs, None) + .map_err(crate::hypervisor::hyperlight_vm::HyperlightVmError::Initialize)?; // If the snapshot was taken from an already-initialized guest // (NextAction::Call), apply the captured special registers so @@ -319,16 +303,7 @@ impl MultiUseSandbox { })?; } - #[cfg(gdb)] - let dbg_mem_wrapper = Arc::new(Mutex::new(hshm.clone())); - - let sbox = MultiUseSandbox::from_uninit( - host_funcs, - hshm, - vm, - #[cfg(gdb)] - dbg_mem_wrapper, - ); + let sbox = MultiUseSandbox::from_uninit(host_funcs, hshm, vm); Ok(sbox) } @@ -921,12 +896,9 @@ impl MultiUseSandbox { self.mem_mgr.write_guest_function_call(buffer)?; - let dispatch_res = self.vm.dispatch_call_from_host( - &mut self.mem_mgr, - &self.host_funcs, - #[cfg(gdb)] - self.dbg_mem_access_fn.clone(), - ); + let dispatch_res = self + .vm + .dispatch_call_from_host(&mut self.mem_mgr, &self.host_funcs); // Convert dispatch errors to HyperlightErrors to maintain backwards compatibility // but first determine if sandbox should be poisoned diff --git a/src/hyperlight_host/src/sandbox/uninitialized_evolve.rs b/src/hyperlight_host/src/sandbox/uninitialized_evolve.rs index ddf407cc3..850f76e1c 100644 --- a/src/hyperlight_host/src/sandbox/uninitialized_evolve.rs +++ b/src/hyperlight_host/src/sandbox/uninitialized_evolve.rs @@ -13,9 +13,6 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */ -#[cfg(gdb)] -use std::sync::{Arc, Mutex}; - use rand::RngExt; use tracing::{Span, instrument}; @@ -63,9 +60,6 @@ pub(super) fn evolve_impl_multi_use(u_sbox: UninitializedSandbox) -> Result Result Date: Thu, 13 Aug 2026 13:24:33 -0700 Subject: [PATCH 2/5] Add unrecoverable sandbox state Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> --- CHANGELOG.md | 3 + src/hyperlight_host/src/error.rs | 5 + .../src/hypervisor/hyperlight_vm/mod.rs | 8 +- .../hypervisor/hyperlight_vm/test_support.rs | 292 ++++++++++ src/hyperlight_host/src/lib.rs | 2 + .../src/sandbox/initialized_multi_use.rs | 526 +++++++++++++++--- src/hyperlight_host/src/sandbox/mod.rs | 2 +- .../src/sandbox/snapshot/file_tests.rs | 2 +- src/hyperlight_host/tests/integration_test.rs | 26 +- .../tests/sandbox_host_tests.rs | 2 +- 10 files changed, 762 insertions(+), 106 deletions(-) create mode 100644 src/hyperlight_host/src/hypervisor/hyperlight_vm/test_support.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 516928815..51c781fae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Prerelease] - Unreleased ### Added +* Add `MultiUseSandbox::status()`, which returns `SandboxStatus` for inspecting sandbox lifecycle state. ### Changed * **Breaking:** Guest MSR state is now saved and restored across snapshots. @@ -13,10 +14,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). resets to a clean default. On KVM the guest may only read or write declared MSRs, on MSHV and WHP this is not enforced. by @ludfjig in https://github.com/hyperlight-dev/hyperlight/pull/991 * **Breaking:** Filesystem paths are now represented using `PathBuf`. `GuestBinary::FilePath` now stores a `PathBuf` instead of a `String`, and `MultiUseSandbox::generate_crashdump_to_dir` accepts `Into` instead of `Into`. Callers passing a `String` to `GuestBinary::FilePath` must convert it using `.into()`. +* Deprecate `MultiUseSandbox::poisoned` in favor of `MultiUseSandbox::status().is_poisoned()`. ### Removed ### Fixed +* Mark a sandbox unrecoverable when snapshot restore fails while updating its VM mappings. * Fix symbol resolution in guest core dumps for sandboxes created from snapshots by @ludfjig in https://github.com/hyperlight-dev/hyperlight/pull/1618 * Reject malformed OCI snapshot metadata and non-regular artifact files during load. * Reset XCR0 during x86 snapshot restore. diff --git a/src/hyperlight_host/src/error.rs b/src/hyperlight_host/src/error.rs index c6738374d..85d56565b 100644 --- a/src/hyperlight_host/src/error.rs +++ b/src/hyperlight_host/src/error.rs @@ -224,6 +224,10 @@ pub enum HyperlightError { #[error("The sandbox was poisoned")] PoisonedSandbox, + /// The sandbox cannot safely perform further operations. + #[error("The sandbox is unrecoverable and must be discarded")] + UnrecoverableSandbox, + /// Raw pointer is less than base address #[error("Raw pointer ({0:?}) was less than the base address ({1})")] RawPointerLessThanBaseAddress(RawPtr, u64), @@ -408,6 +412,7 @@ impl HyperlightError { | HyperlightError::UnexpectedNoOfArguments(_, _) | HyperlightError::UnexpectedParameterValueType(_, _) | HyperlightError::UnexpectedReturnValueType(_, _) + | HyperlightError::UnrecoverableSandbox | HyperlightError::UTF8StringConversionFailure(_) | HyperlightError::VectorCapacityIncorrect(_, _, _) => false, diff --git a/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs b/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs index 93368a335..9e3c6da19 100644 --- a/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs +++ b/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs @@ -19,6 +19,8 @@ mod x86_64; #[cfg(target_arch = "aarch64")] mod aarch64; +#[cfg(all(test, not(gdb), any(kvm, mshv3, target_os = "windows")))] +pub(crate) mod test_support; #[cfg(gdb)] use std::collections::HashMap; use std::str::FromStr; @@ -532,11 +534,12 @@ impl HyperlightVm { let guest_base = crate::mem::layout::SandboxMemoryLayout::BASE_ADDRESS as u64; let rgn = snapshot.mapping_at(guest_base, MemoryRegionType::Snapshot); - if let Some(old_snapshot) = self.snapshot_memory.replace(snapshot) { + if let Some(old_snapshot) = self.snapshot_memory.as_ref() { let old_rgn = old_snapshot.mapping_at(guest_base, MemoryRegionType::Snapshot); self.vm.unmap_memory((self.snapshot_slot, &old_rgn))?; } unsafe { self.vm.map_memory((self.snapshot_slot, &rgn))? }; + self.snapshot_memory = Some(snapshot); Ok(()) } @@ -549,12 +552,13 @@ impl HyperlightVm { let guest_base = hyperlight_common::layout::scratch_base_gpa(scratch.mem_size()); let rgn = scratch.mapping_at(guest_base, MemoryRegionType::Scratch); - if let Some(old_scratch) = self.scratch_memory.replace(scratch) { + if let Some(old_scratch) = self.scratch_memory.as_ref() { let old_base = hyperlight_common::layout::scratch_base_gpa(old_scratch.mem_size()); let old_rgn = old_scratch.mapping_at(old_base, MemoryRegionType::Scratch); self.vm.unmap_memory((self.scratch_slot, &old_rgn))?; } unsafe { self.vm.map_memory((self.scratch_slot, &rgn))? }; + self.scratch_memory = Some(scratch); Ok(()) } diff --git a/src/hyperlight_host/src/hypervisor/hyperlight_vm/test_support.rs b/src/hyperlight_host/src/hypervisor/hyperlight_vm/test_support.rs new file mode 100644 index 000000000..ed4513109 --- /dev/null +++ b/src/hyperlight_host/src/hypervisor/hyperlight_vm/test_support.rs @@ -0,0 +1,292 @@ +/* +Copyright 2025 The Hyperlight Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +use std::collections::VecDeque; + +use super::*; +#[cfg(target_arch = "x86_64")] +use crate::hypervisor::regs::MsrEntry; +use crate::hypervisor::regs::{ + CommonDebugRegs, CommonFpu, CommonRegisters, CommonSpecialRegisters, +}; +use crate::hypervisor::virtual_machine::{CreateVmError, HypervisorError}; + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum VmOperation { + Map(MemoryRegionType), + Unmap(MemoryRegionType), + #[cfg(target_arch = "x86_64")] + SetRegs, + #[cfg(target_arch = "x86_64")] + SetDebugRegs, + #[cfg(target_arch = "x86_64")] + ResetXsave, + #[cfg(target_arch = "x86_64")] + SetSregs, + #[cfg(target_arch = "x86_64")] + SetMsrs, + #[cfg(target_arch = "aarch64")] + ResetVcpu, +} + +#[derive(Clone, Debug)] +pub(crate) struct VmFaultPlan { + operations: Arc>>, +} + +impl VmFaultPlan { + fn new(operations: impl IntoIterator) -> Self { + Self { + operations: Arc::new(Mutex::new(operations.into_iter().collect())), + } + } + + pub(crate) fn is_consumed(&self) -> bool { + self.operations.lock().unwrap().is_empty() + } + + fn should_fail(&self, operation: VmOperation) -> bool { + let mut operations = self.operations.lock().unwrap(); + if operations.front() == Some(&operation) { + operations.pop_front(); + true + } else { + false + } + } +} + +#[derive(Debug)] +struct FaultInjectingVirtualMachine { + inner: Option>, + fault_plan: VmFaultPlan, +} + +impl FaultInjectingVirtualMachine { + fn new( + inner: Box, + operations: impl IntoIterator, + ) -> (Self, VmFaultPlan) { + let fault_plan = VmFaultPlan::new(operations); + ( + Self { + inner: Some(inner), + fault_plan: fault_plan.clone(), + }, + fault_plan, + ) + } + + fn placeholder() -> Self { + Self { + inner: None, + fault_plan: VmFaultPlan::new([]), + } + } + + fn inner(&self) -> &dyn VirtualMachine { + self.inner.as_deref().expect("placeholder VM was used") + } + + fn inner_mut(&mut self) -> &mut dyn VirtualMachine { + self.inner.as_deref_mut().expect("placeholder VM was used") + } + + fn should_fail(&self, operation: VmOperation) -> bool { + self.fault_plan.should_fail(operation) + } + + fn injected_error() -> HypervisorError { + #[cfg(kvm)] + let error = kvm_ioctls::Error::new(libc::EIO); + #[cfg(all(not(kvm), mshv3))] + let error = mshv_ioctls::MshvError::from(libc::EIO); + #[cfg(target_os = "windows")] + let error = windows_result::Error::from_hresult(windows_result::HRESULT::from_win32(5)); + error.into() + } +} + +impl VirtualMachine for FaultInjectingVirtualMachine { + unsafe fn map_memory( + &mut self, + region: (u32, &MemoryRegion), + ) -> std::result::Result<(), MapMemoryError> { + if self.should_fail(VmOperation::Map(region.1.region_type)) { + return Err(MapMemoryError::Hypervisor(Self::injected_error())); + } + // SAFETY: The decorator forwards the caller's preconditions unchanged. + unsafe { self.inner_mut().map_memory(region) } + } + + fn unmap_memory( + &mut self, + region: (u32, &MemoryRegion), + ) -> std::result::Result<(), UnmapMemoryError> { + if self.should_fail(VmOperation::Unmap(region.1.region_type)) { + return Err(UnmapMemoryError::Hypervisor(Self::injected_error())); + } + self.inner_mut().unmap_memory(region) + } + + fn run_vcpu( + &mut self, + #[cfg(feature = "trace_guest")] tc: &mut crate::sandbox::trace::TraceContext, + ) -> std::result::Result { + self.inner_mut().run_vcpu( + #[cfg(feature = "trace_guest")] + tc, + ) + } + + fn regs(&self) -> std::result::Result { + self.inner().regs() + } + + fn set_regs(&self, regs: &CommonRegisters) -> std::result::Result<(), RegisterError> { + #[cfg(target_arch = "x86_64")] + if self.should_fail(VmOperation::SetRegs) { + return Err(RegisterError::SetRegs(Self::injected_error())); + } + self.inner().set_regs(regs) + } + + fn fpu(&self) -> std::result::Result { + self.inner().fpu() + } + + fn set_fpu(&self, fpu: &CommonFpu) -> std::result::Result<(), RegisterError> { + self.inner().set_fpu(fpu) + } + + fn sregs(&self) -> std::result::Result { + self.inner().sregs() + } + + fn set_sregs(&self, sregs: &CommonSpecialRegisters) -> std::result::Result<(), RegisterError> { + #[cfg(target_arch = "x86_64")] + if self.should_fail(VmOperation::SetSregs) { + return Err(RegisterError::SetSregs(Self::injected_error())); + } + self.inner().set_sregs(sregs) + } + + fn debug_regs(&self) -> std::result::Result { + self.inner().debug_regs() + } + + fn set_debug_regs(&self, drs: &CommonDebugRegs) -> std::result::Result<(), RegisterError> { + #[cfg(target_arch = "x86_64")] + if self.should_fail(VmOperation::SetDebugRegs) { + return Err(RegisterError::SetDebugRegs(Self::injected_error())); + } + self.inner().set_debug_regs(drs) + } + + #[cfg(target_arch = "x86_64")] + fn msrs(&self, indices: &[u32]) -> std::result::Result, RegisterError> { + self.inner().msrs(indices) + } + + #[cfg(target_arch = "x86_64")] + fn set_msrs(&self, msrs: &[MsrEntry]) -> std::result::Result<(), RegisterError> { + if self.should_fail(VmOperation::SetMsrs) { + return Err(RegisterError::SetMsrs(Self::injected_error())); + } + self.inner().set_msrs(msrs) + } + + #[cfg(target_arch = "x86_64")] + fn msr_reset_indices( + &self, + guest_msrs: &[u32], + ) -> std::result::Result, CreateVmError> { + self.inner().msr_reset_indices(guest_msrs) + } + + #[cfg(not(target_arch = "aarch64"))] + fn xsave(&self) -> std::result::Result, RegisterError> { + self.inner().xsave() + } + + #[cfg(not(target_arch = "aarch64"))] + fn reset_xsave(&self) -> std::result::Result<(), RegisterError> { + #[cfg(target_arch = "x86_64")] + if self.should_fail(VmOperation::ResetXsave) { + return Err(RegisterError::SetXsave(Self::injected_error())); + } + self.inner().reset_xsave() + } + + #[cfg(not(target_arch = "aarch64"))] + fn set_xsave(&self, xsave: &[u32]) -> std::result::Result<(), RegisterError> { + self.inner().set_xsave(xsave) + } + + #[cfg(all(test, target_arch = "x86_64"))] + fn xcr0(&self) -> std::result::Result { + self.inner().xcr0() + } + + #[cfg(target_arch = "x86_64")] + fn set_xcr0(&self, value: u64) -> std::result::Result<(), RegisterError> { + self.inner().set_xcr0(value) + } + + #[cfg(target_arch = "aarch64")] + fn can_reset_vcpu(&self) -> bool { + self.inner().can_reset_vcpu() + } + + #[cfg(target_arch = "aarch64")] + fn reset_vcpu(&mut self) -> std::result::Result<(), ResetVcpuError> { + if self.should_fail(VmOperation::ResetVcpu) { + return Err(ResetVcpuError::Hypervisor(Self::injected_error())); + } + self.inner_mut().reset_vcpu() + } + + #[cfg(target_os = "windows")] + fn partition_handle(&self) -> windows::Win32::System::Hypervisor::WHV_PARTITION_HANDLE { + self.inner().partition_handle() + } +} + +impl HyperlightVm { + pub(crate) fn inject_vm_faults( + &mut self, + operations: impl IntoIterator, + ) -> VmFaultPlan { + let placeholder = Box::new(FaultInjectingVirtualMachine::placeholder()); + let inner = std::mem::replace(&mut self.vm, placeholder); + let (vm, fault_plan) = FaultInjectingVirtualMachine::new(inner, operations); + self.vm = Box::new(vm); + fault_plan + } + + #[allow(clippy::type_complexity, reason = "test-only mapping state")] + pub(crate) fn base_mapping_state(&self) -> (Option<(usize, usize)>, Option<(usize, usize)>) { + let snapshot = self + .snapshot_memory + .as_ref() + .map(|memory| (memory.base_addr(), memory.mem_size())); + let scratch = self + .scratch_memory + .as_ref() + .map(|memory| (memory.base_addr(), memory.mem_size())); + (snapshot, scratch) + } +} diff --git a/src/hyperlight_host/src/lib.rs b/src/hyperlight_host/src/lib.rs index 162d0420f..cae18b1bc 100644 --- a/src/hyperlight_host/src/lib.rs +++ b/src/hyperlight_host/src/lib.rs @@ -89,6 +89,8 @@ pub use hypervisor::virtual_machine::is_hypervisor_present; /// A sandbox that can call be used to make multiple calls to guest functions, /// and otherwise reused multiple times pub use sandbox::MultiUseSandbox; +/// The lifecycle state of a [`MultiUseSandbox`]. +pub use sandbox::SandboxStatus; /// The re-export for the `UninitializedSandbox` type pub use sandbox::UninitializedSandbox; /// A collection of host functions that can be supplied to a sandbox diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index f455dffa5..781b2e8d1 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -42,6 +42,34 @@ use crate::metrics::{ }; use crate::{HyperlightError, Result, log_then_return}; +/// The lifecycle state of a [`MultiUseSandbox`]. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum SandboxStatus { + /// The sandbox can execute guest operations. + Ready, + /// The sandbox requires a successful restore before further use. + Poisoned, + /// The sandbox must be discarded. + Unrecoverable, +} + +impl SandboxStatus { + /// Returns whether the sandbox can execute guest operations. + pub const fn is_ready(self) -> bool { + matches!(self, Self::Ready) + } + + /// Returns whether the sandbox requires a successful restore. + pub const fn is_poisoned(self) -> bool { + matches!(self, Self::Poisoned) + } + + /// Returns whether the sandbox must be discarded. + pub const fn is_unrecoverable(self) -> bool { + matches!(self, Self::Unrecoverable) + } +} + /// A fully initialized sandbox that can execute guest functions multiple times. /// /// Guest functions can be called repeatedly while maintaining state between calls. @@ -78,11 +106,12 @@ use crate::{HyperlightError, Result, log_then_return}; /// ### Recovery /// /// Use [`restore()`](Self::restore) with a snapshot taken before poisoning occurred. -/// This is the **only safe way** to recover - it completely replaces all memory state, +/// This completely replaces all memory state, /// eliminating any inconsistencies. See [`restore()`](Self::restore) for details. +/// A sandbox becomes [`SandboxStatus::Unrecoverable`] when restore cannot establish +/// a usable VM mapping state. It must be discarded. pub struct MultiUseSandbox { - /// Whether this sandbox is poisoned - poisoned: bool, + status: SandboxStatus, pub(crate) host_funcs: Arc>, pub(crate) mem_mgr: SandboxMemoryManager, vm: HyperlightVm, @@ -108,6 +137,20 @@ pub struct MultiUseSandbox { pub type PtRootFinder = Box Vec + Send>; impl MultiUseSandbox { + fn ensure_usable(&self) -> Result<()> { + match self.status { + SandboxStatus::Ready => Ok(()), + SandboxStatus::Poisoned => Err(HyperlightError::PoisonedSandbox), + SandboxStatus::Unrecoverable => Err(HyperlightError::UnrecoverableSandbox), + } + } + + fn poison(&mut self) { + if self.status.is_ready() { + self.status = SandboxStatus::Poisoned; + } + } + /// Move an `UninitializedSandbox` into a new `MultiUseSandbox` instance. /// /// This function is not equivalent to doing an `evolve` from uninitialized @@ -120,7 +163,7 @@ impl MultiUseSandbox { vm: HyperlightVm, ) -> MultiUseSandbox { Self { - poisoned: false, + status: SandboxStatus::Ready, host_funcs, mem_mgr: mgr, vm, @@ -346,9 +389,7 @@ impl MultiUseSandbox { /// ``` #[instrument(err(Debug), skip_all, parent = Span::current())] pub fn snapshot(&mut self) -> Result> { - if self.poisoned { - return Err(crate::HyperlightError::PoisonedSandbox); - } + self.ensure_usable()?; if let Some(snapshot) = &self.snapshot { return Ok(snapshot.clone()); @@ -403,6 +444,21 @@ impl MultiUseSandbox { Ok(snapshot) } + fn restore_memory_and_mappings(&mut self, snapshot: &Snapshot) -> Result<()> { + let (snapshot_mem, scratch_mem) = self.mem_mgr.restore_snapshot(snapshot)?; + if let Some(snapshot_mem) = snapshot_mem { + self.vm + .update_snapshot_mapping(snapshot_mem) + .map_err(HyperlightVmError::UpdateRegion)?; + } + if let Some(scratch_mem) = scratch_mem { + self.vm + .update_scratch_mapping(scratch_mem) + .map_err(HyperlightVmError::UpdateRegion)?; + } + Ok(()) + } + /// Restores the sandbox's memory to a previously captured snapshot state. /// /// The snapshot's memory layout must be structurally compatible @@ -425,6 +481,9 @@ impl MultiUseSandbox { /// declare every MSR the snapshot saved, or the restore poisons with an MSR /// mismatch. /// + /// A failure while updating the sandbox's base VM mappings leaves the sandbox + /// [`Unrecoverable`](SandboxStatus::Unrecoverable). It must be discarded. + /// /// ## Poison State Recovery /// /// This method automatically clears any poison state when successful. This is safe because: @@ -482,10 +541,10 @@ impl MultiUseSandbox { /// // This might poison the sandbox (guest not run to completion) /// let result = sandbox.call::<()>("guest_panic", ()); /// if result.is_err() { - /// if sandbox.poisoned() { + /// if sandbox.status().is_poisoned() { /// // Restore from snapshot to clear poison /// sandbox.restore(snapshot.clone())?; - /// assert!(!sandbox.poisoned()); + /// assert!(sandbox.status().is_ready()); /// /// // Sandbox is now usable again /// sandbox.call::("Echo", "hello".to_string())?; @@ -496,6 +555,10 @@ impl MultiUseSandbox { /// ``` #[instrument(err(Debug), skip_all, parent = Span::current())] pub fn restore(&mut self, snapshot: Arc) -> Result<()> { + if self.status.is_unrecoverable() { + return Err(HyperlightError::UnrecoverableSandbox); + } + // Currently, we do not try to optimise restore to the // most-current snapshot. This is because the most-current // snapshot, while it must have identical virtual memory @@ -527,36 +590,28 @@ impl MultiUseSandbox { snapshot.validate_compatibility(&self.mem_mgr.layout, &host_funcs)?; } - let (gsnapshot, gscratch) = self.mem_mgr.restore_snapshot(&snapshot)?; - if let Some(gsnapshot) = gsnapshot { - self.vm - .update_snapshot_mapping(gsnapshot) - .map_err(|e| HyperlightError::HyperlightVmError(e.into()))?; - } - if let Some(gscratch) = gscratch { - self.vm - .update_scratch_mapping(gscratch) - .map_err(|e| HyperlightError::HyperlightVmError(e.into()))?; - } - let sregs = snapshot.sregs().ok_or_else(|| { HyperlightError::Error("snapshot from running sandbox should have sregs".to_string()) })?; - // TODO (ludfjig): Go through the rest of possible errors in this `MultiUseSandbox::restore` function - // and determine if they should also poison the sandbox. + + if let Err(error) = self.restore_memory_and_mappings(&snapshot) { + self.status = SandboxStatus::Unrecoverable; + self.snapshot = None; + return Err(error); + } + + self.status = SandboxStatus::Poisoned; + self.snapshot = None; + self.vm .reset_vcpu(snapshot.root_pt_gpa(), sregs) - .map_err(|e| { - self.poisoned = true; - HyperlightVmError::Restore(e) - })?; + .map_err(HyperlightVmError::Restore)?; // Restore captured MSR state. #[cfg(target_arch = "x86_64")] - self.vm.restore_msrs(snapshot.msrs()).map_err(|e| { - self.poisoned = true; - HyperlightVmError::Restore(e) - })?; + self.vm + .restore_msrs(snapshot.msrs()) + .map_err(HyperlightVmError::Restore)?; self.vm.set_stack_top(snapshot.stack_top_gva()); self.vm.set_next_action(snapshot.next_action()); @@ -585,7 +640,7 @@ impl MultiUseSandbox { // - All leaked heap allocations (memory is restored to snapshot state) // - All corrupted data structures (overwritten with consistent snapshot data) // - All inconsistent global state (reset to snapshot values) - self.poisoned = false; + self.status = SandboxStatus::Ready; Ok(()) } @@ -637,9 +692,7 @@ impl MultiUseSandbox { func_name: &str, args: impl ParameterTuple, ) -> Result { - if self.poisoned { - return Err(crate::HyperlightError::PoisonedSandbox); - } + self.ensure_usable()?; let snapshot = self.snapshot()?; let res = self.call(func_name, args); self.restore(snapshot)?; @@ -660,7 +713,7 @@ impl MultiUseSandbox { /// /// If this method returns an error, the sandbox may be poisoned if the guest was not run /// to completion (due to panic, abort, memory violation, stack/heap exhaustion, or forced - /// termination). Use [`poisoned()`](Self::poisoned) to check the poison state and + /// termination). Use [`status()`](Self::status) to check the sandbox state and /// [`restore()`](Self::restore) to recover if needed. /// /// If this method returns `Ok`, the sandbox is guaranteed to **not** be poisoned - the guest @@ -714,7 +767,7 @@ impl MultiUseSandbox { /// if let Err(e) = result { /// eprintln!("Guest function failed: {}", e); /// - /// if sandbox.poisoned() { + /// if sandbox.status().is_poisoned() { /// eprintln!("Sandbox was poisoned, restoring from snapshot"); /// sandbox.restore(snapshot.clone())?; /// } @@ -728,9 +781,7 @@ impl MultiUseSandbox { func_name: &str, args: impl ParameterTuple, ) -> Result { - if self.poisoned { - return Err(crate::HyperlightError::PoisonedSandbox); - } + self.ensure_usable()?; // Reset snapshot since we are mutating the sandbox state self.snapshot = None; maybe_time_and_emit_guest_call(func_name, || { @@ -763,9 +814,7 @@ impl MultiUseSandbox { /// for the lifetime of `self`. #[instrument(err(Debug), skip(self, rgn), parent = Span::current())] pub unsafe fn map_region(&mut self, rgn: &MemoryRegion) -> Result<()> { - if self.poisoned { - return Err(crate::HyperlightError::PoisonedSandbox); - } + self.ensure_usable()?; if rgn.flags.contains(MemoryRegionFlags::WRITE) { // TODO: Implement support for writable mappings, which // need to be registered with the memory manager so that @@ -789,9 +838,7 @@ impl MultiUseSandbox { /// is currently poisoned. Use [`restore()`](Self::restore) to recover from a poisoned state. #[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); - } + self.ensure_usable()?; // Phase 1: host-side OS work (open file, create mapping) let mut prepared = prepare_file_cow(file_path, guest_base)?; @@ -857,9 +904,7 @@ impl MultiUseSandbox { ret_type: ReturnType, args: Vec, ) -> Result { - if self.poisoned { - return Err(crate::HyperlightError::PoisonedSandbox); - } + self.ensure_usable()?; // Reset snapshot since we are mutating the sandbox state self.snapshot = None; maybe_time_and_emit_guest_call(func_name, || { @@ -873,9 +918,7 @@ impl MultiUseSandbox { return_type: ReturnType, args: Vec, ) -> Result { - if self.poisoned { - return Err(crate::HyperlightError::PoisonedSandbox); - } + self.ensure_usable()?; // ===== KILL() TIMING POINT 1 ===== // Clear any stale cancellation from a previous guest function call or if kill() was called too early. // Any kill() that completed (even partially) BEFORE this line has NO effect on this call. @@ -904,7 +947,9 @@ impl MultiUseSandbox { // but first determine if sandbox should be poisoned if let Err(e) = dispatch_res { let (error, should_poison) = e.promote(); - self.poisoned |= should_poison; + if should_poison { + self.poison(); + } return Err(error); } @@ -939,7 +984,9 @@ impl MultiUseSandbox { self.mem_mgr.clear_io_buffers(); // Determine if we should poison the sandbox. - self.poisoned |= e.is_poison_error(); + if e.is_poison_error() { + self.poison(); + } } // Note: clear_call_active() is automatically called when _guard is dropped here @@ -1037,10 +1084,9 @@ impl MultiUseSandbox { ) } - /// Returns whether the sandbox is currently poisoned. + /// Returns whether the sandbox is poisoned. /// - /// A poisoned sandbox is in an inconsistent state due to the guest not running to completion. - /// All operations will be rejected until the sandbox is restored from a non-poisoned snapshot. + /// Use [`status()`](Self::status) to distinguish every lifecycle state. /// /// ## Causes of Poisoning /// @@ -1059,22 +1105,27 @@ impl MultiUseSandbox { /// # Examples /// /// ```no_run - /// # use hyperlight_host::{MultiUseSandbox, UninitializedSandbox, GuestBinary}; + /// # use hyperlight_host::{MultiUseSandbox, UninitializedSandbox, GuestBinary, SandboxStatus}; /// # fn example() -> Result<(), Box> { /// let mut sandbox: MultiUseSandbox = UninitializedSandbox::new( /// GuestBinary::FilePath("guest.bin".into()), /// None /// )?.evolve()?; /// - /// // Check if sandbox is poisoned - /// if sandbox.poisoned() { - /// println!("Sandbox is poisoned and needs attention"); + /// if sandbox.status().is_poisoned() { + /// println!("Sandbox is poisoned"); /// } /// # Ok(()) /// # } /// ``` + #[deprecated(since = "0.16.0", note = "use status().is_poisoned()")] pub fn poisoned(&self) -> bool { - self.poisoned + self.status.is_poisoned() + } + + /// Returns whether the sandbox is ready, poisoned, or unrecoverable. + pub fn status(&self) -> SandboxStatus { + self.status } } @@ -1084,9 +1135,7 @@ impl Callable for MultiUseSandbox { func_name: &str, args: impl ParameterTuple, ) -> Result { - if self.poisoned { - return Err(crate::HyperlightError::PoisonedSandbox); - } + self.ensure_usable()?; self.call(func_name, args) } } @@ -1148,10 +1197,29 @@ mod tests { use hyperlight_testing::sandbox_sizes::{LARGE_HEAP_SIZE, MEDIUM_HEAP_SIZE, SMALL_HEAP_SIZE}; use hyperlight_testing::simple_guest_as_pathbuf; + #[cfg(not(gdb))] + use crate::hypervisor::hyperlight_vm::test_support::VmOperation; use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags, MemoryRegionType}; use crate::mem::shared_mem::{ExclusiveSharedMemory, GuestSharedMemory, SharedMemory as _}; use crate::sandbox::SandboxConfiguration; - use crate::{GuestBinary, HyperlightError, MultiUseSandbox, Result, UninitializedSandbox}; + use crate::{ + GuestBinary, HyperlightError, MultiUseSandbox, Result, SandboxStatus, UninitializedSandbox, + }; + + #[test] + fn sandbox_status_predicates() { + assert!(SandboxStatus::Ready.is_ready()); + assert!(!SandboxStatus::Ready.is_poisoned()); + assert!(!SandboxStatus::Ready.is_unrecoverable()); + + assert!(!SandboxStatus::Poisoned.is_ready()); + assert!(SandboxStatus::Poisoned.is_poisoned()); + assert!(!SandboxStatus::Poisoned.is_unrecoverable()); + + assert!(!SandboxStatus::Unrecoverable.is_ready()); + assert!(!SandboxStatus::Unrecoverable.is_poisoned()); + assert!(SandboxStatus::Unrecoverable.is_unrecoverable()); + } #[test] fn poison() { @@ -1170,7 +1238,7 @@ mod tests { assert!( matches!(res, HyperlightError::GuestAborted(code, context) if code == ErrorCode::UnknownError as u8 && context.contains("hello")) ); - assert!(sbox.poisoned()); + assert!(sbox.status().is_poisoned()); // guest calls should fail when poisoned let res = sbox @@ -1180,7 +1248,7 @@ mod tests { // snapshot should fail when poisoned if let Err(e) = sbox.snapshot() { - assert!(sbox.poisoned()); + assert!(sbox.status().is_poisoned()); assert!(matches!(e, HyperlightError::PoisonedSandbox)); } else { panic!("Snapshot should fail"); @@ -1212,12 +1280,12 @@ mod tests { // restore to non-poisoned snapshot should work and clear poison sbox.restore(snapshot.clone()).unwrap(); - assert!(!sbox.poisoned()); + assert_eq!(sbox.status(), SandboxStatus::Ready); // guest calls should work again after restore let res = sbox.call::("Echo", "hello2".to_string()).unwrap(); assert_eq!(res, "hello2".to_string()); - assert!(!sbox.poisoned()); + assert_eq!(sbox.status(), SandboxStatus::Ready); // re-poison on purpose let res = sbox @@ -1226,16 +1294,16 @@ mod tests { assert!( matches!(res, HyperlightError::GuestAborted(code, context) if code == ErrorCode::UnknownError as u8 && context.contains("hello")) ); - assert!(sbox.poisoned()); + assert!(sbox.status().is_poisoned()); // restore to non-poisoned snapshot should work again sbox.restore(snapshot.clone()).unwrap(); - assert!(!sbox.poisoned()); + assert_eq!(sbox.status(), SandboxStatus::Ready); // guest calls should work again let res = sbox.call::("Echo", "hello3".to_string()).unwrap(); assert_eq!(res, "hello3".to_string()); - assert!(!sbox.poisoned()); + assert_eq!(sbox.status(), SandboxStatus::Ready); // snapshot should work again let _ = sbox.snapshot().unwrap(); @@ -1710,6 +1778,281 @@ mod tests { assert_eq!(sandbox2.call::("GetStatic", ()).unwrap(), 42); } + #[test] + #[cfg(not(gdb))] + fn snapshot_restore_keeps_current_base_mappings() { + let path = simple_guest_as_pathbuf(); + let mut sandbox = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + let snapshot = sandbox.snapshot().unwrap(); + sandbox.restore(snapshot.clone()).unwrap(); + sandbox.call::("AddToStatic", 42i32).unwrap(); + let mappings = sandbox.vm.base_mapping_state(); + let fault_plan = sandbox + .vm + .inject_vm_faults([VmOperation::Unmap(MemoryRegionType::Snapshot)]); + + sandbox.restore(snapshot).unwrap(); + + assert_eq!(sandbox.status(), SandboxStatus::Ready); + assert_eq!(sandbox.vm.base_mapping_state(), mappings); + assert!(!fault_plan.is_consumed()); + assert_eq!(sandbox.call::("GetStatic", ()).unwrap(), 0); + } + + #[test] + #[cfg(not(gdb))] + fn snapshot_restore_mapping_failure_is_unrecoverable() { + let path = simple_guest_as_pathbuf(); + let mut source = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + source.call::("AddToStatic", 42i32).unwrap(); + let snapshot = source.snapshot().unwrap(); + + let path = simple_guest_as_pathbuf(); + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + let mappings = target.vm.base_mapping_state(); + let fault_plan = target + .vm + .inject_vm_faults([VmOperation::Map(MemoryRegionType::Snapshot)]); + + let error = target.restore(snapshot.clone()).unwrap_err(); + assert!(matches!(error, HyperlightError::HyperlightVmError(_))); + assert_eq!(target.status(), SandboxStatus::Unrecoverable); + assert_eq!(target.vm.base_mapping_state(), mappings); + assert!(fault_plan.is_consumed()); + + assert!(matches!( + target.restore(snapshot), + Err(HyperlightError::UnrecoverableSandbox) + )); + assert!(matches!( + target.call::("GetStatic", ()), + Err(HyperlightError::UnrecoverableSandbox) + )); + assert!(matches!( + target.snapshot(), + Err(HyperlightError::UnrecoverableSandbox) + )); + + let map_mem = allocate_guest_memory(); + let region = region_for_memory(&map_mem, 0x200000000_usize, MemoryRegionFlags::READ); + assert!(matches!( + unsafe { target.map_region(®ion) }, + Err(HyperlightError::UnrecoverableSandbox) + )); + } + + #[test] + #[cfg(not(gdb))] + fn snapshot_restore_unmapping_failure_is_unrecoverable() { + let path = simple_guest_as_pathbuf(); + let mut source = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + source.call::("AddToStatic", 42i32).unwrap(); + let snapshot = source.snapshot().unwrap(); + + let path = simple_guest_as_pathbuf(); + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + let mappings = target.vm.base_mapping_state(); + let fault_plan = target + .vm + .inject_vm_faults([VmOperation::Unmap(MemoryRegionType::Snapshot)]); + + let error = target.restore(snapshot).unwrap_err(); + assert!(matches!(error, HyperlightError::HyperlightVmError(_))); + assert_eq!(target.status(), SandboxStatus::Unrecoverable); + assert_eq!(target.vm.base_mapping_state(), mappings); + assert!(fault_plan.is_consumed()); + } + + #[test] + #[cfg(not(gdb))] + fn snapshot_restore_dynamic_unmapping_failure_is_recoverable() { + let path = simple_guest_as_pathbuf(); + let mut source = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + source.call::("AddToStatic", 42i32).unwrap(); + let snapshot = source.snapshot().unwrap(); + + let path = simple_guest_as_pathbuf(); + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + let map_mem = allocate_guest_memory(); + let region = region_for_memory(&map_mem, 0x200000000_usize, MemoryRegionFlags::READ); + unsafe { target.map_region(®ion).unwrap() }; + let fault_plan = target + .vm + .inject_vm_faults([VmOperation::Unmap(MemoryRegionType::Heap)]); + + let error = target.restore(snapshot.clone()).unwrap_err(); + assert!(matches!(error, HyperlightError::HyperlightVmError(_))); + assert!(target.status().is_poisoned()); + assert_eq!(target.vm.get_mapped_regions().count(), 1); + assert!(fault_plan.is_consumed()); + + target.restore(snapshot).unwrap(); + assert_eq!(target.status(), SandboxStatus::Ready); + assert_eq!(target.vm.get_mapped_regions().count(), 0); + assert_eq!(target.call::("GetStatic", ()).unwrap(), 42); + } + + #[test] + #[cfg(not(gdb))] + fn snapshot_restore_partial_dynamic_unmapping_failure_is_recoverable() { + let path = simple_guest_as_pathbuf(); + let mut source = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + source.call::("AddToStatic", 42i32).unwrap(); + let snapshot = source.snapshot().unwrap(); + + let path = simple_guest_as_pathbuf(); + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + let first_mem = allocate_guest_memory(); + let first_region = + region_for_memory(&first_mem, 0x200000000_usize, MemoryRegionFlags::READ); + let second_mem = allocate_guest_memory(); + let mut second_region = + region_for_memory(&second_mem, 0x300000000_usize, MemoryRegionFlags::READ); + second_region.region_type = MemoryRegionType::MappedFile; + unsafe { + target.map_region(&first_region).unwrap(); + target.map_region(&second_region).unwrap(); + } + let fault_plan = target + .vm + .inject_vm_faults([VmOperation::Unmap(MemoryRegionType::MappedFile)]); + + let error = target.restore(snapshot.clone()).unwrap_err(); + assert!(matches!(error, HyperlightError::HyperlightVmError(_))); + assert!(target.status().is_poisoned()); + assert_eq!( + target.vm.get_mapped_regions().collect::>(), + vec![&second_region] + ); + assert!(fault_plan.is_consumed()); + + target.restore(snapshot).unwrap(); + assert_eq!(target.status(), SandboxStatus::Ready); + assert_eq!(target.vm.get_mapped_regions().count(), 0); + assert_eq!(target.call::("GetStatic", ()).unwrap(), 42); + } + + #[test] + #[cfg(not(gdb))] + fn snapshot_restore_vcpu_reset_failure_is_recoverable() { + let path = simple_guest_as_pathbuf(); + let mut source = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + source.call::("AddToStatic", 42i32).unwrap(); + let snapshot = source.snapshot().unwrap(); + + #[cfg(target_arch = "x86_64")] + let reset_operations = [ + VmOperation::SetRegs, + VmOperation::SetDebugRegs, + VmOperation::ResetXsave, + VmOperation::SetSregs, + ]; + #[cfg(target_arch = "aarch64")] + let reset_operations = [VmOperation::ResetVcpu]; + + for reset_operation in reset_operations { + let path = simple_guest_as_pathbuf(); + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + let fault_plan = target.vm.inject_vm_faults([reset_operation]); + + let error = target.restore(snapshot.clone()).unwrap_err(); + assert!(matches!(error, HyperlightError::HyperlightVmError(_))); + assert!(target.status().is_poisoned()); + assert!(fault_plan.is_consumed()); + assert_eq!( + target.vm.base_mapping_state(), + ( + Some(( + target.mem_mgr.shared_mem.base_addr(), + target.mem_mgr.shared_mem.mem_size(), + )), + Some(( + target.mem_mgr.scratch_mem.base_addr(), + target.mem_mgr.scratch_mem.mem_size(), + )), + ) + ); + + target.restore(snapshot.clone()).unwrap(); + assert_eq!(target.status(), SandboxStatus::Ready); + assert_eq!(target.call::("GetStatic", ()).unwrap(), 42); + } + } + + #[test] + #[cfg(all(target_arch = "x86_64", not(gdb)))] + fn snapshot_restore_msr_failure_is_recoverable() { + let path = simple_guest_as_pathbuf(); + let mut source = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + source.call::("AddToStatic", 42i32).unwrap(); + let snapshot = source.snapshot().unwrap(); + + let path = simple_guest_as_pathbuf(); + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + let fault_plan = target.vm.inject_vm_faults([VmOperation::SetMsrs]); + + let error = target.restore(snapshot.clone()).unwrap_err(); + assert!(matches!(error, HyperlightError::HyperlightVmError(_))); + assert!(target.status().is_poisoned()); + assert!(fault_plan.is_consumed()); + assert_eq!( + target.vm.base_mapping_state(), + ( + Some(( + target.mem_mgr.shared_mem.base_addr(), + target.mem_mgr.shared_mem.mem_size(), + )), + Some(( + target.mem_mgr.scratch_mem.base_addr(), + target.mem_mgr.scratch_mem.mem_size(), + )), + ) + ); + + target.restore(snapshot).unwrap(); + assert_eq!(target.status(), SandboxStatus::Ready); + assert_eq!(target.call::("GetStatic", ()).unwrap(), 42); + } + #[test] fn snapshot_restore_rejects_incompatible_layout() { let mut sandbox = { @@ -1724,6 +2067,7 @@ mod tests { let path = simple_guest_as_pathbuf(); let mut cfg = SandboxConfiguration::default(); cfg.set_heap_size(0x20_000); + cfg.set_scratch_size(0x60_000); let u_sbox = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(cfg)).unwrap(); u_sbox.evolve().unwrap() }; @@ -2194,7 +2538,7 @@ mod tests { let _ = sbox .call::<()>("guest_panic", "hello".to_string()) .unwrap_err(); - assert!(sbox.poisoned()); + assert!(sbox.status().is_poisoned()); // map_file_cow should fail with PoisonedSandbox let err = sbox.map_file_cow(&path, 0x1_0000_0000).unwrap_err(); @@ -2202,7 +2546,7 @@ mod tests { // Restore and verify map_file_cow works again sbox.restore(snapshot).unwrap(); - assert!(!sbox.poisoned()); + assert_eq!(sbox.status(), SandboxStatus::Ready); let result = sbox.map_file_cow(&path, 0x1_0000_0000); assert!(result.is_ok()); @@ -3053,7 +3397,7 @@ mod tests { .restore(snapshot.clone()) .expect_err("restore must reject an unrestorable snapshot MSR"); assert_snapshot_msr_index_invalid(&err); - assert!(target.poisoned()); + assert!(target.status().is_poisoned()); assert!(matches!( target.call::("Echo", "hi".to_string()), Err(HyperlightError::PoisonedSandbox) @@ -3114,16 +3458,16 @@ mod tests { matches!(result, Err(HyperlightError::GuestAborted(_, _))), "guest enabled x2APIC through APIC_BASE: {result:?}" ); - assert!(sandbox.poisoned()); + assert!(sandbox.status().is_poisoned()); sandbox.restore(snapshot).unwrap(); - assert!(!sandbox.poisoned()); + assert!(!sandbox.status().is_poisoned()); let result = sandbox.call::<()>("WriteMSR", (MSR_X2APIC_BASE, 1u64)); assert!( matches!(result, Err(HyperlightError::GuestAborted(_, _))), "x2APIC MSR access succeeded after restore: {result:?}" ); - assert!(sandbox.poisoned()); + assert!(sandbox.status().is_poisoned()); } #[test] @@ -3154,7 +3498,7 @@ mod tests { msr_index, result ); - assert!(sbox.poisoned()); + assert!(sbox.status().is_poisoned()); sbox.restore(snapshot.clone()).unwrap(); @@ -3165,7 +3509,7 @@ mod tests { msr_index, result ); - assert!(sbox.poisoned()); + assert!(sbox.status().is_poisoned()); } #[test] @@ -3231,7 +3575,7 @@ mod tests { matches!(result, Err(HyperlightError::GuestAborted(_, _))), "guest entered VMX operation via CR4.VMXE: {result:?}" ); - assert!(sandbox.poisoned()); + assert!(sandbox.status().is_poisoned()); } /// Executing a VM-enter (`VMLAUNCH`) in the guest faults. The guest is @@ -3252,7 +3596,7 @@ mod tests { matches!(result, Err(HyperlightError::GuestAborted(_, _))), "guest executed VMLAUNCH without faulting: {result:?}" ); - assert!(sandbox.poisoned()); + assert!(sandbox.status().is_poisoned()); } /// x2APIC is denied at the MSR level and Hyperlight keeps the APIC in @@ -3417,7 +3761,7 @@ mod tests { "WRMSR 0x{msr_index:X}: expected direct #GP, got: {result:?}" ); assert!( - sbox.poisoned(), + sbox.status().is_poisoned(), "sandbox should be poisoned after a denied WRMSR to 0x{msr_index:X}" ); } @@ -3546,7 +3890,10 @@ mod tests { let original: u64 = match sbox.call("ReadMSR", index) { Ok(value) => value, Err(_) => { - assert!(sbox.poisoned(), "0x{index:X}: fault did not poison sandbox"); + assert!( + sbox.status().is_poisoned(), + "0x{index:X}: fault did not poison sandbox" + ); sbox.restore(baseline).unwrap(); return; } @@ -3559,7 +3906,10 @@ mod tests { continue; } if sbox.call::<()>("WriteMSR", (index, candidate)).is_err() { - assert!(sbox.poisoned(), "0x{index:X}: fault did not poison sandbox"); + assert!( + sbox.status().is_poisoned(), + "0x{index:X}: fault did not poison sandbox" + ); sbox.restore(baseline.clone()).unwrap(); continue; } @@ -3692,7 +4042,7 @@ mod tests { Ok(v) => v, Err(_) => { assert!( - sbox.poisoned(), + sbox.status().is_poisoned(), "0x{msr:X}: a faulting RDMSR should poison the sandbox" ); sbox.restore(baseline).unwrap(); @@ -3706,7 +4056,7 @@ mod tests { if sbox.call::<()>("WriteMSR", (msr, sentinel)).is_err() { assert!( - sbox.poisoned(), + sbox.status().is_poisoned(), "0x{msr:X}: a faulting WRMSR should poison the sandbox" ); sbox.restore(baseline).unwrap(); diff --git a/src/hyperlight_host/src/sandbox/mod.rs b/src/hyperlight_host/src/sandbox/mod.rs index 822b1e388..2e0fe5923 100644 --- a/src/hyperlight_host/src/sandbox/mod.rs +++ b/src/hyperlight_host/src/sandbox/mod.rs @@ -46,7 +46,7 @@ pub use callable::Callable; /// Re-export for `SandboxConfiguration` type pub use config::SandboxConfiguration; /// Re-export for the `MultiUseSandbox` type -pub use initialized_multi_use::{MultiUseSandbox, PtRootFinder}; +pub use initialized_multi_use::{MultiUseSandbox, PtRootFinder, SandboxStatus}; /// Re-export for `GuestBinary` type pub use uninitialized::GuestBinary; /// Re-export for `UninitializedSandbox` type diff --git a/src/hyperlight_host/src/sandbox/snapshot/file_tests.rs b/src/hyperlight_host/src/sandbox/snapshot/file_tests.rs index 4e0604c86..9787debe6 100644 --- a/src/hyperlight_host/src/sandbox/snapshot/file_tests.rs +++ b/src/hyperlight_host/src/sandbox/snapshot/file_tests.rs @@ -363,7 +363,7 @@ fn disk_snapshot_non_superset_guest_msrs_rejected() { format!("{err:?}").contains("InvalidSnapshotMsrIndex"), "expected an MSR reset-set mismatch, got: {err:?}" ); - assert!(target.poisoned()); + assert!(target.status().is_poisoned()); } #[test] diff --git a/src/hyperlight_host/tests/integration_test.rs b/src/hyperlight_host/tests/integration_test.rs index 24db9134a..ecd964516 100644 --- a/src/hyperlight_host/tests/integration_test.rs +++ b/src/hyperlight_host/tests/integration_test.rs @@ -21,7 +21,7 @@ use std::time::Duration; use hyperlight_common::flatbuffer_wrappers::guest_error::ErrorCode; use hyperlight_common::log_level::GuestLogFilter; use hyperlight_host::sandbox::SandboxConfiguration; -use hyperlight_host::{HyperlightError, MultiUseSandbox}; +use hyperlight_host::{HyperlightError, MultiUseSandbox, SandboxStatus}; use hyperlight_testing::simplelogger::{LOGGER, SimpleLogger}; use serial_test::serial; use tracing_core::LevelFilter; @@ -65,11 +65,11 @@ fn interrupt_host_call() { matches!(&result, HyperlightError::ExecutionCanceledByHost()), "unexpected error: {result:?}" ); - assert!(sandbox.poisoned()); + assert!(sandbox.status().is_poisoned()); // Restore from snapshot to clear poison sandbox.restore(snapshot.clone()).unwrap(); - assert!(!sandbox.poisoned()); + assert_eq!(sandbox.status(), SandboxStatus::Ready); thread.join().unwrap(); }); @@ -99,11 +99,11 @@ fn interrupt_in_progress_guest_call() { matches!(&res, HyperlightError::ExecutionCanceledByHost()), "unexpected error: {res:?}" ); - assert!(sbox1.poisoned()); + assert!(sbox1.status().is_poisoned()); // Restore from snapshot to clear poison sbox1.restore(snapshot.clone()).unwrap(); - assert!(!sbox1.poisoned()); + assert_eq!(sbox1.status(), SandboxStatus::Ready); barrier.wait(); // Make sure we can still call guest functions after the VM was interrupted @@ -196,7 +196,7 @@ fn interrupt_same_thread() { Ok(_) | Err(HyperlightError::ExecutionCanceledByHost()) => {} _ => panic!("Unexpected return"), }; - if sbox2.poisoned() { + if sbox2.status().is_poisoned() { sbox2.restore(snapshot2.clone()).unwrap(); } sbox3 @@ -243,7 +243,7 @@ fn interrupt_same_thread_no_barrier() { Ok(_) | Err(HyperlightError::ExecutionCanceledByHost()) => {} other => panic!("Unexpected return: {:?}", other), }; - if sbox2.poisoned() { + if sbox2.status().is_poisoned() { sbox2.restore(snapshot2.clone()).unwrap(); } sbox3 @@ -275,9 +275,9 @@ fn interrupt_moved_sandbox() { matches!(&res, HyperlightError::ExecutionCanceledByHost()), "unexpected error: {res:?}" ); - assert!(sbox1.poisoned()); + assert!(sbox1.status().is_poisoned()); sbox1.restore(snapshot1.clone()).unwrap(); - assert!(!sbox1.poisoned()); + assert_eq!(sbox1.status(), SandboxStatus::Ready); }); let thread2 = thread::spawn(move || { @@ -333,11 +333,11 @@ fn interrupt_custom_signal_no_and_retry_delay() { matches!(&res, HyperlightError::ExecutionCanceledByHost()), "unexpected error: {res:?}" ); - assert!(sbox1.poisoned()); + assert!(sbox1.status().is_poisoned()); // immediately reenter another guest function call after having being cancelled, // so that the vcpu is running again before the interruptor-thread has a chance to see that the vcpu is not running sbox1.restore(snapshot1.clone()).unwrap(); - assert!(!sbox1.poisoned()); + assert_eq!(sbox1.status(), SandboxStatus::Ready); } thread.join().expect("Thread should finish"); }); @@ -572,7 +572,7 @@ fn guest_outb_with_invalid_port_poisons_sandbox() { // The sandbox should be poisoned because the guest didn't complete normally assert!( - sbox.poisoned(), + sbox.status().is_poisoned(), "Sandbox should be poisoned after invalid OUT" ); }); @@ -1143,7 +1143,7 @@ fn interrupt_random_kill_stress_test() { let sandbox_wrapper = guard.sandbox_with_snapshot.as_mut().unwrap(); // Make sure the sandbox is poisoned - assert!(sandbox_wrapper.sandbox.poisoned()); + assert!(sandbox_wrapper.sandbox.status().is_poisoned()); // Try to restore the snapshot if let Err(e) = sandbox_wrapper diff --git a/src/hyperlight_host/tests/sandbox_host_tests.rs b/src/hyperlight_host/tests/sandbox_host_tests.rs index b1a1a9918..6c17e94f9 100644 --- a/src/hyperlight_host/tests/sandbox_host_tests.rs +++ b/src/hyperlight_host/tests/sandbox_host_tests.rs @@ -366,7 +366,7 @@ fn host_function_error() { res ); // C guest panics in rust guest lib when host function returns error, which will poison the sandbox - if init_sandbox.poisoned() { + if init_sandbox.status().is_poisoned() { init_sandbox.restore(snapshot.clone()).unwrap(); } } From cee4f357a89ac769d0fee6773bcdec67e68b7322 Mon Sep 17 00:00:00 2001 From: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Date: Mon, 20 Jul 2026 16:05:36 -0700 Subject: [PATCH 3/5] Relax snapshot restore layout compatibility Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> --- src/hyperlight_host/src/error.rs | 6 - .../src/hypervisor/hyperlight_vm/mod.rs | 5 + src/hyperlight_host/src/mem/layout.rs | 86 ----- .../src/sandbox/initialized_multi_use.rs | 364 +++++++++++++++--- .../src/sandbox/snapshot/file_tests.rs | 49 ++- .../src/sandbox/snapshot/mod.rs | 20 - 6 files changed, 367 insertions(+), 163 deletions(-) diff --git a/src/hyperlight_host/src/error.rs b/src/hyperlight_host/src/error.rs index 85d56565b..109e52aa1 100644 --- a/src/hyperlight_host/src/error.rs +++ b/src/hyperlight_host/src/error.rs @@ -244,11 +244,6 @@ pub enum HyperlightError { #[error("Failed To Convert Return Value {0:?} to {1:?}")] ReturnValueConversionFailure(ReturnValue, &'static str), - /// Tried to restore a snapshot into a sandbox whose memory - /// layout is not compatible with the snapshot's. - #[error("Snapshot memory layout is not compatible with this sandbox")] - SnapshotLayoutMismatch, - /// Tried to restore a snapshot into a sandbox whose registered /// host functions do not satisfy the snapshot's required set. #[error( @@ -405,7 +400,6 @@ impl HyperlightError { | HyperlightError::RefCellBorrowFailed(_) | HyperlightError::RefCellMutBorrowFailed(_) | HyperlightError::ReturnValueConversionFailure(_, _) - | HyperlightError::SnapshotLayoutMismatch | HyperlightError::SnapshotHostFunctionMismatch { .. } | HyperlightError::SystemTimeError(_) | HyperlightError::TryFromSliceError(_) diff --git a/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs b/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs index 9e3c6da19..707d59f18 100644 --- a/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs +++ b/src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs @@ -590,6 +590,11 @@ impl HyperlightVm { self.rt_cfg.entry_point = Some(entry_point); } + #[cfg(crashdump)] + pub(crate) fn clear_crashdump_binary_path(&mut self) { + self.rt_cfg.binary_path = None; + } + pub(crate) fn interrupt_handle(&self) -> Arc { self.interrupt_handle.clone() } diff --git a/src/hyperlight_host/src/mem/layout.rs b/src/hyperlight_host/src/mem/layout.rs index 6422b9b11..73daff646 100644 --- a/src/hyperlight_host/src/mem/layout.rs +++ b/src/hyperlight_host/src/mem/layout.rs @@ -317,40 +317,6 @@ impl Debug for SandboxMemoryLayout { } impl SandboxMemoryLayout { - /// Whether `other` has the same layout configuration as `self`, - /// i.e. the fields that come from the guest binary and the - /// `SandboxConfiguration`. `snapshot_size` and `pt_size` are - /// excluded because they are outputs of building a snapshot blob - /// (the compacted data size and the size of the rebuilt - /// page-table tail), not configuration inputs, so they differ - /// between the sandbox's live layout and any snapshot taken - /// from it. - /// - /// TODO: separate/remove snapshot_size and pt_size from this struct. - pub(crate) fn is_compatible_with(&self, other: &Self) -> bool { - // Exhaustive destructure so adding a field to - // `SandboxMemoryLayout` fails to compile here, forcing the - // author to decide whether it participates in compatibility. - let Self { - input_data_size, - output_data_size, - heap_size, - code_size, - init_data_size, - init_data_permissions, - scratch_size, - snapshot_size: _, - pt_size: _, - } = self; - *input_data_size == other.input_data_size - && *output_data_size == other.output_data_size - && *heap_size == other.heap_size - && *code_size == other.code_size - && *init_data_size == other.init_data_size - && *init_data_permissions == other.init_data_permissions - && *scratch_size == other.scratch_size - } - /// The maximum amount of memory a single sandbox will be allowed. /// /// Both the scratch region and the snapshot region are bounded by @@ -788,58 +754,6 @@ mod tests { assert!(matches!(layout.unwrap_err(), MemoryRequestTooBig(..))); } - #[test] - fn is_compatible_with_identical_layouts() { - let cfg = SandboxConfiguration::default(); - let a = SandboxMemoryLayout::new(cfg, 4096, 0, None).unwrap(); - let b = SandboxMemoryLayout::new(cfg, 4096, 0, None).unwrap(); - assert!(a.is_compatible_with(&b)); - assert!(b.is_compatible_with(&a)); - } - - #[test] - fn is_compatible_with_ignores_snapshot_size_and_pt_size() { - // `snapshot_size` and `pt_size` are outputs of building a - // snapshot blob, not configuration inputs, so flipping - // them must not break compatibility. - let cfg = SandboxConfiguration::default(); - let a = SandboxMemoryLayout::new(cfg, 4096, 0, None).unwrap(); - let mut b = a; - b.snapshot_size = a.snapshot_size + PAGE_SIZE_USIZE; - b.set_pt_size(PAGE_SIZE_USIZE).unwrap(); - assert!(a.is_compatible_with(&b)); - assert!(b.is_compatible_with(&a)); - } - - #[test] - fn is_compatible_with_rejects_each_configured_field() { - let cfg = SandboxConfiguration::default(); - let base = SandboxMemoryLayout::new(cfg, 4096, 0, None).unwrap(); - - // Each mutation must independently break compatibility. - let mutators: &[fn(&mut SandboxMemoryLayout)] = &[ - |l| l.input_data_size += PAGE_SIZE_USIZE, - |l| l.output_data_size += PAGE_SIZE_USIZE, - |l| l.heap_size += PAGE_SIZE_USIZE, - |l| l.code_size += PAGE_SIZE_USIZE, - |l| l.init_data_size += PAGE_SIZE_USIZE, - |l| l.scratch_size += PAGE_SIZE_USIZE, - |l| { - l.init_data_permissions = Some(MemoryRegionFlags::READ); - }, - ]; - for mutate in mutators { - let mut other = base; - mutate(&mut other); - assert!( - !base.is_compatible_with(&other), - "mutation should have broken compatibility: {:?} vs {:?}", - base, - other, - ); - } - } - /// Pinned region offsets. These methods place every region that a /// restored snapshot is interpreted against, so a change shifts /// where the loader reads captured bytes and breaks existing diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index 781b2e8d1..6c12d75d3 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -353,10 +353,9 @@ impl MultiUseSandbox { /// Creates a snapshot of the sandbox's current memory state. /// /// The returned snapshot can be applied to any - /// [`MultiUseSandbox`] whose memory layout is structurally - /// compatible with this sandbox's layout and whose registered - /// host functions are a superset of those registered here at the - /// time of capture. See [`MultiUseSandbox::restore`] and + /// [`MultiUseSandbox`] whose registered host functions are a + /// superset of those registered here at the time of capture. See + /// [`MultiUseSandbox::restore`] and /// [`MultiUseSandbox::from_snapshot`] for the exact compatibility /// rules and the error variants returned on mismatch. /// @@ -461,10 +460,6 @@ impl MultiUseSandbox { /// Restores the sandbox's memory to a previously captured snapshot state. /// - /// The snapshot's memory layout must be structurally compatible - /// with this sandbox's layout, otherwise this returns - /// [`SnapshotLayoutMismatch`](crate::HyperlightError::SnapshotLayoutMismatch). - /// /// The sandbox's registered host functions must be a superset of /// those required by the snapshot (matched by name and /// signature). Extras on the sandbox are allowed. The registry @@ -587,22 +582,28 @@ impl MultiUseSandbox { .host_funcs .try_lock() .map_err(|e| crate::new_error!("Error locking host_funcs: {}", e))?; - snapshot.validate_compatibility(&self.mem_mgr.layout, &host_funcs)?; + snapshot.validate_host_functions(&host_funcs)?; } let sregs = snapshot.sregs().ok_or_else(|| { HyperlightError::Error("snapshot from running sandbox should have sregs".to_string()) })?; + self.status = SandboxStatus::Poisoned; + self.snapshot = None; + + let current_regions: Vec = self.vm.get_mapped_regions().cloned().collect(); + for region in ¤t_regions { + self.vm + .unmap_region(region) + .map_err(HyperlightVmError::UnmapRegion)?; + } + if let Err(error) = self.restore_memory_and_mappings(&snapshot) { self.status = SandboxStatus::Unrecoverable; - self.snapshot = None; return Err(error); } - self.status = SandboxStatus::Poisoned; - self.snapshot = None; - self.vm .reset_vcpu(snapshot.root_pt_gpa(), sregs) .map_err(HyperlightVmError::Restore)?; @@ -618,16 +619,14 @@ impl MultiUseSandbox { // Carry the guest ELF entry point across restore so a later // crashdump fills `AT_ENTRY` from the restored image. #[cfg(crashdump)] - self.vm - .set_crashdump_entry_point(snapshot.original_entrypoint()); - - let current_regions: Vec = self.vm.get_mapped_regions().cloned().collect(); - for region in ¤t_regions { + { self.vm - .unmap_region(region) - .map_err(HyperlightVmError::UnmapRegion)?; + .set_crashdump_entry_point(snapshot.original_entrypoint()); + self.vm.clear_crashdump_binary_path(); } + self.pt_root_finder = None; + // The restored snapshot is now our most current snapshot self.snapshot = Some(snapshot.clone()); @@ -1197,6 +1196,7 @@ mod tests { use hyperlight_testing::sandbox_sizes::{LARGE_HEAP_SIZE, MEDIUM_HEAP_SIZE, SMALL_HEAP_SIZE}; use hyperlight_testing::simple_guest_as_pathbuf; + use crate::func::host_functions::Registerable; #[cfg(not(gdb))] use crate::hypervisor::hyperlight_vm::test_support::VmOperation; use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags, MemoryRegionType}; @@ -2054,27 +2054,206 @@ mod tests { } #[test] - fn snapshot_restore_rejects_incompatible_layout() { - let mut sandbox = { - let path = simple_guest_as_pathbuf(); - let mut cfg = SandboxConfiguration::default(); - cfg.set_heap_size(0x10_000); - let u_sbox = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(cfg)).unwrap(); - u_sbox.evolve().unwrap() - }; + fn snapshot_restore_accepts_different_configured_layout() { + type Configure = fn(&mut SandboxConfiguration); + type LayoutValue = fn(&crate::mem::layout::SandboxMemoryLayout) -> usize; + let cases: &[(&str, Configure, LayoutValue)] = &[ + ( + "input", + |cfg| cfg.set_input_data_size(0x8000), + |layout| layout.input_data_size(), + ), + ( + "output", + |cfg| cfg.set_output_data_size(0x8000), + |layout| layout.output_data_size(), + ), + ( + "heap", + |cfg| cfg.set_heap_size(0x40_000), + |layout| layout.heap_size(), + ), + ( + "scratch", + |cfg| cfg.set_scratch_size(0x90_000), + |layout| layout.get_scratch_size(), + ), + ]; - let mut sandbox2 = { - let path = simple_guest_as_pathbuf(); - let mut cfg = SandboxConfiguration::default(); - cfg.set_heap_size(0x20_000); - cfg.set_scratch_size(0x60_000); - let u_sbox = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(cfg)).unwrap(); - u_sbox.evolve().unwrap() - }; + for (name, configure, layout_value) in cases { + for incoming_is_larger in [true, false] { + let mut custom_cfg = SandboxConfiguration::default(); + configure(&mut custom_cfg); + let (source_cfg, target_cfg) = if incoming_is_larger { + (custom_cfg, SandboxConfiguration::default()) + } else { + (SandboxConfiguration::default(), custom_cfg) + }; - let snapshot = sandbox.snapshot().unwrap(); - let err = sandbox2.restore(snapshot); - assert!(matches!(err, Err(HyperlightError::SnapshotLayoutMismatch))); + let path = simple_guest_as_pathbuf(); + let mut source = + UninitializedSandbox::new(GuestBinary::FilePath(path), Some(source_cfg)) + .unwrap() + .evolve() + .unwrap(); + + let path = simple_guest_as_pathbuf(); + let mut target = + UninitializedSandbox::new(GuestBinary::FilePath(path), Some(target_cfg)) + .unwrap() + .evolve() + .unwrap(); + + let source_value = layout_value(&source.mem_mgr.layout); + assert_ne!(source_value, layout_value(&target.mem_mgr.layout)); + + source.call::("AddToStatic", 42i32).unwrap(); + target + .restore(source.snapshot().unwrap()) + .unwrap_or_else(|err| panic!("restore with different {name} layout: {err}")); + assert_eq!(layout_value(&target.mem_mgr.layout), source_value); + assert_eq!(target.call::("GetStatic", ()).unwrap(), 42); + } + } + } + + #[test] + fn snapshot_restore_recovers_oom_with_larger_heap() { + let mut source_cfg = SandboxConfiguration::default(); + source_cfg.set_heap_size(0x20_000); + let path = simple_guest_as_pathbuf(); + let mut source = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(source_cfg)) + .unwrap() + .evolve() + .unwrap(); + let snapshot = source.snapshot().unwrap(); + + let mut target_cfg = SandboxConfiguration::default(); + target_cfg.set_heap_size(0x6000); + let path = simple_guest_as_pathbuf(); + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(target_cfg)) + .unwrap() + .evolve() + .unwrap(); + + assert!(target.call::<()>("ExhaustHeap", ()).is_err()); + assert!(target.status().is_poisoned()); + + target.restore(snapshot).unwrap(); + assert!(!target.status().is_poisoned()); + assert_eq!( + target.call::("CallMalloc", 0x10_000i32).unwrap(), + 0x10_000 + ); + } + + #[test] + fn snapshot_restore_applies_smaller_heap_limit() { + let mut source_cfg = SandboxConfiguration::default(); + source_cfg.set_heap_size(0x6000); + let path = simple_guest_as_pathbuf(); + let mut source = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(source_cfg)) + .unwrap() + .evolve() + .unwrap(); + let snapshot = source.snapshot().unwrap(); + + let mut target_cfg = SandboxConfiguration::default(); + target_cfg.set_heap_size(0x20_000); + let path = simple_guest_as_pathbuf(); + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(target_cfg)) + .unwrap() + .evolve() + .unwrap(); + + assert_eq!( + target.call::("CallMalloc", 0x10_000i32).unwrap(), + 0x10_000 + ); + target.restore(snapshot).unwrap(); + assert_eq!(target.mem_mgr.layout.heap_size(), 0x6000); + assert!(target.call::("CallMalloc", 0x10_000i32).is_err()); + assert!(target.status().is_poisoned()); + } + + #[test] + fn snapshot_restore_applies_smaller_io_limits() { + let mut source_cfg = SandboxConfiguration::default(); + source_cfg.set_input_data_size(0x2000); + source_cfg.set_output_data_size(0x2000); + let path = simple_guest_as_pathbuf(); + let mut source = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(source_cfg)) + .unwrap() + .evolve() + .unwrap(); + let snapshot = source.snapshot().unwrap(); + + let mut target_cfg = SandboxConfiguration::default(); + target_cfg.set_input_data_size(0x8000); + target_cfg.set_output_data_size(0x8000); + let path = simple_guest_as_pathbuf(); + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(target_cfg)) + .unwrap() + .evolve() + .unwrap(); + let large = "x".repeat(0x3000); + + assert_eq!(target.call::("Echo", large.clone()).unwrap(), large); + target.restore(snapshot).unwrap(); + assert_eq!(target.mem_mgr.layout.input_data_size(), 0x2000); + assert_eq!(target.mem_mgr.layout.output_data_size(), 0x2000); + assert!(target.call::("Echo", large).is_err()); + assert!(!target.status().is_poisoned()); + assert_eq!( + target.call::("Echo", "small".to_string()).unwrap(), + "small" + ); + } + + #[test] + fn snapshot_restore_alternates_different_layouts() { + let mut small_cfg = SandboxConfiguration::default(); + small_cfg.set_input_data_size(0x2000); + small_cfg.set_output_data_size(0x2000); + small_cfg.set_heap_size(0x6000); + let path = simple_guest_as_pathbuf(); + let mut small = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(small_cfg)) + .unwrap() + .evolve() + .unwrap(); + small.call::("AddToStatic", 11i32).unwrap(); + let small_snapshot = small.snapshot().unwrap(); + + let mut large_cfg = SandboxConfiguration::default(); + large_cfg.set_input_data_size(0x8000); + large_cfg.set_output_data_size(0x8000); + large_cfg.set_heap_size(0x40_000); + large_cfg.set_scratch_size(0x90_000); + let path = simple_guest_as_pathbuf(); + let mut large = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(large_cfg)) + .unwrap() + .evolve() + .unwrap(); + large.call::("AddToStatic", 22i32).unwrap(); + let large_snapshot = large.snapshot().unwrap(); + + let path = simple_guest_as_pathbuf(); + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + + target.restore(small_snapshot.clone()).unwrap(); + assert_eq!(target.call::("GetStatic", ()).unwrap(), 11); + assert_eq!(target.mem_mgr.layout.heap_size(), 0x6000); + + target.restore(large_snapshot).unwrap(); + assert_eq!(target.call::("GetStatic", ()).unwrap(), 22); + assert_eq!(target.mem_mgr.layout.heap_size(), 0x40_000); + + target.restore(small_snapshot).unwrap(); + assert_eq!(target.call::("GetStatic", ()).unwrap(), 11); + assert_eq!(target.mem_mgr.layout.heap_size(), 0x6000); } /// Validation runs before any memory or vCPU mutation, so a @@ -2082,26 +2261,50 @@ mod tests { #[test] fn snapshot_restore_failure_leaves_target_usable() { let path = simple_guest_as_pathbuf(); - let mut cfg_a = SandboxConfiguration::default(); - cfg_a.set_heap_size(0x10_000); - let mut source = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(cfg_a)) - .unwrap() - .evolve() + let mut source = UninitializedSandbox::new(GuestBinary::FilePath(path), None).unwrap(); + source + .register_host_function("Add", |a: i32, b: i32| Ok(a + b)) .unwrap(); + let mut source = source.evolve().unwrap(); let path = simple_guest_as_pathbuf(); - let mut cfg_b = SandboxConfiguration::default(); - cfg_b.set_heap_size(0x20_000); - let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(cfg_b)) + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), None) .unwrap() .evolve() .unwrap(); target.call::("AddToStatic", 5i32).unwrap(); + let map_mem = allocate_guest_memory(); + let guest_base = 0x200000000_usize; + let region = region_for_memory(&map_mem, guest_base, MemoryRegionFlags::READ); + // SAFETY: `map_mem` is page-aligned and outlives every use of `target`. + unsafe { target.map_region(®ion).unwrap() }; + target + .call::>( + "ReadMappedBuffer", + ( + guest_base as u64, + hyperlight_common::vmem::PAGE_SIZE as u64, + true, + ), + ) + .unwrap(); + let cached_snapshot = target.snapshot().unwrap(); let bad_snapshot = source.snapshot().unwrap(); let err = target.restore(bad_snapshot); - assert!(matches!(err, Err(HyperlightError::SnapshotLayoutMismatch))); + assert!(matches!( + err, + Err(HyperlightError::SnapshotHostFunctionMismatch { missing, .. }) + if missing.iter().any(|name| name == "Add") + )); + assert!(Arc::ptr_eq(&target.snapshot().unwrap(), &cached_snapshot)); + assert_eq!(target.vm.get_mapped_regions().count(), 1); + assert!( + target + .call::("CheckMapped", guest_base as u64) + .unwrap() + ); assert_eq!(target.call::("GetStatic", ()).unwrap(), 5); target.call::("AddToStatic", 3i32).unwrap(); assert_eq!(target.call::("GetStatic", ()).unwrap(), 8); @@ -2141,6 +2344,71 @@ mod tests { assert_eq!(target.call::("GetStatic", ()).unwrap(), 23); } + #[test] + fn snapshot_restore_unmaps_regions_overlapping_incoming_layout() { + let mut source_cfg = SandboxConfiguration::default(); + source_cfg.set_scratch_size(0x90_000); + let path = simple_guest_as_pathbuf(); + let mut source = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(source_cfg)) + .unwrap() + .evolve() + .unwrap(); + source.call::("AddToStatic", 23i32).unwrap(); + let snapshot = source.snapshot().unwrap(); + + let path = simple_guest_as_pathbuf(); + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + assert!(snapshot.memory().mem_size() > target.mem_mgr.shared_mem.mem_size()); + + let map_mem = allocate_guest_memory(); + let guest_base = crate::mem::layout::SandboxMemoryLayout::BASE_ADDRESS + + target.mem_mgr.shared_mem.mem_size(); + let region = region_for_memory(&map_mem, guest_base, MemoryRegionFlags::READ); + // SAFETY: `map_mem` is page-aligned and outlives every use of `target`. + unsafe { target.map_region(®ion).unwrap() }; + + target.restore(snapshot).unwrap(); + assert_eq!(target.vm.get_mapped_regions().count(), 0); + assert_eq!(target.call::("GetStatic", ()).unwrap(), 23); + } + + #[test] + fn snapshot_restore_unmaps_region_overlapping_incoming_scratch() { + let incoming_scratch_size = 0x90_000; + let mut source_cfg = SandboxConfiguration::default(); + source_cfg.set_scratch_size(incoming_scratch_size); + let path = simple_guest_as_pathbuf(); + let mut source = UninitializedSandbox::new(GuestBinary::FilePath(path), Some(source_cfg)) + .unwrap() + .evolve() + .unwrap(); + source.call::("AddToStatic", 23i32).unwrap(); + let snapshot = source.snapshot().unwrap(); + + let path = simple_guest_as_pathbuf(); + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + let guest_base = + hyperlight_common::layout::scratch_base_gpa(incoming_scratch_size) as usize; + let target_scratch_base = + hyperlight_common::layout::scratch_base_gpa(SandboxConfiguration::DEFAULT_SCRATCH_SIZE) + as usize; + let map_mem = allocate_guest_memory(); + assert!(guest_base + map_mem.mem_size() <= target_scratch_base); + let region = region_for_memory(&map_mem, guest_base, MemoryRegionFlags::READ); + // SAFETY: `map_mem` is page-aligned and outlives every use of `target`. + unsafe { target.map_region(®ion).unwrap() }; + + target.restore(snapshot).unwrap(); + assert_eq!(target.vm.get_mapped_regions().count(), 0); + assert_eq!(target.call::("GetStatic", ()).unwrap(), 23); + } + /// Compacted snapshot data is reachable at the source's GVA even /// when the target had a different region mapped at a different /// GVA. diff --git a/src/hyperlight_host/src/sandbox/snapshot/file_tests.rs b/src/hyperlight_host/src/sandbox/snapshot/file_tests.rs index 9787debe6..764e3ff4d 100644 --- a/src/hyperlight_host/src/sandbox/snapshot/file_tests.rs +++ b/src/hyperlight_host/src/sandbox/snapshot/file_tests.rs @@ -2778,6 +2778,50 @@ fn round_trip_preserves_non_default_scratch_size() { assert_eq!(loaded.layout().get_scratch_size(), custom_scratch); } +#[test] +fn persisted_non_default_layout_loads_and_runs() { + use crate::sandbox::SandboxConfiguration; + + let mut config = SandboxConfiguration::default(); + config.set_input_data_size(0x8000); + config.set_output_data_size(0x8000); + config.set_heap_size(0x40_000); + config.set_scratch_size(0x90_000); + let mut source = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_pathbuf()), + Some(config), + ) + .unwrap() + .evolve() + .unwrap(); + source.call::("AddToStatic", 42i32).unwrap(); + let snapshot = source.snapshot().unwrap(); + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("layout"); + snapshot + .save(&path, &OciTag::new("latest").unwrap()) + .unwrap(); + let loaded = Arc::new(Snapshot::checked_load(&path, OciTag::new("latest").unwrap()).unwrap()); + assert_eq!(loaded.layout().input_data_size(), 0x8000); + assert_eq!(loaded.layout().output_data_size(), 0x8000); + assert_eq!(loaded.layout().heap_size(), 0x40_000); + assert_eq!(loaded.layout().get_scratch_size(), 0x90_000); + + let mut restored = + MultiUseSandbox::from_snapshot(loaded, HostFunctions::default(), None).unwrap(); + assert_eq!(restored.call::("GetStatic", ()).unwrap(), 42); + let large = "x".repeat(0x5000); + assert_eq!( + restored.call::("Echo", large.clone()).unwrap(), + large + ); + assert_eq!( + restored.call::("CallMalloc", 0x10_000i32).unwrap(), + 0x10_000 + ); +} + #[test] fn snapshot_config_records_entrypoint_and_sregs() { let snap = create_snapshot(); @@ -2840,9 +2884,8 @@ fn snapshot_with_no_host_functions_round_trips() { MultiUseSandbox::from_snapshot(Arc::new(loaded), HostFunctions::default(), None).unwrap(); } -// Snapshot lineage and restore semantics. `restore` accepts any -// snapshot whose memory layout and host-function set match the sandbox. -// Snapshots within a compatible set are interchangeable. +// Snapshot lineage and restore semantics. `restore` accepts snapshots +// whose required host functions match the sandbox. #[test] fn linear_chain_restore_in_order() { diff --git a/src/hyperlight_host/src/sandbox/snapshot/mod.rs b/src/hyperlight_host/src/sandbox/snapshot/mod.rs index e1578e2e7..8ab4dde31 100644 --- a/src/hyperlight_host/src/sandbox/snapshot/mod.rs +++ b/src/hyperlight_host/src/sandbox/snapshot/mod.rs @@ -705,26 +705,6 @@ impl Snapshot { signature_mismatches, }) } - - /// Validate that this snapshot can be applied to a sandbox with - /// the given memory layout and host-function registry. - /// - /// The layout must be structurally compatible with the snapshot's - /// layout (see - /// [`SandboxMemoryLayout::is_compatible_with`](crate::mem::layout::SandboxMemoryLayout::is_compatible_with)), - /// and the registry must be a superset of the host functions the - /// snapshot requires (see - /// [`validate_host_functions`](Self::validate_host_functions)). - pub(crate) fn validate_compatibility( - &self, - layout: &crate::mem::layout::SandboxMemoryLayout, - host_funcs: &crate::sandbox::host_funcs::FunctionRegistry, - ) -> Result<()> { - if !self.layout().is_compatible_with(layout) { - return Err(crate::HyperlightError::SnapshotLayoutMismatch); - } - self.validate_host_functions(host_funcs) - } } #[cfg(test)] From d00dbbbc4da91f2b45c5b666de89d7c41720ae82 Mon Sep 17 00:00:00 2001 From: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Date: Mon, 20 Jul 2026 16:06:42 -0700 Subject: [PATCH 4/5] Test cross-guest snapshot restore Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> --- .../src/sandbox/initialized_multi_use.rs | 200 +++++++++++++++++- src/hyperlight_host/tests/wit_test.rs | 163 +++++++++++++- 2 files changed, 359 insertions(+), 4 deletions(-) diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index 6c12d75d3..eec6e669e 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -1194,7 +1194,7 @@ mod tests { use hyperlight_common::flatbuffer_wrappers::guest_error::ErrorCode; use hyperlight_testing::sandbox_sizes::{LARGE_HEAP_SIZE, MEDIUM_HEAP_SIZE, SMALL_HEAP_SIZE}; - use hyperlight_testing::simple_guest_as_pathbuf; + use hyperlight_testing::{c_simple_guest_as_pathbuf, simple_guest_as_pathbuf}; use crate::func::host_functions::Registerable; #[cfg(not(gdb))] @@ -1202,6 +1202,7 @@ mod tests { use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags, MemoryRegionType}; use crate::mem::shared_mem::{ExclusiveSharedMemory, GuestSharedMemory, SharedMemory as _}; use crate::sandbox::SandboxConfiguration; + use crate::sandbox::uninitialized::{GuestBlob, GuestEnvironment}; use crate::{ GuestBinary, HyperlightError, MultiUseSandbox, Result, SandboxStatus, UninitializedSandbox, }; @@ -2256,6 +2257,203 @@ mod tests { assert_eq!(target.mem_mgr.layout.heap_size(), 0x6000); } + #[test] + fn snapshot_restore_replaces_rust_guest_with_c_guest() { + let init_data = b"cross-layout-init-data"; + let source_env = GuestEnvironment { + guest_binary: GuestBinary::FilePath(c_simple_guest_as_pathbuf()), + init_data: Some(GuestBlob { + data: init_data, + permissions: MemoryRegionFlags::READ | MemoryRegionFlags::WRITE, + }), + }; + let mut source = UninitializedSandbox::new(source_env, None) + .unwrap() + .evolve() + .unwrap(); + let mut target = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_pathbuf()), + None, + ) + .unwrap() + .evolve() + .unwrap(); + + assert_eq!(source.call::("StackAllocate", 256i32).unwrap(), 256); + assert_eq!(target.call::("AddToStatic", 17i32).unwrap(), 17); + target.set_pt_root_finder(Box::new(|_, _, root| vec![root])); + assert!(target.pt_root_finder.is_some()); + + assert_ne!( + source.mem_mgr.layout.code_size(), + target.mem_mgr.layout.code_size() + ); + assert_ne!( + source.mem_mgr.layout.init_data_size(), + target.mem_mgr.layout.init_data_size() + ); + assert_ne!( + source.mem_mgr.layout.init_data_permissions(), + target.mem_mgr.layout.init_data_permissions() + ); + + let snapshot = source.snapshot().unwrap(); + target.restore(snapshot).unwrap(); + assert!(target.pt_root_finder.is_none()); + assert_eq!(target.call::("StackAllocate", 512i32).unwrap(), 512); + assert!(matches!( + target.call::("GetStatic", ()), + Err(HyperlightError::GuestError( + ErrorCode::GuestFunctionNotFound, + name + )) if name == "GetStatic" + )); + } + + #[test] + fn snapshot_restore_replaces_c_guest_with_rust_guest() { + let mut source = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_pathbuf()), + None, + ) + .unwrap() + .evolve() + .unwrap(); + assert_eq!(source.call::("AddToStatic", 42i32).unwrap(), 42); + let snapshot = source.snapshot().unwrap(); + + let mut target = UninitializedSandbox::new( + GuestBinary::FilePath(c_simple_guest_as_pathbuf()), + None, + ) + .unwrap() + .evolve() + .unwrap(); + assert_eq!(target.call::("StackAllocate", 256i32).unwrap(), 256); + + target.restore(snapshot).unwrap(); + assert_eq!(target.call::("GetStatic", ()).unwrap(), 42); + assert!(matches!( + target.call::("StackAllocate", 512i32), + Err(HyperlightError::GuestError( + ErrorCode::GuestFunctionNotFound, + name + )) if name == "StackAllocate" + )); + } + + #[test] + fn snapshot_restore_alternates_c_and_rust_guests() { + let mut c_source = UninitializedSandbox::new( + GuestBinary::FilePath(c_simple_guest_as_pathbuf()), + None, + ) + .unwrap() + .evolve() + .unwrap(); + assert_eq!(c_source.call::("StackAllocate", 256i32).unwrap(), 256); + let c_snapshot = c_source.snapshot().unwrap(); + + let mut rust_source = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_pathbuf()), + None, + ) + .unwrap() + .evolve() + .unwrap(); + rust_source.call::("AddToStatic", 42i32).unwrap(); + let rust_snapshot = rust_source.snapshot().unwrap(); + + let mut target = UninitializedSandbox::new( + GuestBinary::FilePath(c_simple_guest_as_pathbuf()), + None, + ) + .unwrap() + .evolve() + .unwrap(); + assert_eq!(target.call::("StackAllocate", 256i32).unwrap(), 256); + + target.restore(rust_snapshot).unwrap(); + assert_eq!(target.call::("GetStatic", ()).unwrap(), 42); + assert!(matches!( + target.call::("StackAllocate", 512i32), + Err(HyperlightError::GuestError( + ErrorCode::GuestFunctionNotFound, + name + )) if name == "StackAllocate" + )); + + target.restore(c_snapshot).unwrap(); + assert_eq!(target.call::("StackAllocate", 512i32).unwrap(), 512); + assert!(matches!( + target.call::("GetStatic", ()), + Err(HyperlightError::GuestError( + ErrorCode::GuestFunctionNotFound, + name + )) if name == "GetStatic" + )); + } + + #[test] + fn snapshot_restore_keeps_target_host_function_implementation() { + let path = simple_guest_as_pathbuf(); + let mut source = UninitializedSandbox::new(GuestBinary::FilePath(path), None).unwrap(); + source + .register_host_function("Echo42", || Ok(1i64)) + .unwrap(); + let mut source = source.evolve().unwrap(); + let snapshot = source.snapshot().unwrap(); + + let path = simple_guest_as_pathbuf(); + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), None).unwrap(); + target + .register_host_function("Echo42", || Ok(42i64)) + .unwrap(); + let mut target = target.evolve().unwrap(); + + target.restore(snapshot).unwrap(); + assert_eq!( + target + .call::( + "CallGivenParamlessHostFuncThatReturnsI64", + "Echo42".to_string(), + ) + .unwrap(), + 42 + ); + } + + #[test] + fn snapshot_restore_recovers_poison_with_different_guest() { + let mut source = UninitializedSandbox::new( + GuestBinary::FilePath(c_simple_guest_as_pathbuf()), + None, + ) + .unwrap() + .evolve() + .unwrap(); + let snapshot = source.snapshot().unwrap(); + + let path = simple_guest_as_pathbuf(); + let mut target = UninitializedSandbox::new(GuestBinary::FilePath(path), None) + .unwrap() + .evolve() + .unwrap(); + assert!(target.call::<()>("ExhaustHeap", ()).is_err()); + assert!(target.status().is_poisoned()); + + target.restore(snapshot).unwrap(); + assert!(!target.status().is_poisoned()); + assert_eq!(target.call::("StackAllocate", 512i32).unwrap(), 512); + assert!(matches!( + target.call::("GetStatic", ()), + Err(HyperlightError::GuestError( + ErrorCode::GuestFunctionNotFound, + name + )) if name == "GetStatic" + )); + } + /// Validation runs before any memory or vCPU mutation, so a /// rejected `restore` leaves the target usable. #[test] diff --git a/src/hyperlight_host/tests/wit_test.rs b/src/hyperlight_host/tests/wit_test.rs index 983804842..800bf21a1 100644 --- a/src/hyperlight_host/tests/wit_test.rs +++ b/src/hyperlight_host/tests/wit_test.rs @@ -14,12 +14,13 @@ See the License for the specific language governing permissions and limitations under the License. */ +use std::path::PathBuf; use std::sync::{Arc, Mutex}; use hyperlight_common::component::{Negative, Positive}; use hyperlight_common::resource::BorrowedResourceGuard; use hyperlight_host::{GuestBinary, MultiUseSandbox, UninitializedSandbox}; -use hyperlight_testing::wit_guest_as_pathbuf; +use hyperlight_testing::{c_simple_guest_as_pathbuf, simple_guest_as_pathbuf, wit_guest_as_pathbuf}; extern crate alloc; mod bindings { @@ -286,7 +287,10 @@ impl test::wit::TestImports for Host { } fn sb() -> TestSandbox { - let path = wit_guest_as_pathbuf(); + sb_from_guest(wit_guest_as_pathbuf()) +} + +fn sb_from_guest(path: PathBuf) -> TestSandbox { let guest_path = GuestBinary::FilePath(path); let uninit = UninitializedSandbox::new(guest_path, None).unwrap(); test::wit::Test::instantiate(uninit, Host {}) @@ -294,10 +298,163 @@ fn sb() -> TestSandbox { mod wit_test { + use hyperlight_common::flatbuffer_wrappers::guest_error::ErrorCode; + use hyperlight_host::HyperlightError; use proptest::prelude::*; use crate::bindings::test::wit::{Roundtrip, TestExports, TestHostResource, roundtrip}; - use crate::sb; + use crate::{ + GuestBinary, UninitializedSandbox, c_simple_guest_as_pathbuf, sb, sb_from_guest, + simple_guest_as_pathbuf, + }; + + #[test] + fn restore_wit_snapshot_replaces_rust_and_c_guests() { + let mut source = sb(); + assert_eq!( + source + .roundtrip() + .roundtrip_string("before snapshot".to_string()), + "before snapshot" + ); + let snapshot = source.sb.snapshot().unwrap(); + + let mut rust_target = sb_from_guest(simple_guest_as_pathbuf()); + assert_eq!( + rust_target.sb.call::("AddToStatic", 17i32).unwrap(), + 17 + ); + rust_target.sb.restore(snapshot.clone()).unwrap(); + assert_eq!( + rust_target + .roundtrip() + .roundtrip_string("restored over Rust".to_string()), + "restored over Rust" + ); + assert!(matches!( + rust_target.sb.call::("GetStatic", ()), + Err(HyperlightError::GuestError( + ErrorCode::GuestFunctionNotFound, + name + )) if name == "GetStatic" + )); + + let mut c_target = sb_from_guest(c_simple_guest_as_pathbuf()); + assert_eq!( + c_target.sb.call::("StackAllocate", 256i32).unwrap(), + 256 + ); + c_target.sb.restore(snapshot).unwrap(); + assert_eq!( + c_target + .roundtrip() + .roundtrip_string("restored over C".to_string()), + "restored over C" + ); + assert!(matches!( + c_target.sb.call::("StackAllocate", 512i32), + Err(HyperlightError::GuestError( + ErrorCode::GuestFunctionNotFound, + name + )) if name == "StackAllocate" + )); + } + + #[test] + fn restore_rust_and_c_snapshots_replace_wit_guest() { + let mut rust_source = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_pathbuf()), + None, + ) + .unwrap() + .evolve() + .unwrap(); + assert_eq!(rust_source.call::("AddToStatic", 42i32).unwrap(), 42); + let rust_snapshot = rust_source.snapshot().unwrap(); + + let mut rust_target = sb(); + assert_eq!( + rust_target + .roundtrip() + .roundtrip_string("WIT before Rust".to_string()), + "WIT before Rust" + ); + rust_target.sb.restore(rust_snapshot).unwrap(); + assert_eq!(rust_target.sb.call::("GetStatic", ()).unwrap(), 42); + + let mut c_source = UninitializedSandbox::new( + GuestBinary::FilePath(c_simple_guest_as_pathbuf()), + None, + ) + .unwrap() + .evolve() + .unwrap(); + assert_eq!(c_source.call::("StackAllocate", 256i32).unwrap(), 256); + let c_snapshot = c_source.snapshot().unwrap(); + + let mut c_target = sb(); + assert_eq!( + c_target + .roundtrip() + .roundtrip_string("WIT before C".to_string()), + "WIT before C" + ); + c_target.sb.restore(c_snapshot).unwrap(); + assert_eq!( + c_target.sb.call::("StackAllocate", 512i32).unwrap(), + 512 + ); + } + + #[test] + fn restore_chain_replaces_each_guest() { + let mut rust_source = UninitializedSandbox::new( + GuestBinary::FilePath(simple_guest_as_pathbuf()), + None, + ) + .unwrap() + .evolve() + .unwrap(); + assert_eq!(rust_source.call::("AddToStatic", 42i32).unwrap(), 42); + let rust_snapshot = rust_source.snapshot().unwrap(); + + let mut wit_source = sb(); + assert_eq!( + wit_source + .roundtrip() + .roundtrip_string("WIT source".to_string()), + "WIT source" + ); + let wit_snapshot = wit_source.sb.snapshot().unwrap(); + + let mut target = sb_from_guest(c_simple_guest_as_pathbuf()); + assert_eq!(target.sb.call::("StackAllocate", 256i32).unwrap(), 256); + + target.sb.restore(rust_snapshot).unwrap(); + assert_eq!(target.sb.call::("GetStatic", ()).unwrap(), 42); + assert!(matches!( + target.sb.call::("StackAllocate", 512i32), + Err(HyperlightError::GuestError( + ErrorCode::GuestFunctionNotFound, + name + )) if name == "StackAllocate" + )); + + target.sb.restore(wit_snapshot).unwrap(); + assert_eq!( + target + .roundtrip() + .roundtrip_string("WIT restored".to_string()), + "WIT restored" + ); + assert!(matches!( + target.sb.call::("GetStatic", ()), + Err(HyperlightError::GuestError( + ErrorCode::GuestFunctionNotFound, + name + )) if name == "GetStatic" + )); + } prop_compose! { fn arb_testrecord()(contents in ".*", length in any::()) -> roundtrip::Testrecord { From 8297af893e52fd3956a24d1690754f84727a24fd Mon Sep 17 00:00:00 2001 From: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Date: Mon, 20 Jul 2026 17:37:08 -0700 Subject: [PATCH 5/5] Document relaxed snapshot restore requirements Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> --- CHANGELOG.md | 1 + .../src/sandbox/initialized_multi_use.rs | 84 ++++++++----------- src/hyperlight_host/tests/wit_test.rs | 40 ++++----- 3 files changed, 54 insertions(+), 71 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51c781fae..e82c6809d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). MSRs, on MSHV and WHP this is not enforced. by @ludfjig in https://github.com/hyperlight-dev/hyperlight/pull/991 * **Breaking:** Filesystem paths are now represented using `PathBuf`. `GuestBinary::FilePath` now stores a `PathBuf` instead of a `String`, and `MultiUseSandbox::generate_crashdump_to_dir` accepts `Into` instead of `Into`. Callers passing a `String` to `GuestBinary::FilePath` must convert it using `.into()`. * Deprecate `MultiUseSandbox::poisoned` in favor of `MultiUseSandbox::status().is_poisoned()`. +* `MultiUseSandbox::restore` has been made more flexible and now accepts snapshots from any guest binary or memory layout when host functions are compatible. ### Removed diff --git a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs index eec6e669e..2df1cafcc 100644 --- a/src/hyperlight_host/src/sandbox/initialized_multi_use.rs +++ b/src/hyperlight_host/src/sandbox/initialized_multi_use.rs @@ -2271,13 +2271,11 @@ mod tests { .unwrap() .evolve() .unwrap(); - let mut target = UninitializedSandbox::new( - GuestBinary::FilePath(simple_guest_as_pathbuf()), - None, - ) - .unwrap() - .evolve() - .unwrap(); + let mut target = + UninitializedSandbox::new(GuestBinary::FilePath(simple_guest_as_pathbuf()), None) + .unwrap() + .evolve() + .unwrap(); assert_eq!(source.call::("StackAllocate", 256i32).unwrap(), 256); assert_eq!(target.call::("AddToStatic", 17i32).unwrap(), 17); @@ -2312,23 +2310,19 @@ mod tests { #[test] fn snapshot_restore_replaces_c_guest_with_rust_guest() { - let mut source = UninitializedSandbox::new( - GuestBinary::FilePath(simple_guest_as_pathbuf()), - None, - ) - .unwrap() - .evolve() - .unwrap(); + let mut source = + UninitializedSandbox::new(GuestBinary::FilePath(simple_guest_as_pathbuf()), None) + .unwrap() + .evolve() + .unwrap(); assert_eq!(source.call::("AddToStatic", 42i32).unwrap(), 42); let snapshot = source.snapshot().unwrap(); - let mut target = UninitializedSandbox::new( - GuestBinary::FilePath(c_simple_guest_as_pathbuf()), - None, - ) - .unwrap() - .evolve() - .unwrap(); + let mut target = + UninitializedSandbox::new(GuestBinary::FilePath(c_simple_guest_as_pathbuf()), None) + .unwrap() + .evolve() + .unwrap(); assert_eq!(target.call::("StackAllocate", 256i32).unwrap(), 256); target.restore(snapshot).unwrap(); @@ -2344,33 +2338,27 @@ mod tests { #[test] fn snapshot_restore_alternates_c_and_rust_guests() { - let mut c_source = UninitializedSandbox::new( - GuestBinary::FilePath(c_simple_guest_as_pathbuf()), - None, - ) - .unwrap() - .evolve() - .unwrap(); + let mut c_source = + UninitializedSandbox::new(GuestBinary::FilePath(c_simple_guest_as_pathbuf()), None) + .unwrap() + .evolve() + .unwrap(); assert_eq!(c_source.call::("StackAllocate", 256i32).unwrap(), 256); let c_snapshot = c_source.snapshot().unwrap(); - let mut rust_source = UninitializedSandbox::new( - GuestBinary::FilePath(simple_guest_as_pathbuf()), - None, - ) - .unwrap() - .evolve() - .unwrap(); + let mut rust_source = + UninitializedSandbox::new(GuestBinary::FilePath(simple_guest_as_pathbuf()), None) + .unwrap() + .evolve() + .unwrap(); rust_source.call::("AddToStatic", 42i32).unwrap(); let rust_snapshot = rust_source.snapshot().unwrap(); - let mut target = UninitializedSandbox::new( - GuestBinary::FilePath(c_simple_guest_as_pathbuf()), - None, - ) - .unwrap() - .evolve() - .unwrap(); + let mut target = + UninitializedSandbox::new(GuestBinary::FilePath(c_simple_guest_as_pathbuf()), None) + .unwrap() + .evolve() + .unwrap(); assert_eq!(target.call::("StackAllocate", 256i32).unwrap(), 256); target.restore(rust_snapshot).unwrap(); @@ -2425,13 +2413,11 @@ mod tests { #[test] fn snapshot_restore_recovers_poison_with_different_guest() { - let mut source = UninitializedSandbox::new( - GuestBinary::FilePath(c_simple_guest_as_pathbuf()), - None, - ) - .unwrap() - .evolve() - .unwrap(); + let mut source = + UninitializedSandbox::new(GuestBinary::FilePath(c_simple_guest_as_pathbuf()), None) + .unwrap() + .evolve() + .unwrap(); let snapshot = source.snapshot().unwrap(); let path = simple_guest_as_pathbuf(); diff --git a/src/hyperlight_host/tests/wit_test.rs b/src/hyperlight_host/tests/wit_test.rs index 800bf21a1..11d7ab46b 100644 --- a/src/hyperlight_host/tests/wit_test.rs +++ b/src/hyperlight_host/tests/wit_test.rs @@ -20,7 +20,9 @@ use std::sync::{Arc, Mutex}; use hyperlight_common::component::{Negative, Positive}; use hyperlight_common::resource::BorrowedResourceGuard; use hyperlight_host::{GuestBinary, MultiUseSandbox, UninitializedSandbox}; -use hyperlight_testing::{c_simple_guest_as_pathbuf, simple_guest_as_pathbuf, wit_guest_as_pathbuf}; +use hyperlight_testing::{ + c_simple_guest_as_pathbuf, simple_guest_as_pathbuf, wit_guest_as_pathbuf, +}; extern crate alloc; mod bindings { @@ -362,13 +364,11 @@ mod wit_test { #[test] fn restore_rust_and_c_snapshots_replace_wit_guest() { - let mut rust_source = UninitializedSandbox::new( - GuestBinary::FilePath(simple_guest_as_pathbuf()), - None, - ) - .unwrap() - .evolve() - .unwrap(); + let mut rust_source = + UninitializedSandbox::new(GuestBinary::FilePath(simple_guest_as_pathbuf()), None) + .unwrap() + .evolve() + .unwrap(); assert_eq!(rust_source.call::("AddToStatic", 42i32).unwrap(), 42); let rust_snapshot = rust_source.snapshot().unwrap(); @@ -382,13 +382,11 @@ mod wit_test { rust_target.sb.restore(rust_snapshot).unwrap(); assert_eq!(rust_target.sb.call::("GetStatic", ()).unwrap(), 42); - let mut c_source = UninitializedSandbox::new( - GuestBinary::FilePath(c_simple_guest_as_pathbuf()), - None, - ) - .unwrap() - .evolve() - .unwrap(); + let mut c_source = + UninitializedSandbox::new(GuestBinary::FilePath(c_simple_guest_as_pathbuf()), None) + .unwrap() + .evolve() + .unwrap(); assert_eq!(c_source.call::("StackAllocate", 256i32).unwrap(), 256); let c_snapshot = c_source.snapshot().unwrap(); @@ -408,13 +406,11 @@ mod wit_test { #[test] fn restore_chain_replaces_each_guest() { - let mut rust_source = UninitializedSandbox::new( - GuestBinary::FilePath(simple_guest_as_pathbuf()), - None, - ) - .unwrap() - .evolve() - .unwrap(); + let mut rust_source = + UninitializedSandbox::new(GuestBinary::FilePath(simple_guest_as_pathbuf()), None) + .unwrap() + .evolve() + .unwrap(); assert_eq!(rust_source.call::("AddToStatic", 42i32).unwrap(), 42); let rust_snapshot = rust_source.snapshot().unwrap();