From cd67747404f78f71dead4f58489313b4ed78575c Mon Sep 17 00:00:00 2001 From: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Date: Fri, 13 Nov 2026 11:05:56 -0800 Subject: [PATCH] Add a configurable surrogate extraction directory on Windows Read HYPERLIGHT_SURROGATE_EXTRACT_DIR and default to the executable's directory. Resolve it to an absolute path once. Launch surrogates with CreateProcessW so Unicode paths and paths with spaces work. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> --- CHANGELOG.md | 2 + Justfile | 12 ++ .../hyperlight-surrogate-development-notes.md | 4 +- src/hyperlight_host/build.rs | 5 +- .../hypervisor/surrogate_process_manager.rs | 115 +++++++++++++----- .../src/hypervisor/wrappers.rs | 40 ------ 6 files changed, 101 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ca6659619..d2419203c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,9 +5,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Prerelease] - Unreleased ### Added +* Add `HYPERLIGHT_SURROGATE_EXTRACT_DIR` to configure the surrogate extraction directory on Windows. * Add `Sandbox::recreate()`, which rebuilds a sandbox's VM and memory from a snapshot. It recovers unrecoverable sandboxes and keeps configuration, host functions, and interrupt handles by @ludfjig in https://github.com/hyperlight-dev/hyperlight/pull/1751 ### Fixed +* Fix surrogate process creation for Windows paths containing Unicode or spaces. * Fix restore on WHP not rewinding the guest TSC on hosts with `TSC_ADJUST` support. * HVF sandbox initialization accepts seeds that match Apple hypercall identifiers. * Host-written snapshot memory has instruction-cache coherence before guest execution. diff --git a/Justfile b/Justfile index 528fc4df2e..91654125b6 100644 --- a/Justfile +++ b/Justfile @@ -278,6 +278,7 @@ test-loom: # runs tests that requires being run separately, for example due to global state test-isolated target=default-target features="" : + {{ if os() == "windows" { 'just test-surrogate-directory ' + target + ' "' + features + '"' } else { "" } }} {{ cargo-cmd }} test {{ if features =="" {''} else if features=="no-default-features" {"--no-default-features" } else {"--no-default-features -F " + features } }} --profile={{ if target == "debug" { "dev" } else { target } }} {{ target-triple-flag }} -p hyperlight-host --lib -- sandbox::uninitialized::tests::test_log_trace --exact --ignored {{ cargo-cmd }} test {{ if features =="" {''} else if features=="no-default-features" {"--no-default-features" } else {"--no-default-features -F " + features } }} --profile={{ if target == "debug" { "dev" } else { target } }} {{ target-triple-flag }} -p hyperlight-host --lib -- sandbox::outb::tests::test_log_emit_guest_log --exact --ignored {{ cargo-cmd }} test {{ if features =="" {''} else if features=="no-default-features" {"--no-default-features" } else {"--no-default-features -F " + features } }} --profile={{ if target == "debug" { "dev" } else { target } }} {{ target-triple-flag }} -p hyperlight-host --lib -- sandbox::initialized::tests::from_snapshot::max_guest_log_level_is_honored_from_snapshot --exact --ignored @@ -320,6 +321,17 @@ test-compilation-no-default-features target=default-target: {{ if os() == "linux" { cargo-cmd + " check -p hyperlight-host --no-default-features --features kvm" } else { "" } }} {{ target-triple-flag }} {{ if os() == "linux" { cargo-cmd + " check -p hyperlight-host --no-default-features --features mshv3" } else { "" } }} {{ target-triple-flag }} +[windows] +test-surrogate-directory target=default-target features="": + $env:HYPERLIGHT_SURROGATE_EXTRACT_DIR='{{root}}/target/surrogate directory é'; $env:HYPERLIGHT_INITIAL_SURROGATES=1; $env:HYPERLIGHT_MAX_SURROGATES=1; \ + {{ cargo-cmd }} test {{ if features =="" {''} else if features=="no-default-features" {"--no-default-features" } else {"--no-default-features -F " + features } }} --profile={{ if target == "debug" { "dev" } else { target } }} {{ target-triple-flag }} -p hyperlight-host --lib -- hypervisor::surrogate_process_manager::tests::test_surrogate_directory_override --exact --ignored + + $env:HYPERLIGHT_SURROGATE_EXTRACT_DIR='../../target/surrogate-directory-test'; $env:HYPERLIGHT_INITIAL_SURROGATES=0; $env:HYPERLIGHT_MAX_SURROGATES=1; \ + {{ cargo-cmd }} test {{ if features =="" {''} else if features=="no-default-features" {"--no-default-features" } else {"--no-default-features -F " + features } }} --profile={{ if target == "debug" { "dev" } else { target } }} {{ target-triple-flag }} -p hyperlight-host --lib -- hypervisor::surrogate_process_manager::tests::test_surrogate_directory_relative --exact --ignored + + $env:HYPERLIGHT_SURROGATE_EXTRACT_DIR='{{root}}/target/surrogate-directory-test/missing'; \ + {{ cargo-cmd }} test {{ if features =="" {''} else if features=="no-default-features" {"--no-default-features" } else {"--no-default-features -F " + features } }} --profile={{ if target == "debug" { "dev" } else { target } }} {{ target-triple-flag }} -p hyperlight-host --lib -- hypervisor::surrogate_process_manager::tests::test_surrogate_directory_missing --exact --ignored + # runs a subset of existing tests with HYPERLIGHT_MAX_SURROGATES=0 (Windows only). # Covers guest calls, host callbacks, virtqueue buffers and errors, # in-memory snapshot/restore, and save/load from disk. diff --git a/docs/hyperlight-surrogate-development-notes.md b/docs/hyperlight-surrogate-development-notes.md index 431cc85da1..8341950a18 100644 --- a/docs/hyperlight-surrogate-development-notes.md +++ b/docs/hyperlight-surrogate-development-notes.md @@ -8,4 +8,6 @@ These surrogate processes are managed by the host via the [surrogate_process_man > **Note:** `HYPERLIGHT_MAX_SURROGATES` is authoritative — if `HYPERLIGHT_INITIAL_SURROGATES` exceeds it, the initial count is silently clamped down to the maximum. For example, setting only `HYPERLIGHT_MAX_SURROGATES=256` limits both the initial pool and the ceiling to 256. -`hyperlight_surrogate.exe` gets built during `hyperlight-host`'s build script, gets embedded into the `hyperlight-host` Rust library via [rust-embed](https://crates.io/crates/rust-embed), and is extracted at runtime next to the executable when the surrogate process manager is initialized. The extracted filename includes a short BLAKE3 hash of the binary content (e.g., `hyperlight_surrogate_a1b2c3d4.exe`) so that multiple hyperlight versions can coexist without file-deletion races. +`hyperlight_surrogate.exe` gets built during `hyperlight-host`'s build script, gets embedded into the `hyperlight-host` Rust library via [rust-embed](https://crates.io/crates/rust-embed), and is extracted at runtime, by default next to the executable, when the surrogate process manager is initialized. The extracted filename includes a short BLAKE3 hash of the binary content (e.g., `hyperlight_surrogate_a1b2c3d4.exe`) so that multiple hyperlight versions can coexist without file-deletion races. + +Set `HYPERLIGHT_SURROGATE_EXTRACT_DIR` before creating the first sandbox to choose the extraction directory. The directory must exist and allow writing and running the surrogate executable. Relative paths are resolved when the manager initializes. Initialization failures are logged, and sandbox creation fails. diff --git a/src/hyperlight_host/build.rs b/src/hyperlight_host/build.rs index 3ec2fe181e..654ad7a793 100644 --- a/src/hyperlight_host/build.rs +++ b/src/hyperlight_host/build.rs @@ -13,10 +13,7 @@ fn main() -> Result<()> { let out_path = std::path::PathBuf::from(&out_dir); let manifest_dir = std::env::var("CARGO_MANIFEST_DIR")?; - // Windows requires the hyperlight_surrogate.exe binary to be next to the executable running - // hyperlight. We are using rust-embed to include the binary in the hyperlight-host library - // and then extracting it at runtime when the surrogate process manager starts. We need to pass - // the location of the binary to the rust build. + // Embed hyperlight_surrogate.exe for extraction when the surrogate process manager starts. // This logic runs when targeting Windows, even if cross-compiling from Linux. if std::env::var("CARGO_CFG_TARGET_OS")? == "windows" { println!("cargo:rerun-if-changed=src/hyperlight_surrogate/src/main.rs"); diff --git a/src/hyperlight_host/src/hypervisor/surrogate_process_manager.rs b/src/hyperlight_host/src/hypervisor/surrogate_process_manager.rs index f0af4f155b..5c622ce3a9 100644 --- a/src/hyperlight_host/src/hypervisor/surrogate_process_manager.rs +++ b/src/hyperlight_host/src/hypervisor/surrogate_process_manager.rs @@ -20,12 +20,12 @@ use windows::Win32::System::JobObjects::{ JobObjectExtendedLimitInformation, SetInformationJobObject, TerminateJobObject, }; use windows::Win32::System::Threading::{ - CREATE_SUSPENDED, CreateProcessA, PROCESS_INFORMATION, STARTUPINFOA, + CREATE_SUSPENDED, CreateProcessW, PROCESS_INFORMATION, STARTUPINFOW, }; -use windows::core::PCSTR; +use windows::core::{HSTRING, PCSTR}; use super::surrogate_process::SurrogateProcess; -use super::wrappers::{HandleWrapper, PSTRWrapper}; +use super::wrappers::HandleWrapper; use crate::HyperlightError::WindowsAPIError; use crate::{Result, log_then_return, new_error}; @@ -58,6 +58,10 @@ const INITIAL_SURROGATES_ENV_VAR: &str = "HYPERLIGHT_INITIAL_SURROGATES"; /// Defaults to that limit if unset. const MAX_SURROGATES_ENV_VAR: &str = "HYPERLIGHT_MAX_SURROGATES"; +/// Environment variable overriding the runtime surrogate extraction directory. +/// Defaults to the current executable's directory. +const SURROGATE_EXTRACT_DIR_ENV_VAR: &str = "HYPERLIGHT_SURROGATE_EXTRACT_DIR"; + /// Returns the on-disk filename for the surrogate binary, incorporating the /// first 8 hex characters of the BLAKE3 hash of the embedded binary so that /// different hyperlight versions produce different filenames and can coexist @@ -194,8 +198,8 @@ impl SurrogateProcessManager { #[instrument(err(Debug), skip_all, parent = Span::current(), level= "Trace")] fn new() -> Result { let binary_name = surrogate_binary_name()?; - ensure_surrogate_process_exe(&binary_name)?; let surrogate_process_path = get_surrogate_process_dir()?.join(&binary_name); + ensure_surrogate_process_exe(&surrogate_process_path)?; let (initial, max) = surrogate_process_counts(); @@ -393,6 +397,10 @@ fn create_job_object() -> Result { #[instrument(err(Debug), skip_all, parent = Span::current(), level= "Trace")] fn get_surrogate_process_dir() -> Result { + if let Some(dir) = std::env::var_os(SURROGATE_EXTRACT_DIR_ENV_VAR) { + return Ok(std::path::absolute(dir)?); + } + let binding = std::env::current_exe()?; let path = binding .parent() @@ -413,10 +421,7 @@ fn get_surrogate_process_dir() -> Result { /// creating the file, the partial file is deleted so future runs do not /// mistake it for a valid binary. #[instrument(err(Debug), skip_all, parent = Span::current(), level= "Trace")] -fn ensure_surrogate_process_exe(binary_name: &str) -> Result<()> { - let dir = get_surrogate_process_dir()?; - let surrogate_process_path = dir.join(binary_name); - +fn ensure_surrogate_process_exe(surrogate_process_path: &Path) -> Result<()> { // Resolve the embedded asset before touching the filesystem so a // missing asset can't leave a zero-byte ghost file on disk. let exe = Asset::get(EMBEDDED_SURROGATE_NAME) @@ -426,12 +431,11 @@ fn ensure_surrogate_process_exe(binary_name: &str) -> Result<()> { // returns `AlreadyExists` and we skip extraction. The filename // embeds the content hash, so an existing file is guaranteed to // have the correct content. - match File::create_new(&surrogate_process_path) { + match File::create_new(surrogate_process_path) { Ok(mut f) => { info!( - "{} does not exist, extracting to {}", - binary_name, - &surrogate_process_path.display() + "extracting surrogate to {}", + surrogate_process_path.display() ); if let Err(e) = f.write_all(exe.data.as_ref()) { @@ -440,7 +444,7 @@ fn ensure_surrogate_process_exe(binary_name: &str) -> Result<()> { // Drop the file handle first — on Windows, open handles // prevent deletion. drop(f); - let _ = std::fs::remove_file(&surrogate_process_path); + let _ = std::fs::remove_file(surrogate_process_path); return Err(e.into()); } } @@ -465,23 +469,20 @@ fn create_surrogate_process( surrogate_process_path: &Path, job_handle: HandleWrapper, ) -> Result { - let mut process_information: PROCESS_INFORMATION = unsafe { std::mem::zeroed() }; - let mut startup_info: STARTUPINFOA = unsafe { std::mem::zeroed() }; - let process_attributes: SECURITY_ATTRIBUTES = Default::default(); - let thread_attributes: SECURITY_ATTRIBUTES = Default::default(); - startup_info.cb = std::mem::size_of::() as u32; - - let cmd_line = surrogate_process_path.to_str().ok_or(new_error!( - "failed to convert surrogate process path to a string" - ))?; - let p_cmd_line = &PSTRWrapper::try_from(cmd_line)?; + let mut process_information = PROCESS_INFORMATION::default(); + let startup_info = STARTUPINFOW { + cb: size_of::() as u32, + ..Default::default() + }; + let application = HSTRING::from(surrogate_process_path); + // SAFETY: all pointer arguments refer to locals that outlive the call. if let Err(e) = unsafe { - CreateProcessA( - PCSTR::null(), - Some(p_cmd_line.into()), - Some(&process_attributes), - Some(&thread_attributes), + CreateProcessW( + &application, + None, + None, + None, false, CREATE_SUSPENDED, None, @@ -495,6 +496,7 @@ fn create_surrogate_process( let job_handle: HANDLE = job_handle.into(); let process_handle: HANDLE = process_information.hProcess; + // SAFETY: both handles refer to live objects created by this manager. unsafe { if let Err(e) = AssignProcessToJobObject(job_handle, process_handle) { log_then_return!(WindowsAPIError(e.clone())); @@ -519,6 +521,56 @@ mod tests { use super::*; use crate::mem::shared_mem::{ExclusiveSharedMemory, SharedMemory}; + + #[test] + #[ignore = "Run by `just test-surrogate-directory` with isolated process-wide configuration"] + fn test_surrogate_directory_override() { + let dir = PathBuf::from(std::env::var_os(SURROGATE_EXTRACT_DIR_ENV_VAR).unwrap()); + let directory_name = dir.file_name().unwrap().to_str().unwrap(); + assert!(directory_name.contains('é')); + assert!(directory_name.contains(' ')); + std::fs::create_dir_all(&dir).unwrap(); + + let manager = get_surrogate_process_manager().unwrap(); + let path = dir.join(surrogate_binary_name().unwrap()); + assert_eq!(manager.surrogate_process_path, path); + assert_eq!( + std::fs::read(path).unwrap(), + Asset::get(EMBEDDED_SURROGATE_NAME).unwrap().data.as_ref() + ); + let _process = manager.get_surrogate_process().unwrap(); + } + + #[test] + #[ignore = "Run by `just test-surrogate-directory` with isolated process-wide configuration"] + fn test_surrogate_directory_relative() { + let dir = PathBuf::from(std::env::var_os(SURROGATE_EXTRACT_DIR_ENV_VAR).unwrap()); + assert!(dir.is_relative()); + std::fs::create_dir_all(&dir).unwrap(); + let path = std::path::absolute(dir) + .unwrap() + .join(surrogate_binary_name().unwrap()); + let manager = get_surrogate_process_manager().unwrap(); + assert_eq!(manager.surrogate_process_path, path); + assert_eq!(manager.created_count.load(Ordering::Acquire), 0); + + let original_dir = std::env::current_dir().unwrap(); + let other_dir = tempfile::tempdir().unwrap(); + std::env::set_current_dir(other_dir.path()).unwrap(); + let process = manager.get_surrogate_process(); + std::env::set_current_dir(original_dir).unwrap(); + let _process = process.unwrap(); + } + + #[test] + #[ignore = "Run by `just test-surrogate-directory` with isolated process-wide configuration"] + fn test_surrogate_directory_missing() { + let dir = PathBuf::from(std::env::var_os(SURROGATE_EXTRACT_DIR_ENV_VAR).unwrap()); + assert!(!dir.exists()); + assert!(get_surrogate_process_manager().is_err()); + assert!(!dir.exists()); + } + #[test] fn test_surrogate_process_manager() { let mut threads = Vec::new(); @@ -816,7 +868,7 @@ mod tests { let _ = std::fs::remove_file(&path); // --- First call: extracts the binary --- - ensure_surrogate_process_exe(test_binary_name).expect("first call should succeed"); + ensure_surrogate_process_exe(&path).expect("first call should succeed"); assert!(path.exists(), "binary should exist after extraction"); // --- Verify extracted content matches embedded binary --- @@ -829,15 +881,14 @@ mod tests { ); // --- Second call: file exists, should skip (AlreadyExists path) --- - ensure_surrogate_process_exe(test_binary_name) + ensure_surrogate_process_exe(&path) .expect("second call should succeed when file already exists"); // --- Delete and re-extract --- std::fs::remove_file(&path).expect("should be able to delete test binary"); assert!(!path.exists(), "binary should be gone after deletion"); - ensure_surrogate_process_exe(test_binary_name) - .expect("should succeed re-extracting after deletion"); + ensure_surrogate_process_exe(&path).expect("should succeed re-extracting after deletion"); assert!(path.exists(), "binary should be re-created after deletion"); // Clean up test artifact. diff --git a/src/hyperlight_host/src/hypervisor/wrappers.rs b/src/hyperlight_host/src/hypervisor/wrappers.rs index 394ddc3de5..95114280c0 100644 --- a/src/hyperlight_host/src/hypervisor/wrappers.rs +++ b/src/hyperlight_host/src/hypervisor/wrappers.rs @@ -1,47 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2025 The Hyperlight Authors. -use std::ffi::CString; - -use tracing::{Span, instrument}; use windows::Win32::Foundation::{HANDLE, HMODULE}; -use windows::core::PSTR; - -use crate::{HyperlightError, Result}; - -/// A wrapper for `windows::core::PSTR` values that ensures memory for the -/// underlying string is properly dropped. -#[derive(Debug)] -pub(super) struct PSTRWrapper(*mut i8); - -impl TryFrom<&str> for PSTRWrapper { - type Error = HyperlightError; - #[instrument(err(Debug), skip_all, parent = Span::current(), level= "Trace")] - fn try_from(value: &str) -> Result { - let c_str = CString::new(value)?; - Ok(Self(c_str.into_raw())) - } -} - -impl Drop for PSTRWrapper { - #[instrument(skip_all, parent = Span::current(), level= "Trace")] - fn drop(&mut self) { - let cstr = unsafe { CString::from_raw(self.0) }; - drop(cstr); - } -} - -/// Convert a `WindowsStringWrapper` into a `PSTR`. -/// -/// # Safety -/// The returned `PSTR` must not outlive the origin `WindowsStringWrapper` -impl From<&PSTRWrapper> for PSTR { - #[instrument(skip_all, parent = Span::current(), level= "Trace")] - fn from(value: &PSTRWrapper) -> Self { - let raw = value.0; - PSTR::from_raw(raw as *mut u8) - } -} /// Wrapper for HANDLE, required since HANDLE is no longer Send. #[derive(Debug, Copy, Clone, PartialEq, Eq)]