diff --git a/boxlite/src/bin/shim/main.rs b/boxlite/src/bin/shim/main.rs index 9932737a..7b15936b 100644 --- a/boxlite/src/bin/shim/main.rs +++ b/boxlite/src/bin/shim/main.rs @@ -14,7 +14,7 @@ mod crash_capture; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::thread; use std::time::Duration; @@ -49,8 +49,20 @@ struct ShimArgs { /// /// This contains the full InstanceSpec including rootfs path, volumes, /// networking, guest entrypoint, and other runtime configuration. - #[arg(long)] - config: String, + #[arg( + long, + conflicts_with = "config_file", + required_unless_present = "config_file" + )] + config: Option, + + /// Path to Box configuration JSON file + #[arg( + long = "config-file", + conflicts_with = "config", + required_unless_present = "config" + )] + config_file: Option, } /// Initialize tracing with file logging. @@ -85,9 +97,8 @@ fn main() -> BoxliteResult<()> { // VmmKind parsed via FromStr trait automatically let args = ShimArgs::parse(); - // Parse InstanceSpec from JSON - let config: InstanceSpec = serde_json::from_str(&args.config) - .map_err(|e| BoxliteError::Engine(format!("Failed to parse config JSON: {}", e)))?; + // Parse InstanceSpec from --config-file (preferred) or --config. + let config = load_instance_spec(&args)?; // Initialize logging using box_dir derived from exit_file path. // Logs go to box_dir/logs/ so the sandbox only needs write access to box_dir. @@ -125,6 +136,35 @@ fn main() -> BoxliteResult<()> { }) } +fn load_instance_spec(args: &ShimArgs) -> BoxliteResult { + if let Some(config_file) = &args.config_file { + let config_json = std::fs::read_to_string(config_file).map_err(|e| { + BoxliteError::Engine(format!( + "Failed to read config file {}: {}", + config_file.display(), + e + )) + })?; + + return serde_json::from_str(&config_json).map_err(|e| { + BoxliteError::Engine(format!( + "Failed to parse config JSON from {}: {}", + config_file.display(), + e + )) + }); + } + + if let Some(config_json) = &args.config { + return serde_json::from_str(config_json) + .map_err(|e| BoxliteError::Engine(format!("Failed to parse config JSON: {}", e))); + } + + Err(BoxliteError::Engine( + "Either --config-file or --config must be provided".to_string(), + )) +} + fn run_shim(args: ShimArgs, mut config: InstanceSpec) -> BoxliteResult<()> { tracing::debug!( shares = ?config.fs_shares.shares(), @@ -394,3 +434,117 @@ fn start_parent_watchdog() { std::process::exit(137); // 128 + 9 (SIGKILL) }); } + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::Value; + use std::io::Write; + + fn test_instance_spec_json() -> String { + serde_json::json!({ + "box_id": "box-test", + "cpus": null, + "memory_mib": null, + "fs_shares": { "shares": [] }, + "block_devices": { "devices": [] }, + "guest_entrypoint": { + "executable": "/boxlite/bin/boxlite-guest", + "args": ["--listen", "vsock://2695"], + "env": [] + }, + "transport": { "Unix": { "socket_path": "/tmp/box.sock" } }, + "ready_transport": { "Unix": { "socket_path": "/tmp/ready.sock" } }, + "guest_rootfs": { + "path": "/tmp/rootfs", + "strategy": "Direct", + "kernel": null, + "initrd": null, + "env": [] + }, + "network_config": null, + "home_dir": "/tmp/home", + "console_output": "/tmp/console.log", + "exit_file": "/tmp/exit", + "detach": false, + "parent_pid": 1 + }) + .to_string() + } + + #[test] + fn shim_args_accepts_legacy_config_flag() { + let args = + ShimArgs::try_parse_from(["boxlite-shim", "--engine", "libkrun", "--config", "{}"]) + .unwrap(); + assert!(args.config.is_some()); + assert!(args.config_file.is_none()); + } + + #[test] + fn shim_args_accepts_config_file_flag() { + let args = ShimArgs::try_parse_from([ + "boxlite-shim", + "--engine", + "libkrun", + "--config-file", + "/tmp/shim-config.json", + ]) + .unwrap(); + assert!(args.config.is_none()); + assert_eq!( + args.config_file.as_deref(), + Some(Path::new("/tmp/shim-config.json")) + ); + } + + #[test] + fn load_instance_spec_from_legacy_config() { + let args = ShimArgs { + engine: VmmKind::Libkrun, + config: Some(test_instance_spec_json()), + config_file: None, + }; + + let parsed = load_instance_spec(&args).unwrap(); + assert_eq!(parsed.box_id, "box-test"); + } + + #[test] + fn load_instance_spec_from_config_file() { + let mut file = tempfile::NamedTempFile::new().unwrap(); + file.write_all(test_instance_spec_json().as_bytes()) + .unwrap(); + + let args = ShimArgs { + engine: VmmKind::Libkrun, + config: None, + config_file: Some(file.path().to_path_buf()), + }; + + let parsed = load_instance_spec(&args).unwrap(); + assert_eq!(parsed.box_id, "box-test"); + } + + #[test] + fn load_instance_spec_from_config_file_with_large_payload() { + let mut json: Value = serde_json::from_str(&test_instance_spec_json()).unwrap(); + let large_env: Vec<(String, String)> = (0..2000) + .map(|i| (format!("KEY_{i}"), "x".repeat(128))) + .collect(); + json["guest_entrypoint"]["env"] = serde_json::json!(large_env); + + let mut file = tempfile::NamedTempFile::new().unwrap(); + file.write_all(json.to_string().as_bytes()).unwrap(); + + let args = ShimArgs { + engine: VmmKind::Libkrun, + config: None, + config_file: Some(file.path().to_path_buf()), + }; + + let parsed = load_instance_spec(&args).unwrap(); + assert_eq!(parsed.box_id, "box-test"); + assert_eq!(parsed.guest_entrypoint.env.len(), 2000); + } +} diff --git a/boxlite/src/litebox/box_impl.rs b/boxlite/src/litebox/box_impl.rs index fc8338f3..587b5d06 100644 --- a/boxlite/src/litebox/box_impl.rs +++ b/boxlite/src/litebox/box_impl.rs @@ -203,20 +203,23 @@ impl BoxImpl { let live = self.live_state().await?; - // Inject container ID into environment if not already set - let command = if command - .env - .as_ref() - .map(|env| env.iter().any(|(k, _)| k == executor_const::ENV_VAR)) - .unwrap_or(false) - { - command - } else { - command.env( + // Inject container ID into environment if not already set. + let mut command = command; + if effective_env_value(&command, executor_const::ENV_VAR).is_none() { + command = command.env( executor_const::ENV_VAR, format!("{}={}", executor_const::CONTAINER_KEY, self.container_id()), - ) - }; + ); + } + + // For explicit guest execution, merge box-level defaults at exec-time. + // Command-level env entries must take precedence. + if matches!( + effective_env_value(&command, executor_const::ENV_VAR), + Some(v) if v == executor_const::GUEST + ) { + command = merge_box_env_for_guest_exec(command, &self.config.options.env); + } // Set working directory from BoxOptions if not set in command let command = match (&command.working_dir, &self.config.options.working_dir) { @@ -647,6 +650,31 @@ impl crate::runtime::backend::BoxBackend for BoxImpl { } } +fn effective_env_value<'a>(command: &'a BoxCommand, key: &str) -> Option<&'a str> { + command.env.as_ref().and_then(|env| { + env.iter() + .rev() + .find(|(k, _)| k == key) + .map(|(_, v)| v.as_str()) + }) +} + +fn merge_box_env_for_guest_exec( + mut command: BoxCommand, + box_env: &[(String, String)], +) -> BoxCommand { + if box_env.is_empty() { + return command; + } + + let mut merged_env = box_env.to_vec(); + if let Some(command_env) = command.env.take() { + merged_env.extend(command_env); + } + command.env = Some(merged_env); + command +} + fn build_tar_from_host( src: &std::path::Path, tar_path: &std::path::Path, diff --git a/boxlite/src/litebox/init/tasks/guest_entrypoint.rs b/boxlite/src/litebox/init/tasks/guest_entrypoint.rs index 8269cfda..d170727b 100644 --- a/boxlite/src/litebox/init/tasks/guest_entrypoint.rs +++ b/boxlite/src/litebox/init/tasks/guest_entrypoint.rs @@ -92,20 +92,11 @@ impl GuestEntrypointBuilder { /// Add an env var, using FILO semantics (later calls override earlier ones). /// - /// If a var with the same key exists, it's removed and its space is reclaimed - /// before adding the new value. + /// If a var with the same key exists, replacement is atomic: when the new value + /// is invalid or doesn't fit, the old value is preserved. /// /// Returns `true` if added, `false` if skipped (logged as warning). pub fn with_env(&mut self, key: &str, value: &str) -> bool { - // FILO: Remove existing key if present, reclaim space - if let Some(pos) = self.env.iter().position(|(k, _)| k == key) { - let (old_key, old_value) = &self.env[pos]; - let old_size = old_key.len() + old_value.len() + Self::ENV_VAR_OVERHEAD; - self.total_size = self.total_size.saturating_sub(old_size); - self.env.remove(pos); - tracing::trace!(env_key = %key, "Overriding existing env var"); - } - // Check ASCII if !is_printable_ascii(key) || !is_printable_ascii(value) { tracing::warn!( @@ -118,11 +109,24 @@ impl GuestEntrypointBuilder { // Check size limit let var_size = key.len() + value.len() + Self::ENV_VAR_OVERHEAD; - if self.total_size + var_size > self.limit { + let existing = self.env.iter().position(|(k, _)| k == key).map(|pos| { + let old_size = { + let (old_key, old_value) = &self.env[pos]; + old_key.len() + old_value.len() + Self::ENV_VAR_OVERHEAD + }; + (pos, old_size) + }); + let projected_total = match existing { + Some((_, old_size)) => self.total_size.saturating_sub(old_size) + var_size, + None => self.total_size + var_size, + }; + + if projected_total > self.limit { tracing::warn!( env_key = %key, env_value = %Self::redact_for_log(value), total_size = self.total_size, + projected_total = projected_total, var_size, limit = self.limit, "Skipping env var: kernel cmdline size limit reached" @@ -130,6 +134,12 @@ impl GuestEntrypointBuilder { return false; } + if let Some((pos, old_size)) = existing { + self.total_size = self.total_size.saturating_sub(old_size); + self.env.remove(pos); + tracing::trace!(env_key = %key, "Overriding existing env var"); + } + self.total_size += var_size; self.env.push((key.to_string(), value.to_string())); tracing::trace!(env_key = %key, var_size, "Added env var"); @@ -184,3 +194,47 @@ impl GuestEntrypointBuilder { } } } + +#[cfg(test)] +mod tests { + use super::GuestEntrypointBuilder; + + #[test] + fn with_env_oversized_override_preserves_existing_value() { + let mut builder = GuestEntrypointBuilder::new("/boxlite/bin/boxlite-guest".to_string()); + assert!(builder.with_env("PATH", "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin")); + + let long_path = "x".repeat(10_000); + assert!(!builder.with_env("PATH", &long_path)); + + let entrypoint = builder.build(); + let path = entrypoint + .env + .iter() + .find(|(k, _)| k == "PATH") + .map(|(_, v)| v.as_str()); + assert_eq!( + path, + Some("/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin") + ); + } + + #[test] + fn with_env_invalid_override_preserves_existing_value() { + let mut builder = GuestEntrypointBuilder::new("/boxlite/bin/boxlite-guest".to_string()); + assert!(builder.with_env("PATH", "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin")); + + assert!(!builder.with_env("PATH", "ok\u{0080}bad")); + + let entrypoint = builder.build(); + let path = entrypoint + .env + .iter() + .find(|(k, _)| k == "PATH") + .map(|(_, v)| v.as_str()); + assert_eq!( + path, + Some("/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin") + ); + } +} diff --git a/boxlite/src/litebox/init/tasks/vmm_spawn.rs b/boxlite/src/litebox/init/tasks/vmm_spawn.rs index 1aae46f3..893ff826 100644 --- a/boxlite/src/litebox/init/tasks/vmm_spawn.rs +++ b/boxlite/src/litebox/init/tasks/vmm_spawn.rs @@ -192,8 +192,7 @@ async fn build_config( let vmm_config = volume_mgr.build_vmm_config(); // Guest entrypoint - let guest_entrypoint = - build_guest_entrypoint(&transport, &ready_transport, &guest_rootfs, options)?; + let guest_entrypoint = build_guest_entrypoint(&transport, &ready_transport, &guest_rootfs)?; // Network configuration let network_config = build_network_config(container_image_config, options, layout); @@ -258,7 +257,6 @@ fn build_guest_entrypoint( transport: &Transport, ready_transport: &Transport, guest_rootfs: &GuestRootfs, - options: &crate::runtime::options::BoxOptions, ) -> BoxliteResult { let listen_uri = transport.to_uri(); let ready_notify_uri = ready_transport.to_uri(); @@ -278,13 +276,11 @@ fn build_guest_entrypoint( builder.with_env("RUST_BACKTRACE", &v); } - // FILO order: image → user (later overrides earlier) + // Guest bootstrap env only. + // Container runtime env (options.env) is delivered via ContainerInit gRPC. for (key, value) in &guest_rootfs.env { builder.with_env(key, value); } - for (key, value) in &options.env { - builder.with_env(key, value); - } Ok(builder.build()) } @@ -349,3 +345,86 @@ async fn spawn_vm( controller.start(config).await } + +#[cfg(test)] +mod tests { + use super::*; + use crate::runtime::guest_rootfs::{GuestRootfs, Strategy}; + use std::path::PathBuf; + + fn test_guest_rootfs(env: Vec<(String, String)>) -> GuestRootfs { + GuestRootfs { + path: PathBuf::from("/tmp/rootfs"), + strategy: Strategy::Direct, + kernel: None, + initrd: None, + env, + } + } + + #[test] + fn guest_entrypoint_includes_guest_rootfs_env() { + let guest_rootfs = test_guest_rootfs(vec![ + ( + "PATH".to_string(), + "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin".to_string(), + ), + ("HOME".to_string(), "/root".to_string()), + ]); + let entrypoint = build_guest_entrypoint( + &Transport::unix(PathBuf::from("/tmp/box.sock")), + &Transport::unix(PathBuf::from("/tmp/ready.sock")), + &guest_rootfs, + ) + .unwrap(); + + assert!(entrypoint.env.iter().any(|(k, v)| { + k == "PATH" && v == "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin" + })); + assert!( + entrypoint + .env + .iter() + .any(|(k, v)| k == "HOME" && v == "/root") + ); + } + + #[test] + fn guest_entrypoint_sets_listen_and_notify_args() { + let guest_rootfs = test_guest_rootfs(vec![]); + let entrypoint = build_guest_entrypoint( + &Transport::unix(PathBuf::from("/tmp/box.sock")), + &Transport::unix(PathBuf::from("/tmp/ready.sock")), + &guest_rootfs, + ) + .unwrap(); + + assert_eq!( + entrypoint.args, + vec![ + "--listen".to_string(), + "unix:///tmp/box.sock".to_string(), + "--notify".to_string(), + "unix:///tmp/ready.sock".to_string(), + ] + ); + } + + #[test] + fn guest_entrypoint_does_not_include_user_container_env() { + let guest_rootfs = test_guest_rootfs(vec![("PATH".to_string(), "/usr/bin".to_string())]); + let entrypoint = build_guest_entrypoint( + &Transport::unix(PathBuf::from("/tmp/box.sock")), + &Transport::unix(PathBuf::from("/tmp/ready.sock")), + &guest_rootfs, + ) + .unwrap(); + + assert!( + entrypoint + .env + .iter() + .any(|(k, v)| k == "PATH" && v == "/usr/bin") + ); + } +} diff --git a/boxlite/src/runtime/layout.rs b/boxlite/src/runtime/layout.rs index 62f19c3c..f852e391 100644 --- a/boxlite/src/runtime/layout.rs +++ b/boxlite/src/runtime/layout.rs @@ -489,6 +489,15 @@ impl BoxFilesystemLayout { self.box_dir.join("exit") } + /// Shim config file path: ~/.boxlite/boxes/{box_id}/shim-config.json + /// + /// Written by the host process before spawning the shim. The shim reads this + /// file via `--config-file` to avoid large command-line payloads. + /// Context: https://github.com/boxlite-ai/boxlite/pull/227 + pub fn shim_config_path(&self) -> PathBuf { + self.box_dir.join("shim-config.json") + } + /// Stderr file path: ~/.boxlite/boxes/{box_id}/shim.stderr /// /// Captures libkrun stderr output for crash diagnostics. @@ -927,4 +936,20 @@ mod tests { assert!(layout.tmp_dir().exists()); } + + #[test] + fn test_shim_config_path_is_under_box_dir() { + let config = FsLayoutConfig::without_bind_mount(); + let layout = BoxFilesystemLayout::new( + PathBuf::from("/home/user/.boxlite/boxes/box-123"), + config, + false, + ); + + let path = layout.shim_config_path(); + assert_eq!( + path, + PathBuf::from("/home/user/.boxlite/boxes/box-123/shim-config.json") + ); + } } diff --git a/boxlite/src/vmm/controller/shim.rs b/boxlite/src/vmm/controller/shim.rs index c8d9fa4c..9769f54a 100644 --- a/boxlite/src/vmm/controller/shim.rs +++ b/boxlite/src/vmm/controller/shim.rs @@ -1,6 +1,6 @@ //! ShimController and ShimHandler - Universal process management for all Box engines. -use std::{path::PathBuf, process::Child, sync::Mutex, time::Instant}; +use std::{io::Write, path::Path, path::PathBuf, process::Child, sync::Mutex, time::Instant}; use crate::{ BoxID, @@ -9,11 +9,7 @@ use crate::{ }; use boxlite_shared::errors::{BoxliteError, BoxliteResult}; -use super::watchdog; -use super::{ - VmmController, VmmHandler as VmmHandlerTrait, VmmMetrics, - spawn::{ShimSpawner, SpawnedShim}, -}; +use super::{VmmController, VmmHandler as VmmHandlerTrait, VmmMetrics, spawn::spawn_subprocess}; // ============================================================================ // SHIM HANDLER - Runtime operations on running VM @@ -31,38 +27,34 @@ pub struct ShimHandler { /// When we spawn the process, we keep the Child to properly wait() on stop. /// When we attach to an existing process, this is None. process: Option, - /// Watchdog keepalive. Dropping closes the pipe write end, delivering - /// POLLHUP to the shim and triggering graceful shutdown. - /// Defense-in-depth: even if `stop()` is never called, dropping the - /// handler closes this, triggering shim cleanup automatically. - #[allow(dead_code)] - keepalive: Option, /// Shared System instance for CPU metrics calculation across calls. /// CPU usage requires comparing snapshots over time, so we must reuse the same System. metrics_sys: Mutex, } impl ShimHandler { - /// Create a handler from a spawned shim. + /// Create a handler for a spawned VM with process ownership. + /// + /// This constructor takes ownership of the Child process handle for proper + /// lifecycle management (clean shutdown with wait()). /// - /// Takes ownership of the `SpawnedShim` (child process + keepalive) for - /// proper lifecycle management. The keepalive keeps the watchdog pipe - /// alive; dropping it triggers shim shutdown. - pub fn from_spawned(spawned: SpawnedShim, box_id: BoxID) -> Self { - let pid = spawned.child.id(); + /// # Arguments + /// * `process` - The spawned subprocess (Child handle) + /// * `box_id` - Box identifier (for logging) + pub fn from_child(process: Child, box_id: BoxID) -> Self { + let pid = process.id(); Self { pid, box_id, - process: Some(spawned.child), - keepalive: spawned.keepalive, + process: Some(process), metrics_sys: Mutex::new(sysinfo::System::new()), } } /// Create a handler for an existing VM (attach mode). /// - /// Used when reconnecting to a running box. We don't have a Child handle - /// or keepalive, so we manage the process by PID only. + /// Used when reconnecting to a running box. We don't have a Child handle, + /// so we manage the process by PID only. /// /// # Arguments /// * `pid` - Process ID of the running VM @@ -72,7 +64,6 @@ impl ShimHandler { pid, box_id, process: None, - keepalive: None, metrics_sys: Mutex::new(sysinfo::System::new()), } } @@ -264,18 +255,6 @@ impl VmmController for ShimController { config.guest_entrypoint.args ); - // Prepare environment with RUST_LOG if present - // Note: We clone the config components needed for subprocess serialization - let mut env = config.guest_entrypoint.env.clone(); - if let Ok(rust_log) = std::env::var("RUST_LOG") { - env.push(("RUST_LOG".to_string(), rust_log.clone())); - } - - // Create a temporary struct for serialization with modified env - // This avoids cloning the config which now contains non-clonable NetworkBackend - let mut guest_entrypoint = config.guest_entrypoint.clone(); - guest_entrypoint.env = env; // Use the modified env with RUST_LOG - let serializable_config = InstanceSpec { // Box identification and security (from ShimController) box_id: self.box_id.to_string(), @@ -285,7 +264,7 @@ impl VmmController for ShimController { memory_mib: config.memory_mib, fs_shares: config.fs_shares.clone(), block_devices: config.block_devices.clone(), - guest_entrypoint, + guest_entrypoint: config.guest_entrypoint.clone(), transport: config.transport.clone(), ready_transport: config.ready_transport.clone(), guest_rootfs: config.guest_rootfs.clone(), @@ -297,9 +276,8 @@ impl VmmController for ShimController { detach: config.detach, }; - // Serialize the config for passing to subprocess - let config_json = serde_json::to_string(&serializable_config) - .map_err(|e| BoxliteError::Engine(format!("Failed to serialize config: {}", e)))?; + let config_path = self.layout.shim_config_path(); + write_instance_spec_file(&serializable_config, &config_path)?; // Clean up stale socket file if it exists (defense in depth) // Only relevant for Unix sockets @@ -317,22 +295,22 @@ impl VmmController for ShimController { "Starting Box subprocess" ); tracing::debug!(binary = %self.binary_path.display(), "Box runner binary"); - tracing::trace!(config = %config_json, "Box configuration"); + tracing::trace!(config_path = %config_path.display(), "Box configuration file"); // Measure subprocess spawn time let shim_spawn_start = Instant::now(); - let spawner = ShimSpawner::new( + let child = spawn_subprocess( &self.binary_path, self.engine_type, + &config_path, &self.layout, self.box_id.as_str(), &self.options, - ); - let spawned = spawner.spawn(&config_json, config.detach)?; + )?; // spawn_duration: time to create Box subprocess let shim_spawn_duration = shim_spawn_start.elapsed(); - let pid = spawned.child.id(); + let pid = child.id(); tracing::info!( box_id = %self.box_id, pid = pid, @@ -344,8 +322,9 @@ impl VmmController for ShimController { // GuestConnectTask handles waiting for guest readiness, // which allows reusing that task across spawn/restart/reconnect. - // Create handler from spawned shim (takes ownership of child + keepalive) - let handler = ShimHandler::from_spawned(spawned, self.box_id.clone()); + // Create handler for the running VM + // Note: stdio is null (no pipes), so no LogStreamHandler needed + let handler = ShimHandler::from_child(child, self.box_id.clone()); tracing::info!( box_id = %self.box_id, @@ -357,3 +336,197 @@ impl VmmController for ShimController { Ok(Box::new(handler)) } } + +fn write_instance_spec_file(config: &InstanceSpec, config_path: &Path) -> BoxliteResult<()> { + let config_json = serde_json::to_vec(config) + .map_err(|e| BoxliteError::Engine(format!("Failed to serialize shim config: {}", e)))?; + + let config_dir = config_path.parent().ok_or_else(|| { + BoxliteError::Storage(format!( + "Invalid shim config path (missing parent): {}", + config_path.display() + )) + })?; + + let mut temp_file = tempfile::NamedTempFile::new_in(config_dir).map_err(|e| { + BoxliteError::Storage(format!( + "Failed to create temporary shim config in {}: {}", + config_dir.display(), + e + )) + })?; + + temp_file.write_all(&config_json).map_err(|e| { + BoxliteError::Storage(format!( + "Failed to write temporary shim config {}: {}", + config_path.display(), + e + )) + })?; + temp_file.flush().map_err(|e| { + BoxliteError::Storage(format!( + "Failed to flush temporary shim config {}: {}", + config_path.display(), + e + )) + })?; + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + temp_file + .as_file() + .set_permissions(std::fs::Permissions::from_mode(0o600)) + .map_err(|e| { + BoxliteError::Storage(format!( + "Failed to set permissions on temporary shim config {}: {}", + config_path.display(), + e + )) + })?; + } + + temp_file.persist(config_path).map_err(|e| { + BoxliteError::Storage(format!( + "Failed to persist shim config {}: {}", + config_path.display(), + e.error + )) + })?; + + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::jailer::SecurityOptions; + use crate::runtime::guest_rootfs::{GuestRootfs, Strategy}; + use crate::vmm::{BlockDevices, Entrypoint, FsShares}; + use std::sync::{Mutex, OnceLock}; + use std::thread; + + fn test_instance_spec() -> InstanceSpec { + InstanceSpec { + box_id: "box-test".to_string(), + security: SecurityOptions::default(), + cpus: None, + memory_mib: None, + fs_shares: FsShares::default(), + block_devices: BlockDevices::default(), + guest_entrypoint: Entrypoint { + executable: "/boxlite/bin/boxlite-guest".to_string(), + args: vec!["--listen".to_string(), "vsock://2695".to_string()], + env: vec![("RUST_LOG".to_string(), "info".to_string())], + }, + transport: boxlite_shared::Transport::unix(PathBuf::from("/tmp/box.sock")), + ready_transport: boxlite_shared::Transport::unix(PathBuf::from("/tmp/ready.sock")), + guest_rootfs: GuestRootfs { + path: PathBuf::from("/tmp/rootfs"), + strategy: Strategy::Direct, + kernel: None, + initrd: None, + env: vec![], + }, + network_config: None, + network_backend_endpoint: None, + home_dir: PathBuf::from("/tmp/home"), + console_output: Some(PathBuf::from("/tmp/console.log")), + exit_file: PathBuf::from("/tmp/exit"), + detach: false, + } + } + + #[test] + fn write_instance_spec_file_persists_json() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("shim-config.json"); + let spec = test_instance_spec(); + + write_instance_spec_file(&spec, &path).unwrap(); + + let content = std::fs::read_to_string(&path).unwrap(); + let parsed: InstanceSpec = serde_json::from_str(&content).unwrap(); + assert_eq!(parsed.box_id, "box-test"); + assert_eq!( + parsed.guest_entrypoint.executable, + "/boxlite/bin/boxlite-guest" + ); + } + + fn fd_test_lock() -> std::sync::MutexGuard<'static, ()> { + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(|| Mutex::new(())).lock().unwrap() + } + + fn count_open_fds() -> usize { + #[cfg(target_os = "linux")] + { + if let Ok(entries) = std::fs::read_dir("/proc/self/fd") { + return entries.count(); + } + } + + std::fs::read_dir("/dev/fd") + .map(|entries| entries.count()) + .unwrap_or(0) + } + + #[test] + fn write_instance_spec_file_concurrent_writes_are_isolated() { + let dir = tempfile::tempdir().unwrap(); + let mut handles = Vec::new(); + let mut expected = Vec::new(); + + for i in 0..16 { + let box_id = format!("box-concurrent-{i}"); + let path = dir.path().join(format!("shim-config-{i}.json")); + let path_for_thread = path.clone(); + let box_id_for_thread = box_id.clone(); + expected.push((box_id, path)); + + handles.push(thread::spawn(move || { + let mut spec = test_instance_spec(); + spec.box_id = box_id_for_thread; + write_instance_spec_file(&spec, &path_for_thread).unwrap(); + })); + } + + for handle in handles { + handle.join().unwrap(); + } + + for (expected_box_id, path) in expected { + let content = std::fs::read_to_string(&path).unwrap(); + let parsed: InstanceSpec = serde_json::from_str(&content).unwrap(); + assert_eq!(parsed.box_id, expected_box_id); + assert_eq!( + parsed.guest_entrypoint.executable, + "/boxlite/bin/boxlite-guest" + ); + } + } + + #[test] + fn write_instance_spec_file_persist_failure_does_not_leak_fds() { + let _guard = fd_test_lock(); + let dir = tempfile::tempdir().unwrap(); + let target_dir = dir.path().join("target-as-dir"); + std::fs::create_dir_all(&target_dir).unwrap(); + let spec = test_instance_spec(); + + let baseline = count_open_fds(); + for _ in 0..50 { + let err = write_instance_spec_file(&spec, &target_dir).unwrap_err(); + let msg = err.to_string(); + assert!(msg.contains("Failed to persist shim config"), "{msg}"); + } + let after = count_open_fds(); + + assert!( + after <= baseline + 4, + "FD count grew unexpectedly: baseline={baseline}, after={after}" + ); + } +} diff --git a/boxlite/src/vmm/controller/spawn.rs b/boxlite/src/vmm/controller/spawn.rs index 0feb8e32..fafb35de 100644 --- a/boxlite/src/vmm/controller/spawn.rs +++ b/boxlite/src/vmm/controller/spawn.rs @@ -13,275 +13,107 @@ use crate::vmm::VmmKind; use boxlite_shared::errors::{BoxliteError, BoxliteResult}; use libkrun_sys::krun_create_ctx; -use super::watchdog; - -/// A shim that was spawned, with its child process handle and optional keepalive. -/// -/// The `keepalive` holds the parent side of the watchdog pipe. While it exists, -/// the shim's watchdog thread blocks on `poll()`. Dropping it closes the pipe -/// write end, delivering POLLHUP to the shim and triggering graceful shutdown. -pub struct SpawnedShim { - pub child: Child, - /// Parent-side watchdog keepalive. Dropping triggers shim shutdown. - /// `None` for detached boxes (no watchdog). - pub keepalive: Option, -} - -/// Spawns `boxlite-shim` with full isolation, environment, and watchdog. +/// Spawns a subprocess with jailer isolation. /// -/// Composes: Jailer (isolation) + watchdog (lifecycle) + env/stdio setup. +/// # Arguments +/// * `binary_path` - Path to the boxlite-shim binary +/// * `engine_type` - Type of VM engine to use +/// * `config_path` - Path to serialized InstanceSpec JSON +/// * `layout` - Box filesystem layout (provides paths for box directory, stderr, etc.) +/// * `box_id` - Unique box identifier +/// * `options` - Box options (includes security and volumes) /// -/// # Fields -/// -/// Stable inputs grouped into the struct; variable inputs (`config_json`, `detach`) -/// are passed to [`spawn()`](Self::spawn). -pub struct ShimSpawner<'a> { - binary_path: &'a Path, +/// # Returns +/// * `Ok(Child)` - Successfully spawned subprocess +/// * `Err(...)` - Failed to spawn subprocess +pub(crate) fn spawn_subprocess( + binary_path: &Path, engine_type: VmmKind, - layout: &'a BoxFilesystemLayout, - box_id: &'a str, - options: &'a BoxOptions, -} - -impl<'a> ShimSpawner<'a> { - pub fn new( - binary_path: &'a Path, - engine_type: VmmKind, - layout: &'a BoxFilesystemLayout, - box_id: &'a str, - options: &'a BoxOptions, - ) -> Self { - Self { - binary_path, - engine_type, - layout, - box_id, - options, - } - } - - /// Spawn the shim subprocess with jailer isolation and optional watchdog. - /// - /// When `detach` is false, creates a watchdog pipe so the shim detects - /// parent death via POLLHUP. When `detach` is true, no watchdog is created. - /// - /// # Returns - /// * `SpawnedShim` containing the child process and optional keepalive - pub fn spawn(&self, config_json: &str, detach: bool) -> BoxliteResult { - // 1. Create watchdog pipe (non-detached only) - let (keepalive, child_setup) = if !detach { - let (k, s) = watchdog::create()?; - (Some(k), Some(s)) - } else { - (None, None) - }; - - // 2. Build jailer with optional FD preservation for watchdog pipe - let mut builder = JailerBuilder::new() - .with_box_id(self.box_id) - .with_layout(self.layout.clone()) - .with_security(self.options.advanced.security.clone()) - .with_volumes(self.options.volumes.clone()); - - if let Some(ref setup) = child_setup { - builder = builder.with_preserved_fd(setup.raw_fd(), watchdog::PIPE_FD); - } - - let jail = builder.build()?; - - // 3. Setup pre-spawn isolation (cgroups on Linux, no-op on macOS) - jail.prepare()?; - - // 4. Build isolated command (includes pre_exec hook) - let shim_args = self.build_shim_args(config_json); - let mut cmd = jail.command(self.binary_path, &shim_args); - - // 5. Configure environment - self.configure_env(&mut cmd); - - // 6. Configure stdio (stdin/stdout=null, stderr=file) - let stderr_file = self.create_stderr_file()?; - cmd.stdin(Stdio::null()); - cmd.stdout(Stdio::null()); - cmd.stderr(Stdio::from(stderr_file)); - - // 7. Spawn - let child = cmd.spawn().map_err(|e| { - let err_msg = format!( - "Failed to spawn VM subprocess at {}: {}", - self.binary_path.display(), - e - ); - tracing::error!("{}", err_msg); - BoxliteError::Engine(err_msg) - })?; - - // 8. Close read end in parent (child inherited it via fork) - drop(child_setup); - - Ok(SpawnedShim { child, keepalive }) + config_path: &Path, + layout: &BoxFilesystemLayout, + box_id: &str, + options: &BoxOptions, +) -> BoxliteResult { + // Build shim arguments + let shim_args = build_shim_args(engine_type, config_path); + + // Build jailer with security options and volumes + let jailer = JailerBuilder::new() + .with_box_id(box_id) + .with_layout(layout.clone()) + .with_security(options.advanced.security.clone()) + .with_volumes(options.volumes.clone()) + .build() + .map_err(BoxliteError::from)?; + + // Setup pre-spawn isolation (cgroups on Linux, no-op on macOS) + jailer.prepare()?; + + // Build isolated command (includes pre_exec FD cleanup hook) + let mut cmd = jailer.command(binary_path, &shim_args); + + // Pass debugging environment variables to subprocess + if let Ok(rust_log) = std::env::var("RUST_LOG") { + cmd.env("RUST_LOG", rust_log); } - - fn build_shim_args(&self, config_json: &str) -> Vec { - vec![ - "--engine".to_string(), - format!("{:?}", self.engine_type), - "--config".to_string(), - config_json.to_string(), - ] + if let Ok(rust_backtrace) = std::env::var("RUST_BACKTRACE") { + cmd.env("RUST_BACKTRACE", rust_backtrace); } - fn configure_env(&self, cmd: &mut std::process::Command) { - // Pass debugging environment variables to subprocess - if let Ok(rust_log) = std::env::var("RUST_LOG") { - cmd.env("RUST_LOG", rust_log); - } - if let Ok(rust_backtrace) = std::env::var("RUST_BACKTRACE") { - cmd.env("RUST_BACKTRACE", rust_backtrace); - } - - // Keep temp artifacts inside the box-scoped allowlist when using the - // built-in macOS seatbelt profile. libkrun may create a transient - // `krun-empty-root-*` under `env::temp_dir()` when booting from block - // devices; under deny-default seatbelt this must resolve to an - // explicitly granted path. - if self.options.advanced.security.jailer_enabled - && self.options.advanced.security.sandbox_profile.is_none() - { - let tmp_dir = self.layout.tmp_dir(); - cmd.env("TMPDIR", &tmp_dir); - cmd.env("TMP", &tmp_dir); - cmd.env("TEMP", &tmp_dir); - } - - // Set library search paths for bundled dependencies - configure_library_env(cmd, krun_create_ctx as *const libc::c_void); - } + // Set library search paths for bundled dependencies + configure_library_env(&mut cmd, krun_create_ctx as *const libc::c_void); + + // Create stderr file BEFORE spawn to capture ALL errors including pre-main dyld errors. + // This is critical: dyld errors happen before main() and would go to /dev/null otherwise. + let stderr_file_path = layout.stderr_file_path(); + let stderr_file = std::fs::File::create(&stderr_file_path).map_err(|e| { + BoxliteError::Storage(format!( + "Failed to create stderr file {}: {}", + stderr_file_path.display(), + e + )) + })?; + + // Use null for stdin/stdout to support detach/reattach without pipe issues. + // - stdin: prevents libkrun from affecting parent's stdin + // - stdout: prevents SIGPIPE when LogStreamHandler is dropped on detach + // - stderr: captured to file for crash diagnostics + cmd.stdin(Stdio::null()); + cmd.stdout(Stdio::null()); + cmd.stderr(Stdio::from(stderr_file)); + + cmd.spawn().map_err(|e| { + let err_msg = format!( + "Failed to spawn VM subprocess at {}: {}", + binary_path.display(), + e + ); + tracing::error!("{}", err_msg); + BoxliteError::Engine(err_msg) + }) +} - fn create_stderr_file(&self) -> BoxliteResult { - // Create stderr file BEFORE spawn to capture ALL errors including pre-main dyld errors. - // This is critical: dyld errors happen before main() and would go to /dev/null otherwise. - let stderr_file_path = self.layout.stderr_file_path(); - std::fs::File::create(&stderr_file_path).map_err(|e| { - BoxliteError::Storage(format!( - "Failed to create stderr file {}: {}", - stderr_file_path.display(), - e - )) - }) - } +fn build_shim_args(engine_type: VmmKind, config_path: &Path) -> Vec { + vec![ + "--engine".to_string(), + format!("{:?}", engine_type), + "--config-file".to_string(), + config_path.display().to_string(), + ] } #[cfg(test)] mod tests { use super::*; - use std::ffi::OsStr; #[test] - fn test_build_shim_args() { - use crate::runtime::layout::{BoxFilesystemLayout, FsLayoutConfig}; - use std::path::PathBuf; + fn build_shim_args_uses_config_file_flag() { + let args = build_shim_args(VmmKind::Libkrun, Path::new("/tmp/shim-config.json")); - let layout = BoxFilesystemLayout::new( - PathBuf::from("/tmp/box"), - FsLayoutConfig::without_bind_mount(), - false, + assert!( + args.windows(2) + .any(|w| w == ["--config-file", "/tmp/shim-config.json"]) ); - let options = BoxOptions::default(); - - let spawner = ShimSpawner::new( - Path::new("/usr/bin/boxlite-shim"), - VmmKind::Libkrun, - &layout, - "test-box", - &options, - ); - - let args = spawner.build_shim_args("{\"test\":true}"); - assert_eq!(args.len(), 4); - assert_eq!(args[0], "--engine"); - assert_eq!(args[1], "Libkrun"); - assert_eq!(args[2], "--config"); - assert_eq!(args[3], "{\"test\":true}"); - } - - #[test] - fn test_configure_env_sets_box_scoped_temp_dir() { - use crate::runtime::layout::{BoxFilesystemLayout, FsLayoutConfig}; - use std::path::PathBuf; - - let layout = BoxFilesystemLayout::new( - PathBuf::from("/tmp/box"), - FsLayoutConfig::without_bind_mount(), - false, - ); - let options = BoxOptions::default(); - - let spawner = ShimSpawner::new( - Path::new("/usr/bin/boxlite-shim"), - VmmKind::Libkrun, - &layout, - "test-box", - &options, - ); - - let mut cmd = std::process::Command::new("/usr/bin/true"); - spawner.configure_env(&mut cmd); - - let envs: std::collections::HashMap<_, _> = cmd.get_envs().collect(); - let expected = layout.tmp_dir(); - - assert_eq!( - envs.get(OsStr::new("TMPDIR")).and_then(|v| *v), - Some(expected.as_os_str()) - ); - assert_eq!( - envs.get(OsStr::new("TMP")).and_then(|v| *v), - Some(expected.as_os_str()) - ); - assert_eq!( - envs.get(OsStr::new("TEMP")).and_then(|v| *v), - Some(expected.as_os_str()) - ); - } - - #[test] - fn test_configure_env_does_not_override_temp_for_custom_profile() { - use crate::runtime::advanced_options::{AdvancedBoxOptions, SecurityOptions}; - use crate::runtime::layout::{BoxFilesystemLayout, FsLayoutConfig}; - use std::path::PathBuf; - - let layout = BoxFilesystemLayout::new( - PathBuf::from("/tmp/box"), - FsLayoutConfig::without_bind_mount(), - false, - ); - let options = BoxOptions { - advanced: AdvancedBoxOptions { - security: SecurityOptions { - jailer_enabled: true, - sandbox_profile: Some(PathBuf::from("/tmp/custom.sbpl")), - ..SecurityOptions::default() - }, - ..AdvancedBoxOptions::default() - }, - ..BoxOptions::default() - }; - - let spawner = ShimSpawner::new( - Path::new("/usr/bin/boxlite-shim"), - VmmKind::Libkrun, - &layout, - "test-box", - &options, - ); - - let mut cmd = std::process::Command::new("/usr/bin/true"); - spawner.configure_env(&mut cmd); - - let envs: std::collections::HashMap<_, _> = cmd.get_envs().collect(); - assert!(!envs.contains_key(OsStr::new("TMPDIR"))); - assert!(!envs.contains_key(OsStr::new("TMP"))); - assert!(!envs.contains_key(OsStr::new("TEMP"))); + assert!(!args.iter().any(|arg| arg == "--config")); } } diff --git a/boxlite/tests/env_delivery.rs b/boxlite/tests/env_delivery.rs new file mode 100644 index 00000000..e7934093 --- /dev/null +++ b/boxlite/tests/env_delivery.rs @@ -0,0 +1,134 @@ +//! Integration tests for container environment delivery. +//! +//! These tests verify end-to-end behavior: +//! 1) User-provided OCI env values are visible inside the box. +//! 2) Large env payloads are delivered via config-file/gRPC path without +//! breaking guest startup. + +use boxlite::runtime::options::{BoxOptions, BoxliteOptions, RootfsSpec}; +use boxlite::{BoxCommand, BoxliteRuntime}; +use futures::StreamExt; +use tempfile::TempDir; + +struct TestContext { + runtime: BoxliteRuntime, + _temp_dir: TempDir, +} + +impl TestContext { + fn new() -> Self { + // Keep paths short on macOS to avoid UNIX socket length limits. + let temp_dir = TempDir::new_in("/tmp").expect("Failed to create temp dir"); + let options = BoxliteOptions { + home_dir: temp_dir.path().to_path_buf(), + image_registries: vec![], + }; + let runtime = BoxliteRuntime::new(options).expect("Failed to create runtime"); + Self { + runtime, + _temp_dir: temp_dir, + } + } +} + +async fn run_and_capture_stdout( + handle: &boxlite::LiteBox, + cmd: BoxCommand, +) -> (String, boxlite::ExecResult) { + let mut exec = handle.exec(cmd).await.expect("exec failed"); + let mut stdout = exec.stdout().expect("stdout stream not available"); + let mut output = String::new(); + while let Some(chunk) = stdout.next().await { + output.push_str(&chunk); + } + let result = exec.wait().await.expect("wait failed"); + (output, result) +} + +#[tokio::test] +async fn env_is_visible_inside_box() { + let ctx = TestContext::new(); + + let handle = ctx + .runtime + .create( + BoxOptions { + rootfs: RootfsSpec::Image("alpine:latest".into()), + env: vec![("E2E_ENV_VISIBLE".to_string(), "visible-value".to_string())], + auto_remove: false, + ..Default::default() + }, + None, + ) + .await + .expect("Failed to create box"); + + let (stdout, result) = run_and_capture_stdout( + &handle, + BoxCommand::new("sh").args(["-lc", "printf '%s' \"$E2E_ENV_VISIBLE\""]), + ) + .await; + + assert!(result.success(), "command failed: {:?}", result); + assert_eq!(stdout, "visible-value"); + + handle.stop().await.expect("stop failed"); + ctx.runtime + .remove(handle.id().as_str(), false) + .await + .expect("remove failed"); +} + +#[tokio::test] +async fn large_env_payload_is_delivered_inside_box() { + let ctx = TestContext::new(); + + let mut env = Vec::new(); + for i in 0..300usize { + let key = format!("E2E_LARGE_ENV_{i:04}"); + let value = format!("VAL_{i:04}_{}", "x".repeat(220)); + env.push((key, value)); + } + env.push(( + "E2E_LARGE_ENV_SENTINEL".to_string(), + "sentinel-ok".to_string(), + )); + + let expected_tail = env + .iter() + .find(|(k, _)| k == "E2E_LARGE_ENV_0299") + .map(|(_, v)| v.clone()) + .expect("missing tail env"); + + let handle = ctx + .runtime + .create( + BoxOptions { + rootfs: RootfsSpec::Image("alpine:latest".into()), + env, + auto_remove: false, + ..Default::default() + }, + None, + ) + .await + .expect("Failed to create box"); + + let (stdout, result) = run_and_capture_stdout( + &handle, + BoxCommand::new("sh").args([ + "-lc", + "printf '%s|%s' \"$E2E_LARGE_ENV_SENTINEL\" \"$E2E_LARGE_ENV_0299\"", + ]), + ) + .await; + + assert!(result.success(), "command failed: {:?}", result); + assert_eq!(stdout, format!("sentinel-ok|{expected_tail}")); + + handle.stop().await.expect("stop failed"); + ctx.runtime + .remove(handle.id().as_str(), false) + .await + .expect("remove failed"); +}