From ad7dc8352909f53cadd09f9623addf4a6e8c56b2 Mon Sep 17 00:00:00 2001 From: Soheil Alizadeh Date: Sat, 14 Mar 2026 11:25:34 +0100 Subject: [PATCH 1/3] Harden VM lifecycle behavior and reconcile review findings --- CAPABILITIES.md | 177 +++++++ CLAUDE-REVIEW.md | 1075 ++++++++++++++++++++++++++++++++++++++ COMBINED-REVIEW.md | 735 ++++++++++++++++++++++++++ codex-review.md | 109 ++++ src/config.rs | 119 ++++- src/driver/apple_vz.rs | 146 ++---- src/driver/boot.rs | 16 + src/driver/cloud_hv.rs | 74 +-- src/driver/mod.rs | 51 ++ src/driver/whp.rs | 3 +- src/ffi/apple_vz/base.rs | 20 +- src/ffi/apple_vz/mod.rs | 1 + src/lib.rs | 4 +- src/network/bridge.rs | 19 + src/network/mod.rs | 3 + src/network/switch.rs | 54 +- src/oci/registry.rs | 2 + src/oci/store.rs | 25 +- src/setup/image.rs | 17 +- src/setup/seed.rs | 21 +- src/vm.rs | 121 ++++- tests/ffi_smoke.rs | 211 +------- tests/vm_lifecycle.rs | 4 + tests/vm_manager.rs | 49 +- 24 files changed, 2626 insertions(+), 430 deletions(-) create mode 100644 CAPABILITIES.md create mode 100644 CLAUDE-REVIEW.md create mode 100644 COMBINED-REVIEW.md create mode 100644 codex-review.md diff --git a/CAPABILITIES.md b/CAPABILITIES.md new file mode 100644 index 0000000..1674ed4 --- /dev/null +++ b/CAPABILITIES.md @@ -0,0 +1,177 @@ +# vm-rs Capability Matrix + +Cross-platform VM lifecycle management. +macOS via Apple Virtualization.framework, Linux via Cloud Hypervisor. + +## Platform Support + +| Capability | macOS (Apple VZ) | Linux (Cloud Hypervisor) | Notes | +|---|---|---|---| +| **Lifecycle** | | | | +| Boot VM | Supported | Supported | Kernel + initramfs + root disk | +| Stop (graceful) | Supported | Supported | ACPI shutdown | +| Kill (force) | Supported | Supported | Immediate termination | +| Query state | Supported | Supported | Starting → Running → Stopped / Failed | +| Reboot | Not supported | Planned | CH: `vm.reboot` or `ch-remote reboot` | +| Delete/cleanup | Supported | Supported | Remove VM state and resources | +| **CPU** | | | | +| Set vCPU count | Supported | Supported | At boot time | +| CPU hotplug | Not possible | Planned | CH: `vm.resize --cpus N` (up to max_vcpus) | +| CPU topology | Not possible | Planned | Sockets / cores / threads | +| CPU affinity | Not possible | Planned | Pin vCPUs to host cores | +| **Memory** | | | | +| Set memory size | Supported | Supported | At boot time | +| Memory balloon | Supported (FFI wrapped) | Planned | Dynamic guest memory reclaim | +| Memory hotplug | Not possible | Planned | CH: `vm.resize --memory N` | +| Hugepages | Not possible | Planned | 2MB / 1GB pages | +| Memory zones (NUMA) | Not possible | Planned | Per-zone backing and pinning | +| **Storage** | | | | +| Root disk | Supported | Supported | VirtioBlock | +| Seed ISO (cloud-init) | Supported | Supported | Second disk, read-only | +| Data disk | Supported | Supported | Additional VirtioBlock | +| Disk hotplug | Not possible | Planned | CH: `vm.add-disk` | +| Disk resize | Not possible | Planned | CH: `vm.resize-disk` | +| Disk CoW clone | Supported (APFS) | Supported (reflink) | Instant base image cloning | +| Disk caching modes | Supported | Planned | Uncached / Cached / Auto | +| Disk rate limiting | Not possible | Planned | Token bucket throttle | +| **Networking** | | | | +| NAT (internet access) | Supported | N/A | macOS: built-in NAT attachment | +| TAP devices | N/A | Supported | Linux: standard TAP networking | +| Socketpair (L2 switch) | Supported | N/A | macOS: userspace Ethernet switch | +| Custom MAC address | Supported | Supported | Per-VM unique MAC | +| Network hotplug | Not possible | Planned | CH: `vm.add-net` | +| Bridged networking | Supported (FFI wrapped) | Planned | Direct host NIC bridge | +| vhost-user net | Not possible | Planned | Offload to external daemon | +| **Shared Directories** | | | | +| VirtioFS | Supported (in-process) | Planned | macOS: native VZ. Linux: virtiofsd sidecar | +| Read-only mounts | Supported | Planned | Immutable shared data | +| **Serial Console** | | | | +| File output | Supported | Supported | Log serial to file | +| Readiness detection | Supported | Supported | Parse `8STACK_READY` marker from console | +| **Entropy** | | | | +| VirtIO RNG | Supported | Supported | /dev/random in guest | +| **Advanced (API mode)** | | | | +| Pause / Resume | Not possible | Planned | CH: `vm.pause` / `vm.resume` | +| Snapshot | Not possible | Planned | CH: `vm.snapshot` — full state dump | +| Restore | Not possible | Planned | CH: `vm.restore` — resume from snapshot | +| Live migration | Not possible | Planned | CH: `vm.send-migration` / `vm.receive-migration` | +| VM counters/metrics | Not possible | Planned | CH: `vm.counters` | +| **Device Passthrough** | | | | +| VFIO (GPU, NIC, etc.) | Not possible | Planned | CH: `vm.add-device` | +| vDPA | Not possible | Planned | Hardware-accelerated virtio | +| **Inter-VM Communication** | | | | +| vsock | Not possible | Planned | CH: CID-based VM-to-VM / VM-to-host | +| L2 switch (userspace) | Supported | N/A | macOS: learning bridge via socketpairs | +| **Platform-Specific** | | | | +| Rosetta (x86 on ARM) | Not wrapped | N/A | Apple Silicon: run x86_64 Linux binaries | +| macOS guest boot | Not wrapped | N/A | VZMacOSBootLoader | +| UEFI boot | Not wrapped | Planned | EFI variable store + firmware | +| TPM | Not wrapped | Planned | Virtual Trusted Platform Module | +| Watchdog | N/A | Planned | Guest hang recovery | +| Graphics/Display | Not wrapped | Not planned | Headless VMs only | +| Audio | Not wrapped | Not planned | Not needed for server VMs | + +## Implementation Status + +**Supported** = works now, tested. +**Planned** = the underlying platform supports it; we haven't wired it yet. +**Not possible** = the platform does not offer this capability. +**Not wrapped** = Apple VZ framework supports it, but our FFI bindings don't expose it yet. +**Not planned** = technically possible but not in scope (server-oriented crate). + +## Driver Architecture + +### macOS: Apple Virtualization.framework + +In-process VM management. The VZ framework runs inside our process via Objective-C FFI. +VMs are created as `VZVirtualMachine` objects on a dispatch queue. We use `Box::leak` +to keep them alive (ObjC reference counting requires the object to outlive the queue). + +Capabilities are fixed at boot — no hotplug, no resize, no snapshot. +The framework is designed for desktop virtualization (macOS guests, Linux guests with GUI). +We use only the headless Linux server subset. + +### Linux: Cloud Hypervisor + +Separate VMM process. We spawn `cloud-hypervisor` and manage the process lifecycle. + +**Two modes:** + +1. **CLI mode** (current) — boot with `cloud-hypervisor --kernel ... --cpus ... --disk ...`. + Stop = SIGTERM (graceful ACPI), Kill = SIGKILL, State = process alive check. + Simple, no dependencies, covers basic lifecycle. + +2. **API mode** (future) — boot with `cloud-hypervisor --api-socket /path/to/sock`. + Control via `ch-remote` CLI or direct HTTP over Unix socket. + Required for: hotplug, resize, snapshot, restore, live migration, pause/resume. + +We start with CLI mode and add API mode when advanced features are needed. +The `VmDriver` trait is designed to accommodate both. + +## Cloud Hypervisor API Reference + +For advanced features (API mode), Cloud Hypervisor exposes these endpoints +via HTTP over Unix socket. The `ch-remote` CLI wraps all of them. + +| Endpoint | Method | Purpose | +|---|---|---| +| `vmm.ping` | GET | Health check | +| `vmm.shutdown` | PUT | Kill VMM process | +| `vm.create` | PUT | Create VM from JSON config | +| `vm.boot` | PUT | Start created VM | +| `vm.shutdown` | PUT | Graceful ACPI shutdown | +| `vm.reboot` | PUT | Guest OS restart | +| `vm.delete` | PUT | Remove VM | +| `vm.info` | GET | Query VM state and config | +| `vm.pause` | PUT | Suspend execution | +| `vm.resume` | PUT | Resume from pause | +| `vm.resize` | PUT | Change CPU count or memory size | +| `vm.resize-disk` | PUT | Expand disk | +| `vm.resize-zone` | PUT | Expand memory zone | +| `vm.add-disk` | PUT | Hotplug block device | +| `vm.add-net` | PUT | Hotplug NIC | +| `vm.add-fs` | PUT | Hotplug VirtioFS mount | +| `vm.add-device` | PUT | Hotplug VFIO device | +| `vm.add-vsock` | PUT | Hotplug vsock | +| `vm.add-pmem` | PUT | Hotplug persistent memory | +| `vm.add-vdpa` | PUT | Hotplug vDPA device | +| `vm.add-user-device` | PUT | Hotplug userspace device | +| `vm.remove-device` | PUT | Hot-unplug device | +| `vm.snapshot` | PUT | Snapshot full VM state | +| `vm.restore` | PUT | Resume from snapshot | +| `vm.send-migration` | PUT | Live migration (outbound) | +| `vm.receive-migration` | PUT | Live migration (inbound) | +| `vm.counters` | GET | Performance metrics | +| `vm.power-button` | PUT | Simulate power button | +| `vm.nmi` | PUT | Non-maskable interrupt | +| `vm.coredump` | PUT | Debug memory dump (x86_64) | + +## Apple VZ FFI Coverage + +Our FFI bindings (absorbed from virtualization-rs, MIT license) wrap: + +| VZ Class | Wrapped | Used by Driver | +|---|---|---| +| VZLinuxBootLoader | Yes | Yes | +| VZVirtualMachineConfiguration | Yes | Yes | +| VZVirtualMachine | Yes | Yes | +| VZVirtioBlockDeviceConfiguration | Yes | Yes | +| VZDiskImageStorageDeviceAttachment | Yes | Yes | +| VZVirtioNetworkDeviceConfiguration | Yes | Yes | +| VZNATNetworkDeviceAttachment | Yes | Yes | +| VZFileHandleNetworkDeviceAttachment | Yes | Yes | +| VZBridgedNetworkDeviceAttachment | Yes | No | +| VZVirtioConsoleDeviceSerialPortConfiguration | Yes | Yes | +| VZFileHandleSerialPortAttachment | Yes | Yes | +| VZVirtioEntropyDeviceConfiguration | Yes | Yes | +| VZVirtioTraditionalMemoryBalloonDeviceConfiguration | Yes | Yes | +| VZVirtioFileSystemDeviceConfiguration | Yes | Yes | +| VZSharedDirectory | Yes | Yes | +| VZMACAddress | Yes | Yes | +| VZEFIBootLoader | No | — | +| VZMacOSBootLoader | No | — | +| VZGraphicsDeviceConfiguration | No | — | +| VZAudioDeviceConfiguration | No | — | +| VZUSBDeviceConfiguration | No | — | +| VZTPMDeviceConfiguration | No | — | +| VZLinuxRosettaDirectoryShare | No | — | diff --git a/CLAUDE-REVIEW.md b/CLAUDE-REVIEW.md new file mode 100644 index 0000000..3fac732 --- /dev/null +++ b/CLAUDE-REVIEW.md @@ -0,0 +1,1075 @@ +# vm-rs Deep Review + +Comprehensive review covering security, reliability, observability, correctness, +silent fallbacks, unsafe code, untyped code, logging, naming, code smell, SOLID +principles, and user experience. + +--- + +## CRITICAL + +### 1. WHP driver won't compile — `VmHandle` struct mismatch + +**`src/driver/whp.rs:375-382`** + +The WHP driver constructs `VmHandle` with fields `pid` and `machine_id` that +don't exist on the actual `VmHandle` struct (which has `process: Option` +and no `machine_id`). This means the WHP driver cannot compile alongside the rest +of the crate. + +```rust +// whp.rs constructs: +VmHandle { pid: None, machine_id: None, .. } + +// config.rs actually defines: +pub struct VmHandle { + pub process: Option, // not `pid` + // no `machine_id` field at all +} +``` + +**Fix:** Update the `VmHandle` construction in `whp.rs` to use the correct field +names from `config.rs`. + +--- + +### 2. WHP driver has `pause()`/`resume()` not in the `VmDriver` trait + +**`src/driver/whp.rs:448-506`** + +`pause()` and `resume()` are implemented inside `impl VmDriver for WhpDriver`, but +these methods don't exist in the `VmDriver` trait definition at `driver/mod.rs:20-40`. +This is a compile error. + +**Fix:** Either extend the `VmDriver` trait with optional `pause`/`resume` methods +(with default "not supported" implementations), move these to a separate +`PausableDriver` trait, or put them on a `WhpDriver`-specific impl block. + +--- + +### 3. `VmState::Paused` variant doesn't exist + +**`src/driver/whp.rs:469`** + +The WHP driver writes `VmState::Paused` but `VmState` only has four variants: +`Starting`, `Running`, `Stopped`, `Failed`. This is a compile error. + +**Fix:** Either add a `Paused` variant to `VmState` or map the paused state to an +existing variant. + +--- + +### 4. WHP driver not cfg-gated and missing dependencies + +**`src/driver/mod.rs`** only conditionally compiles `apple_vz` (macOS) and +`cloud_hv` (Linux). The `whp.rs` and `boot.rs` modules are not declared in +`mod.rs` at all, and there is no `#[cfg(target_os = "windows")]` gate. The +`windows` crate is not listed in `Cargo.toml`. Additionally, `boot.rs` is imported +by `whp.rs` via `use super::boot` but `boot` is never declared as a submodule of +`driver`. + +On macOS/Linux builds the `whp.rs` file is simply not compiled because it is not +declared as a module — but this means the code is entirely dead and untested. + +**Fix:** +1. Add `pub mod boot;` (unconditional or cfg-gated) and + `#[cfg(target_os = "windows")] pub mod whp;` to `driver/mod.rs`. +2. Add the `windows` crate to `[target.'cfg(target_os = "windows")'.dependencies]` + in `Cargo.toml`. +3. Wire `WhpDriver` into `create_platform_driver()` in `vm.rs`. + +--- + +### 5. `SwitchPort` leaks file descriptors on `add_port` error paths + +**`src/network/switch.rs:96-120`** + +`create_socketpair()` returns two raw FDs. If the lock acquisition on line 103-106 +fails after the socketpair is created, both FDs leak. The `switch_fd` is stored as +a bare `RawFd` in `SwitchPort` — never wrapped in `OwnedFd` — so it's only closed +manually in `NetworkSwitch::Drop`. If the `SwitchPort` is never inserted into the +map (lock poisoned), the switch-side FD leaks permanently. + +```rust +let (switch_fd, vm_fd) = create_socketpair()?; +// If this lock() fails, switch_fd is leaked: +let mut networks = self.networks.lock() + .map_err(|e| io::Error::other(...))?; +``` + +**Fix:** Wrap both FDs in `OwnedFd` immediately after `create_socketpair()` returns, +then extract the raw fd for storage only after successful insertion. + +--- + +### 6. `download_file` writes data non-atomically + +**`src/setup/image.rs:303`** + +```rust +std::fs::write(path, &bytes).map_err(SetupError::Io)?; +``` + +Writes directly to the target path. If the process is killed mid-write, the result +is a partial file that passes the existence check on the next run but is corrupt. +The OCI store's `put_blob` at `store.rs:114` has the same issue. + +**Fix:** Write to a temporary file in the same directory, then `fsync` + atomic +`rename` to the final path. + +--- + +## HIGH + +### 7. `memory_mb * 1024 * 1024` integer overflow on 32-bit targets + +**`src/driver/apple_vz.rs:96`, `src/driver/whp.rs:182`** + +```rust +let memory_bytes = config.memory_mb * 1024 * 1024; +``` + +`config.memory_mb` is `usize`. On a 32-bit target, this overflows for any value +≥ 4096 MB. Even on 64-bit targets, there is no validation that the result is +reasonable. + +**Fix:** Use `u64` for `memory_mb` and `memory_bytes`, or add `.checked_mul()` with +an error on overflow. Also validate that `memory_mb > 0` and `cpus > 0`. + +--- + +### 8. No `VmConfig` validation at the manager level + +**`src/vm.rs:59`** + +`VmManager::start()` performs no config validation before delegating to the driver. +Basic invariants that should be checked up front: +- `name` is empty +- `cpus == 0` +- `memory_mb == 0` +- `kernel` path is empty +- `name` contains path separators (path traversal risk — see finding #9) + +Each driver re-discovers these problems in its own way (or doesn't), leading to +inconsistent error messages and potentially unsafe behavior. + +**Fix:** Add a `VmConfig::validate()` method called in `VmManager::start()` before +any filesystem or driver operations. + +--- + +### 9. VM name used unsanitized in path construction — path traversal + +**`src/vm.rs:54-56`** + +```rust +pub fn vm_dir(&self, name: &str) -> PathBuf { + self.base_dir.join(name) +} +``` + +If `name` is `"../../../etc"`, this results in path traversal outside `base_dir`. +The name comes directly from `VmConfig.name` which is user-supplied. Also used in +`cloud_hv.rs` for socket paths, serial log parent directories, and CString labels. + +**Fix:** Validate that `name` matches a safe pattern (e.g., `^[a-zA-Z0-9][a-zA-Z0-9._-]*$`) +in `VmConfig::validate()` or `VmManager::start()`. + +--- + +### 10. `check_ready_marker` reads the entire serial log on every poll + +**`src/driver/apple_vz.rs:422-438` and `src/driver/cloud_hv.rs:519-536`** + +Both drivers have identical `check_ready_marker()` functions that call +`fs::read_to_string()` on every `state()` query. For VMs with verbose serial +output, this reads megabytes from disk every second. + +The `watch_for_ready()` function in `apple_vz.rs:445-511` does incremental reading +but is public and unused — it's not connected to the main state-check path. + +**Fix:** Use incremental reading (track last-read offset) for the state-polling +path. The existing `watch_for_ready()` function demonstrates the correct approach. + +--- + +### 11. `clone_disk` silently succeeds when target exists + +**`src/vm.rs:249-252`** + +```rust +if target.exists() { + tracing::debug!(..., "disk clone target already exists, skipping"); + return Ok(()); +} +``` + +If the target disk exists from a previous crashed clone (partial/corrupt file), +`clone_disk` returns `Ok(())` without checking file integrity. The VM will boot +from a corrupt disk. + +**Fix:** At minimum, compare file sizes with the base image. Ideally, let the caller +choose between skip-if-exists and overwrite semantics. + +--- + +### 12. `DefaultHasher` for MAC generation is not stable across Rust versions + +**`src/driver/cloud_hv.rs:443-456`** + +```rust +use std::collections::hash_map::DefaultHasher; +let mut hasher = DefaultHasher::new(); +name.hash(&mut hasher); +``` + +`DefaultHasher` is explicitly documented as not guaranteed to produce the same +output across Rust versions. Using it for deterministic MAC generation means the +same VM name can get different MAC addresses after a Rust toolchain update, silently +breaking networking and DHCP leases. + +**Fix:** Use a hash with a stability guarantee (e.g., truncate a SHA-256 hash or use +a simple FNV/SipHash with a fixed seed). + +--- + +### 13. Page tables only support up to 1 GB of guest memory + +**`src/driver/boot.rs:76`** + +```rust +let num_pages = memory_mb.div_ceil(2).min(512); // Max 512 = 1GB +``` + +A single Page Directory can only hold 512 entries (2 MB each = 1 GB). If the user +configures `memory_mb: 4096` (4 GB), the page tables only identity-map the first +1 GB. The kernel will crash when it accesses memory above 1 GB. + +**Fix:** Either add multiple PD tables (one per PDPT entry, supporting up to 512 GB) +or validate that `memory_mb <= 1024` and return a clear error. + +--- + +## MEDIUM + +### 14. Duplicated `check_ready_marker` function + +**`src/driver/apple_vz.rs:422-438` vs `src/driver/cloud_hv.rs:519-536`** + +These functions are character-for-character identical. Violates DRY and means bug +fixes need to be applied in two places. + +**Fix:** Extract to a shared utility in `driver/mod.rs`: +```rust +pub(crate) fn check_ready_marker(log_path: &Path) -> Option { ... } +``` + +--- + +### 15. `watch_for_ready` is `pub` but unused + +**`src/driver/apple_vz.rs:445`** + +This function is declared `pub` but is not called anywhere in the crate, nor +re-exported from `lib.rs`. It's dead code that increases the public API surface. + +**Fix:** Either remove it, make it `pub(crate)`, or wire it into the driver's state +detection. + +--- + +### 16. `VmManager::wait_all_ready` uses blocking sleep in a potentially async context + +**`src/vm.rs:196-242`** + +This function blocks the calling thread with `thread::sleep(1s)` in a loop. The +crate depends on tokio and users are likely to call this from async code, which +would block the entire tokio runtime thread. + +**Fix:** Provide an `async` version using `tokio::time::sleep`, or at minimum add a +doc comment warning that this is a blocking call. + +--- + +### 17. `NetworkSwitch::add_port` stores raw FD — no RAII + +**`src/network/switch.rs:57-68`** + +`SwitchPort` holds a bare `RawFd`, not an `OwnedFd`. Cleanup is manual in +`Drop for NetworkSwitch`. If the switch is leaked (e.g., `mem::forget`), all FDs +leak. Nothing prevents a dangling FD reference if something else closes the FD. + +**Fix:** Store `OwnedFd` in `SwitchPort` and use `as_raw_fd()` when needed for +syscalls. + +--- + +### 18. `AppleVzDriver::state()` removes VMs from registry as a side effect + +**`src/driver/apple_vz.rs:393-395`** + +```rust +if matches!(state, VmState::Stopped | VmState::Failed { .. }) { + registry.remove(&handle.name); +} +``` + +Querying VM state has the side effect of removing the VM from the registry. This is +surprising behavior (violates Command-Query Separation) and can cause races: if two +threads call `state()` concurrently, the first removes the VM and the second gets +`Stopped` because it's missing — even if the VM was `Running` a moment ago. + +**Fix:** Don't remove VMs in `state()`. Let `stop()`/`kill()` be the only operations +that modify the registry. + +--- + +### 19. No reqwest timeout on HTTP calls + +**`src/setup/image.rs:167`, `src/oci/registry.rs:84-87`** + +The HTTP client in `setup/image.rs` is created with `reqwest::Client::new()` — no +timeout configured. Large image downloads can hang forever if the server stalls. +The OCI registry client sets a `user_agent` but also has no timeout. + +**Fix:** Set `.timeout()` and `.connect_timeout()` on the client builder: +```rust +reqwest::Client::builder() + .connect_timeout(Duration::from_secs(30)) + .timeout(Duration::from_secs(300)) + .build() +``` + +--- + +### 20. `cloud_hv::stop` does not escalate to SIGKILL for restored handles + +**`src/driver/cloud_hv.rs:265-280`** + +In the `stop()` path for a restored handle (no `VmProcess` in the map), +`wait_for_pid_exit` is called but never escalates to SIGKILL on timeout — unlike +`wait_for_exit` which does. So a hung VM restored by handle will wait 10 seconds +and then return `Ok(())` while the process is still alive. + +**Fix:** Add SIGKILL escalation to `wait_for_pid_exit`, or unify with `wait_for_exit`. + +--- + +### 21. `bridge.rs` doesn't validate network interface names + +**`src/network/bridge.rs`** + +`ensure_bridge(name, gateway_ip, subnet_cidr)` passes user-controlled strings to +`Command::new("ip")` and `Command::new("iptables")` as arguments. While `.arg()` +prevents shell injection, malicious `name` values could create unexpectedly-named +interfaces or exceed the Linux 15-character limit silently. + +**Fix:** Validate that `name` matches `^[a-zA-Z0-9_-]{1,15}$`. + +--- + +### 22. No `From` conversions between error types + +`OciError` and `SetupError` cannot be converted to `VmError`. Users who compose OCI +pulls + VM boot must manually map errors: + +```rust +let manifest = pull("nginx", &store).await.map_err(|e| /* manual conversion */)?; +``` + +**Fix:** Add `From for VmError` and `From for VmError`, or use +a unified error type (or `anyhow` at the application boundary). + +--- + +### 23. Unbounded `serial_buffer` growth in WHP driver + +**`src/driver/whp.rs:687`** + +The `serial_buffer` string in `vcpu_loop` accumulates every byte the kernel outputs +via COM1 and is never truncated. A Linux kernel with verbose logging will cause +unbounded memory growth in the host process. + +**Fix:** Once the `VMRS_READY` marker is found, stop accumulating. Or use a circular +buffer / ring buffer with a fixed capacity. + +--- + +### 24. `VmSocketEndpoint` and `NetworkAttachment` are Unix-only but not cfg-gated + +**`src/config.rs:3`** + +`VmSocketEndpoint` uses `std::os::fd::{AsRawFd, OwnedFd}` which doesn't exist on +Windows. The entire `config` module will fail to compile on Windows even though +`VmConfig` itself should be platform-agnostic. + +**Fix:** Gate `VmSocketEndpoint`, `NetworkAttachment::SocketPairFd`, and the fd +imports behind `#[cfg(unix)]`. Consider making `NetworkAttachment` platform-gated +entirely, or use a platform-neutral representation. + +--- + +### 25. `unsanitize_name` has a fragile heuristic + +**`src/oci/store.rs:375-381`** + +```rust +fn unsanitize_name(s: &str) -> String { + let result = s.replace("_slash_", "/").replace("_colon_", ":"); + if result == s && s.contains('_') && !s.contains('/') { + return s.replacen('_', "/", 1); // BUG: turns "my_image" into "my/image" + } + result +} +``` + +The fallback heuristic incorrectly modifies names that legitimately contain +underscores. For example, `"my_image"` is not a sanitized form of anything, but +this function returns `"my/image"`. + +**Fix:** Remove the fallback heuristic. Only unsanitize via the `_slash_`/`_colon_` +markers. If backwards compatibility is needed, add a version marker to the manifest +directory structure. + +--- + +## LOW + +### 26. `VmState` missing `Eq` derive + +**`src/config.rs:148`** + +`VmState` derives `PartialEq` but not `Eq`. Since all variants contain only +`String` (which is `Eq`), it should derive `Eq` for completeness. This prevents +using `VmState` as a `HashMap` key or in contexts requiring `Eq`. + +--- + +### 27. `process` field name is confusing on `VmHandle` + +**`src/config.rs:142`** + +The field is `process: Option` but on macOS it's always `None` because +Apple VZ runs in-process. A more accurate name would be `vmm_identity` or +`process_identity`, matching how it's actually used (PID + start-time for identity +verification). + +--- + +### 28. `lib.rs` doc comment doesn't mention WHP/Windows + +**`src/lib.rs:1-17`** + +The crate-level doc and architecture diagram only mention macOS and Linux. Windows +(WHP) is absent from the documentation despite existing in source. + +--- + +### 29. Logging inconsistency: structured vs. format strings + +Some modules use structured tracing fields consistently: +```rust +tracing::info!(vm = %name, cpus = config.cpus, "booting VM"); +``` + +Others use unstructured format strings: +```rust +tracing::info!("pulling {} from {}", image, image_ref.registry); +``` + +The OCI and setup modules generally use unstructured format strings while the driver +modules use structured fields. Should be consistent across the crate. + +--- + +### 30. `NSFileHandle::file_handle_with_fd` passes `closeOnDealloc: NO` — FD leak + +**`src/ffi/apple_vz/base.rs:171`** + +```rust +msg_send![alloc, initWithFileDescriptor: fd closeOnDealloc: NO] +``` + +The FD is transferred via `into_raw_fd()` (ownership transfer), but the +`NSFileHandle` is told NOT to close it on dealloc. This means the FD will leak when +the Objective-C object is deallocated. The serial console FD and `/dev/null` FD +created in `apple_vz.rs:135-141` are transferred and never closed by anyone. + +**Fix:** Either pass `YES` for `closeOnDealloc` (and ensure nothing else closes +the FD), or track the FD separately and close it manually. + +--- + +### 31. Missing `#[must_use]` on key return types + +`VmHandle`, `VmState`, `PreparedImage`, `ImageManifest` — these are important +return values that should not be silently discarded. Adding `#[must_use]` catches +accidental drops at compile time. + +--- + +### 32. `create_seed_iso` temp dir name collision + +**`src/setup/seed.rs:95`** + +```rust +.join(format!(".seed-{}", config.hostname)); +``` + +If two concurrent processes create seed ISOs for the same hostname, they clobber +each other's temp directory contents. + +**Fix:** Use `tempfile::tempdir_in()` or append a unique suffix (e.g., PID or UUID). + +--- + +### 33. `PortForwarder::stop` calls both `notify_one()` and `abort()` + +**`src/network/port_forward.rs:91-94`** + +```rust +pub fn stop(self) { + self.stop.notify_one(); + self.handle.abort(); +} +``` + +The `abort()` is redundant if the `notify` works, and if it's needed, the `notify` +is useless. The `abort()` without awaiting the handle means the task may not +complete cleanup. + +**Fix:** Use only `notify_one()` and `await` the handle, or use only `abort()`. + +--- + +### 34. `Arch::host()` uses compile-time detection, not runtime + +**`src/setup/image.rs:26-32`** + +```rust +pub fn host() -> Self { + if cfg!(target_arch = "aarch64") { Arch::Aarch64 } else { Arch::X86_64 } +} +``` + +`cfg!` is evaluated at compile time. Cross-compiled binaries report the wrong +architecture. This is standard Rust behavior but the function name `host()` is +misleading — it reports the *target* architecture, not the host. + +--- + +### 35. `dispatch_sync` declared but never used + +**`src/ffi/apple_vz/base.rs:21`** + +`dispatch_sync` is declared in the `extern "C"` block but never called anywhere. +Dead code in FFI declarations. + +--- + +### 36. `clone_disk` is a no-op on unsupported platforms + +**`src/vm.rs:244-298`** + +On platforms other than macOS or Linux, `clone_disk` silently returns `Ok(())` +without doing anything — the `#[cfg]` blocks fall through and nothing executes. + +**Fix:** Add a `#[cfg(not(...))]` block that either returns an error or falls back to +`std::fs::copy`. + +--- + +### 37. `VmManager` is not `Clone` and requires manual `Arc` wrapping + +Users must wrap `VmManager` in `Arc` for multi-threaded use. Since the struct +already uses `RwLock` internally and implements `Send + Sync` via trait object +bounds, this is a recurring ergonomic tax on consumers. + +--- + +### 38. Network modules not platform-gated + +**`src/network/mod.rs`** + +Both `switch` (macOS-only, uses `libc::socketpair`) and `bridge` (Linux-only, uses +`ip` command) are compiled on all platforms. They fail at the `libc` level on the +wrong platform or are dead code. + +**Fix:** Gate `switch` behind `#[cfg(target_os = "macos")]` and `bridge` behind +`#[cfg(target_os = "linux")]`. + +--- + +## OBSERVABILITY GAPS + +### 39. No logging when VM boot fails at the manager level + +**`src/vm.rs:99-106`** + +When `driver.boot(config)` fails, the VM is silently removed from the map and the +error is returned. No `tracing::warn!` or `tracing::error!` at the manager level. +The caller sees the error but the operational logs have a gap. + +**Fix:** +```rust +Err(err) => { + tracing::warn!(vm = %config.name, error = %err, "VM boot failed"); + // ... cleanup and return Err +} +``` + +--- + +### 40. `wait_all_ready` doesn't log progress + +**`src/vm.rs:196-242`** + +During a potentially multi-minute wait, there is no periodic logging of which VMs +are still pending. Users watching logs see nothing until timeout or success. + +**Fix:** Add a periodic `tracing::info!` every N iterations listing pending VMs. + +--- + +### 41. Missing logging in `stop_by_handle` / `kill_by_handle` + +**`src/vm.rs:147-165`** + +Unlike `stop()` and `kill()` which delegate to the driver (which logs), there's no +manager-level logging for `stop_by_handle` and `kill_by_handle`. No way to +correlate these operations in logs. + +--- + +## DESIGN / CODE SMELL + +### 42. `VmDriver` trait is synchronous but the crate is async-heavy + +The `VmDriver` trait methods are all synchronous (`fn boot(...)`) but the crate +uses tokio extensively (`setup`, `oci`, `port_forward`). The `apple_vz` driver uses +`thread::sleep(500ms)` to "give dispatch queue a moment" (`apple_vz.rs:310`), which +is a blocking sleep in what is a boot path. + +The mixed sync/async model means users must use `spawn_blocking` or risk blocking +the tokio runtime. + +--- + +### 43. `ImageStore` methods are sync but called from async context + +`ImageStore::put_blob`, `get_blob`, `extract_layers` all perform synchronous +filesystem I/O. When called from `async fn pull()` in `registry.rs`, they block the +tokio runtime thread. + +**Fix:** Use `tokio::task::spawn_blocking` for heavy I/O, or make the store async. + +--- + +### 44. `apple_vz.rs:310` — sleep as a synchronization mechanism + +```rust +// Give dispatch queue a moment to fire +std::thread::sleep(std::time::Duration::from_millis(500)); +``` + +This is a race condition disguised as a fix. The 500ms sleep is a guess that the +GCD dispatch queue will have fired the block by then. On a loaded system, it may +not be enough. On a fast system, it's wasted time. + +**Fix:** Use a synchronization primitive (e.g., a `Condvar` or channel) that the +completion handler signals, so `boot()` waits only as long as necessary. + +--- + +### 45. Lock poisoning handling is verbose and repetitive + +The pattern `.map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?` +appears 15+ times across `vm.rs`, `apple_vz.rs`, and `cloud_hv.rs`. This is +boilerplate that obscures the actual logic. + +**Fix:** Add a helper trait or function: +```rust +trait PoisonRecover { + fn or_poison(self) -> Result; +} +``` + +--- + +## SUMMARY TABLE + +| Severity | Count | Key themes | +|----------|-------|------------| +| **Critical** | 6 | WHP won't compile, FD leaks, non-atomic writes | +| **High** | 7 | Integer overflow, path traversal, no config validation, O(n) log reads | +| **Medium** | 12 | Duplicated code, missing timeouts, unbounded buffers, error conversions | +| **Low** | 13 | Naming, missing derives, dead code, doc gaps | +| **Observability** | 3 | Silent failures, no progress logging | +| **Design** | 4 | Sync/async mismatch, platform gating, sleep-as-sync | + +**Strongest areas:** Input validation in `seed.rs` (thorough shell injection +prevention), OCI store digest verification, learning-bridge design, PID-reuse +detection in `cloud_hv.rs`. + +**Weakest areas:** WHP driver integration (not compilable), missing config +validation at the API boundary, sync/async impedance mismatch, non-atomic file +writes. + +--- + +## Commentary on REVIEW.md (Codex Review) + +The Codex review (`REVIEW.md`, dated 2026-03-13) is a thorough prior review. Its +remediation section states all concrete findings have been addressed in the current +tree. Below is a comparison of the two reviews — what overlaps, what each caught +uniquely, and where I agree or disagree with the Codex review's framing. + +### Areas of agreement (both reviews flagged) + +| Topic | Codex | Claude | +|-------|-------|--------| +| FD ownership in public API (`RawFd` exposure) | Unsafe Surfaces #1-2 | Critical #5, Medium #17 | +| `NSFileHandle` `closeOnDealloc: NO` FD leak | Unsafe Surfaces #4 | Low #30 | +| Stringly-typed error enums | Weak Error Handling #1 | Medium #22 | +| Missing logging at manager level (stop/kill/state) | Missing Logging #1-2 | Observability #39-41 | +| `check_ready_marker` / `watch_for_ready` returning `Option` where `Result` is appropriate | Weak Error Handling #4-5 | High #10 (different angle — I focused on the O(n) re-read, Codex on the type) | +| Missing strong typing for names, IPs, MACs | Missing Strong Typing #1-2 | High #8-9 (I focused on validation, Codex on type modeling) | +| Network switch frame drops are silent | Missing Logging #4-5 | (I noted this in the exploration but didn't elevate it — the Codex review is right to call it out) | +| Test self-skip masking real coverage | Test Analysis #2, Silent Fallbacks #33-37 | Not flagged (see below) | + +Both reviews converge on the same fundamental weaknesses: lifecycle ownership +safety, stringly-typed APIs, FD management, and observability gaps. The Codex +review's framing around "silent fallbacks" (37 items cataloged) is particularly +valuable — it's a dimension I under-explored. + +### What the Codex review caught that this review did not + +1. **OCI opaque whiteout handling wrong for files** (Codex High #6). + I read `store.rs` but focused on the path-traversal defense and missed that the + opaque whiteout (`remove_dir_all`) doesn't handle regular files. The Codex + remediation section says this was fixed, but I did not verify the fix. + +2. **Apple FFI constructors not returning `Result`** (Codex Unsafe Surfaces #5). + Valid concern. ObjC `alloc`/`init` can return nil, and the FFI wrappers silently + produce null-pointer wrappers. I read these files but didn't flag this. + +3. **Exhaustive silent-fallback catalog** (Codex Silent Fallbacks #1-37). + The Codex review methodically cataloged every silent fallback, default + substitution, and error-swallowing pattern. My review noted some of these (e.g., + `clone_disk` fallback, credential loading fallback) but didn't systematically + catalog them. The Codex approach is more useful for auditors. + +4. **Test suite structural analysis** (Codex Test Coverage Gaps + Test Suite Analysis). + I didn't analyze the test suite at all. The Codex review's observation that green + CI runs can mean "not exercised" due to self-skipping tests is an important + operational insight. + +5. **CI/workflow security analysis** (Codex GitHub Workflow Analysis). + I didn't review the CI configuration. The Codex review's points about missing + `permissions:` blocks, no dependency audit, and disabled VM lifecycle jobs are + valid and actionable. + +### What this review caught that the Codex review did not + +1. **WHP driver is entirely broken** (Claude Critical #1-4). + The Codex review does not mention the WHP driver at all — not its struct + mismatches, the missing `VmState::Paused` variant, the undeclared modules, or + the missing `windows` dependency. This is the single largest correctness issue + in the current codebase (6 compile-blocking errors in one module). Likely the + WHP driver was added after the Codex review date. + +2. **Page table 1GB limit** (Claude High #13). + `boot.rs` only identity-maps up to 1GB regardless of configured memory. Any VM + with >1GB RAM will crash when accessing memory beyond the first GB. The Codex + review doesn't cover `boot.rs` at all. + +3. **Integer overflow on `memory_mb * 1024 * 1024`** (Claude High #7). + Arithmetic overflow risk on 32-bit targets, and no validation that the values + are non-zero. Not mentioned in the Codex review. + +4. **Non-atomic file writes** (Claude Critical #6). + `download_file` and `put_blob` write directly to the target path. A crash + mid-write produces a corrupt file that passes existence checks on restart. The + Codex review noted download verification was missing (their Medium #10, now + fixed), but not the atomicity gap. + +5. **`DefaultHasher` MAC instability** (Claude High #12). + `generate_mac()` uses `DefaultHasher` which is explicitly not stable across Rust + versions. A toolchain update silently changes MAC addresses, breaking networking. + Not mentioned in the Codex review. + +6. **`clone_disk` succeeds on corrupt existing targets** (Claude High #11). + Returns `Ok(())` if the target file exists, regardless of integrity. Not + mentioned in the Codex review. + +7. **`state()` removes VMs as a side-effect** (Claude Medium #18). + The Apple VZ `state()` method removes stopped/failed VMs from the registry, + which is a Command-Query Separation violation that can cause races. The Codex + review noted state tracking was incorrect (their High #2, now fixed), but the + CQS violation in the current fix wasn't flagged. + +8. **Sync/async impedance mismatch** (Claude Design #42-43). + `VmDriver` is synchronous but the crate depends on tokio. `ImageStore` does + blocking I/O from async contexts. `wait_all_ready` blocks with `thread::sleep`. + The Codex review doesn't discuss the sync/async design tension. + +9. **`sleep(500ms)` as GCD synchronization** (Claude Design #44). + `apple_vz.rs:310` uses a fixed sleep instead of a proper synchronization + primitive. This is a race condition. Not mentioned in the Codex review. + +10. **`unsanitize_name` heuristic bug** (Claude Medium #25). + The fallback in `unsanitize_name` turns `"my_image"` into `"my/image"`. Not + mentioned in the Codex review. + +11. **Unbounded `serial_buffer` in WHP** (Claude Medium #23). + The vCPU loop accumulates all serial output forever. Not mentioned (likely + because WHP was added after the review). + +12. **Path traversal via VM name** (Claude High #9). + `vm_dir()` joins user-supplied `name` directly into `base_dir`. The Codex + review's "Missing Strong Typing #1" implicitly covers this, but doesn't + explicitly call out the path traversal risk. + +13. **`cloud_hv::stop` doesn't escalate for restored handles** (Claude Medium #20). + `wait_for_pid_exit` never sends SIGKILL on timeout, unlike `wait_for_exit`. + The Codex review noted the restored-handle path was unsafe (their High #3, + now partially fixed), but the escalation gap in the current code wasn't caught. + +### Where I disagree with the Codex review + +1. **Severity of OCI config default handling** (Codex Silent Fallback #8). + The Codex review lists "OCI image config silently defaults missing config fields + to empty values" as a silent fallback. I consider this correct behavior — the OCI + spec says these fields are optional, and defaulting to empty is what Docker does. + This isn't a fallback, it's spec compliance. + +2. **Framing of Docker credential fallback** (Codex Silent Fallback #13-14). + Falling back from missing credentials to anonymous pull is the expected UX for + public images. The Codex review frames this as a problem, but it's by design. + The logging is now adequate (the code logs which auth path is taken). + +3. **Port forward connect failures as "weak error handling"** (Codex Weak Error + Handling #6). + Backend connect failures in a port forwarder should be logged-and-dropped, not + propagated to the owner — the owner can't do anything about a single failed + connection. The current `tracing::warn!` + drop behavior is correct for a proxy. + +### Overall assessment + +The Codex review is strong on **lifecycle correctness**, **ownership safety**, +**silent fallback cataloging**, **test gap analysis**, and **CI hardening**. It +drove meaningful improvements to the codebase (the remediation list is substantial). + +This review adds coverage for the **WHP driver** (entirely missed by Codex), +**boot protocol correctness** (`boot.rs` page table limits), **atomicity and +crash safety**, **sync/async design**, and **cross-platform compilation** +correctness. + +Together, the two reviews provide comprehensive coverage. The highest-priority +remaining items from both reviews combined: + +1. Fix the WHP driver to compile (Claude Critical #1-4) +2. Add `VmConfig` validation with name sanitization (Claude High #8-9) +3. Make file writes atomic (Claude Critical #6) +4. Extend page tables beyond 1GB (Claude High #13) +5. Replace `DefaultHasher` in MAC generation (Claude High #12) +6. Add HTTP timeouts (Claude Medium #19) +7. Fix `unsanitize_name` heuristic (Claude Medium #25) +8. Resolve sync/async design tension (Claude Design #42-43) + +--- + +## Codex comments + +1. Validation result: `cargo check` currently passes, while `cargo test --no-run` fails. The most immediate correctness problem in the repo today is not "the crate does not build", but "the shipped tests and the real public surface are out of sync". That distinction matters for prioritization. + +2. I would reframe Claude Critical #1-4 accordingly. The WHP findings are directionally correct, but they describe dead or unreachable Windows code rather than the current shipped build breaking on macOS/Linux. The higher-level issue is product/API inconsistency: Windows support exists in-source but is not wired into [`src/driver/mod.rs`](/Users/soheilalizadeh/vm-rs/src/driver/mod.rs) or [`src/vm.rs`](/Users/soheilalizadeh/vm-rs/src/vm.rs). + +3. A major missing finding is Apple VZ startup correctness. [`src/driver/apple_vz.rs`](/Users/soheilalizadeh/vm-rs/src/driver/apple_vz.rs) logs async start failure in the completion handler but still returns success from `boot()` after a fixed sleep. That means callers can get `Ok(VmHandle)` even when framework startup has already failed. + +4. Another missing finding is a `VmManager::start()` ghost-reservation bug. [`src/vm.rs`](/Users/soheilalizadeh/vm-rs/src/vm.rs) reserves the VM name before creating the VM directory; if `create_dir_all` fails, the placeholder is never removed, and subsequent retries can be blocked by a stale `Starting` entry. + +5. Claude Medium #20 is important, but the consequence should be stated more strongly: restored Cloud Hypervisor stop can return `Ok(())` while the process is still alive. [`wait_for_pid_exit`](/Users/soheilalizadeh/vm-rs/src/driver/cloud_hv.rs#L567) only logs on timeout, so this is a silent lifecycle correctness failure, not just a missing escalation nicety. + +6. Another missing issue is switch liveness reporting. The forwarding thread in [`src/network/switch.rs`](/Users/soheilalizadeh/vm-rs/src/network/switch.rs) can exit on poisoned locks, but the `running` flag stays set, so later `start()` calls can report success while no worker thread exists. + +7. OCI extraction has a correctness bug not covered here: opaque whiteout handling in [`src/oci/store.rs`](/Users/soheilalizadeh/vm-rs/src/oci/store.rs) removes every child with `remove_dir_all`, which fails for regular files and leaves stale files behind. That can produce an incorrect merged rootfs. + +8. I agree with the existing comments near the end of this file that not every "fallback" is a bug. In particular, anonymous OCI pull and defaulting optional OCI config fields are expected behavior. The stronger review focus should stay on silent success reporting, silent state divergence, and unsafe ownership/resource management. + +--- + +## Claude response to Codex comments + +### On comment #1 (cargo check passes, cargo test --no-run fails) + +Accepted. I verified: `cargo check` succeeds, `cargo test --no-run` fails with 40 +errors in `tests/vm_manager.rs` referencing `VmState::Paused`, wrong `VmHandle` +fields (`pid`, `machine_id`), and `pause`/`resume` methods that don't exist on the +trait. The distinction matters — the *library* compiles and ships fine on +macOS/Linux. The breakage is confined to test code that references WHP types not +wired into the public API. I should have run `cargo test --no-run` myself before +categorizing these as "Critical". + +### On comment #2 (reframe Critical #1-4) + +Accepted. I'll concede the severity downgrade. Since `whp.rs` and `boot.rs` are +never declared as modules in `driver/mod.rs`, they're dead source files — the +compiler never touches them on macOS/Linux. The correct framing is: + +- **The WHP code is unreachable dead code**, not a build-breaking issue. +- **The test file (`tests/vm_manager.rs`) references WHP types** that aren't in the + public API, causing test compilation to fail. +- **The product issue** is that Windows support was partially committed — source + files exist but the module wiring, Cargo dependencies, and platform factory are + all missing. + +This should have been categorized as High (product/API inconsistency + broken test +suite), not Critical (build failure). + +### On comment #3 (Apple VZ fire-and-forget boot) + +Accepted — this is a real bug I missed entirely. Looking at the code: + +``` +apple_vz.rs:281-310 +``` + +The completion handler at line 284 is an async callback that runs on the GCD +dispatch queue. If startup fails, it calls `tracing::error!()` — but `boot()` has +already returned `Ok(VmHandle)` after a fixed `sleep(500ms)` at line 310. There is +no channel, condvar, or shared state connecting the completion handler's result back +to the `boot()` caller. + +This means: +1. `boot()` can return `Ok` while the VM failed to start. +2. The error is only visible in logs, not programmatically. +3. The caller proceeds to track a VM that never actually booted. +4. `state()` will eventually report the failure, but there's a window where the + system believes the VM is healthy. + +This should have been in my review as **High severity** — it's a silent success +report for a failed operation, which is exactly the class of bug the Codex review +rightly emphasizes. + +### On comment #4 (ghost reservation bug) + +Accepted — verified in the code. The sequence at `vm.rs:59-88`: + +1. Line 74: Name reserved by inserting a `Starting` placeholder into the map. +2. Line 84: Lock is dropped. +3. Line 88: `create_dir_all(&vm_dir)` — if this fails (permissions, disk full), + the error is returned via `?`. +4. The placeholder from step 1 is **never removed**. + +On retry, line 67 sees the stale `Starting` entry and returns `BootFailed("VM +already exists in state: starting")`. The only recovery is to restart the process +(clearing the in-memory map). + +The same ghost-reservation pattern would occur if `driver.boot()` at line 97 +panics instead of returning `Err` — though the `Err` path at line 99-106 does +clean up correctly. + +This is a genuine bug I missed. Should be **Medium severity** — it's a +reliability issue in error recovery paths. + +### On comment #5 (Medium #20 severity upgrade) + +Accepted. I understated the consequence. The full picture: + +- `cloud_hv.rs:265-280`: restored-handle `stop()` sends SIGTERM and calls + `wait_for_pid_exit()`. +- `cloud_hv.rs:567-577`: `wait_for_pid_exit()` polls for 10 seconds, then + **only logs a warning** and returns. +- The caller (`stop()`) then returns `Ok(())`. + +So: `stop()` returns `Ok(())` while the process may still be alive. The +`VmManager` then marks the VM as `Stopped` in its map. The state is now +diverged from reality — the process is running but the manager thinks it's +stopped. A subsequent `start()` with the same name would succeed (the old +entry is `Stopped`), potentially creating a name collision with the orphaned +process still holding resources. + +This is a **silent lifecycle correctness failure**, not just a missing +escalation. I should upgrade this from Medium to High. + +### On comment #6 (switch liveness reporting) + +Accepted — verified. At `switch.rs:260-262`: + +```rust +let nets = match networks.lock() { + Ok(n) => n, + Err(_) => break, // Thread exits silently +}; +``` + +When the forwarding thread breaks out of the loop, it simply returns — it does +**not** set `running` to `false`. Meanwhile `start()` at line 129 checks +`running` and returns `Ok(())` early if it's `true`: + +```rust +if self.running.load(Ordering::Relaxed) { + return Ok(()); +} +``` + +So after a lock-poison exit: +1. The worker thread is dead. +2. `running` is still `true`. +3. `start()` reports success without spawning a new thread. +4. No frames are forwarded, but the API reports the switch is running. + +This is a genuine bug I missed. Should be **Medium severity** — silent state +divergence in a networking component. + +### On comment #7 (OCI opaque whiteout) + +Partially accepted. Looking at the current code at `store.rs:314-324`: + +The **opaque whiteout** path (`.wh..wh..opq`) uses `remove_dir_all()` on each +child. If a child is a regular file, `remove_dir_all()` does actually work on +files in Rust — the `std::fs::remove_dir_all` docs say "if path is not a +directory, an error is returned" but the actual behavior on most platforms is +that it calls `remove_file` as a fallback. However, this is platform-dependent +behavior and should not be relied upon. + +The **named whiteout** path (`.wh.`, lines 327-334) correctly tries +`remove_file` first, then falls back to `remove_dir_all`. + +The Codex comment is directionally correct: relying on `remove_dir_all` for +regular files is fragile. The fix is to check `file_type()` and dispatch to +`remove_file` or `remove_dir_all` accordingly. I should have flagged this. + +### On comment #8 (fallback framing) + +Agreed. The Codex comment aligns with my own "Where I disagree" section. The +three most impactful classes of issue across both reviews are: + +1. **Silent success reporting** (Apple VZ fire-and-forget boot, restored-handle + stop returning Ok while process is alive, ghost reservations) +2. **Silent state divergence** (switch liveness, state() side-effects, stale + Starting entries) +3. **Unsafe ownership/resource management** (FD leaks, non-atomic writes, raw + FD storage) + +These are more important than cataloging every intentional fallback. + +### Updated priority list (incorporating Codex feedback) + +1. Fix test suite compilation (`tests/vm_manager.rs` references non-existent + WHP types) — blocks CI entirely +2. Fix Apple VZ fire-and-forget boot (Codex comment #3) — silent success on + failure +3. Fix ghost reservation in `VmManager::start()` (Codex comment #4) — blocks + retry on transient errors +4. Fix restored-handle `stop()` to escalate and not return `Ok` on timeout + (Codex comment #5, Claude Medium #20) — silent lifecycle failure +5. Fix switch liveness reporting (Codex comment #6) — silent state divergence +6. Add `VmConfig` validation with name sanitization (Claude High #8-9) — path + traversal +7. Make file writes atomic (Claude Critical #6) — crash corruption +8. Extend page tables beyond 1GB (Claude High #13) — guest crash +9. Replace `DefaultHasher` in MAC generation (Claude High #12) — networking + breakage across toolchain updates +10. Add HTTP timeouts (Claude Medium #19) — hangs on stalled servers diff --git a/COMBINED-REVIEW.md b/COMBINED-REVIEW.md new file mode 100644 index 0000000..f20af79 --- /dev/null +++ b/COMBINED-REVIEW.md @@ -0,0 +1,735 @@ +# vm-rs Combined Review & Remediation Plan + +Combined findings from Codex review (2026-03-13), Claude review (2026-03-14), and +cross-review commentary. Deduplicated, severity-normalized, and organized by +category. + +**Finding sources:** +- **CX** = Codex REVIEW.md (historical, most items remediated in prior pass) +- **CL** = Claude CLAUDE-REVIEW.md +- **CX2** = Codex comments on Claude review (2026-03-14) + +Findings marked **(fixed)** were remediated during the Codex review pass. +All remaining findings are **open** in the current tree. + +--- + +## 1. Lifecycle Correctness + +These are the highest-impact findings. A VM lifecycle library that silently reports +success on failure, or diverges its internal state from reality, is fundamentally +broken for its users. + +### 1.1 Apple VZ `boot()` returns `Ok` on async startup failure [HIGH] + +**Source:** CX2 #3 (new finding), CL #44 (related sleep issue) + +`boot()` dispatches VM startup to a GCD queue via `dispatch_async`, then sleeps +500ms and returns `Ok(VmHandle)`. The completion handler logs failure via +`tracing::error!` but has no channel back to the caller. Result: `boot()` can +return `Ok` for a VM that failed to start. + +**Files:** `src/driver/apple_vz.rs:281-310` + +**Impact:** Callers track a VM that never booted. `state()` eventually catches up, +but there is a window of silent state divergence. + +**Fix:** Use a `std::sync::mpsc::channel` or `Condvar` in the completion handler. +`boot()` blocks on the receiver instead of sleeping. Returns `Err` if the +completion handler reports failure. + +--- + +### 1.2 `VmManager::start()` ghost reservation on `create_dir_all` failure [MEDIUM] + +**Source:** CX2 #4 + +The name is reserved (line 74) before `create_dir_all` (line 88). If directory +creation fails, the stale `Starting` placeholder is never removed. Subsequent +retries are blocked permanently (until process restart). + +**Files:** `src/vm.rs:59-88` + +**Fix:** Wrap the post-reservation code in a cleanup guard: +```rust +let handle = match self.driver.boot(config) { ... }; +// On Err, already cleaned up at line 104 +``` +Move `create_dir_all` before the reservation, or add cleanup on its error path. + +--- + +### 1.3 Restored-handle `stop()` returns `Ok` while process is alive [HIGH] + +**Source:** CL #20, CX2 #5 (severity upgrade) + +`cloud_hv::stop()` for restored handles calls `wait_for_pid_exit()`, which only +logs on timeout and returns. The caller returns `Ok(())`. The manager marks the VM +`Stopped` while the process may still be running. + +**Files:** `src/driver/cloud_hv.rs:265-280`, `src/driver/cloud_hv.rs:567-577` + +**Fix:** `wait_for_pid_exit` must escalate to SIGKILL on timeout and return an error +if the process is still alive after escalation. Unify with `wait_for_exit` which +already does this correctly. + +--- + +### 1.4 `state()` removes VMs from registry as a side effect [MEDIUM] + +**Source:** CL #18 + +Apple VZ `state()` removes stopped/failed VMs from the registry. This violates +Command-Query Separation and races under concurrent callers. + +**Files:** `src/driver/apple_vz.rs:393-395` + +**Fix:** Remove the `registry.remove()` from `state()`. Only `stop()`/`kill()` +should modify the registry. + +--- + +### 1.5 Network switch liveness divergence [MEDIUM] + +**Source:** CX2 #6 + +The forwarding thread exits on lock poison (`break` at line 262) but `running` +stays `true`. Subsequent `start()` calls return `Ok(())` without spawning a thread. +The switch appears running but forwards nothing. + +**Files:** `src/network/switch.rs:260-262`, `src/network/switch.rs:129-131` + +**Fix:** Set `running.store(false, ...)` at the end of `forwarding_loop`, or before +each `break`. Alternatively, check if the worker thread handle is still alive. + +--- + +### 1.6 `clone_disk` silently succeeds on corrupt existing files [MEDIUM] + +**Source:** CL #11 + +Returns `Ok(())` if target exists, regardless of integrity. A partial file from a +crashed clone is silently used. + +**Files:** `src/vm.rs:249-252` + +**Fix:** Compare file sizes with base image. Or remove skip-if-exists and always +clone (caller can check themselves). + +--- + +### 1.7 Previously fixed lifecycle issues [FIXED] + +- Apple VZ `kill()` not terminating VMs — **(fixed)** CX High #1 +- Apple VZ state tracking incorrect after shutdown — **(fixed)** CX High #2 +- Linux restored-handle lifecycle unsafe — **(fixed)** CX High #3 +- Linux zombie misclassification — **(fixed)** CX High #4 +- Duplicate-name race in `VmManager::start()` — **(fixed)** CX Medium #11 + +--- + +## 2. Security + +### 2.1 VM name path traversal [HIGH] + +**Source:** CL #9 + +`vm_dir()` joins user-supplied `name` into `base_dir` without validation. A name +like `"../../etc"` escapes the base directory. Used for VM directories, socket +paths, serial logs, CString labels. + +**Files:** `src/vm.rs:54-56` + +**Fix:** Validate name matches `^[a-zA-Z0-9][a-zA-Z0-9._-]{0,127}$` in a +`VmConfig::validate()` method called at the top of `VmManager::start()`. + +--- + +### 2.2 No `VmConfig` validation at manager level [HIGH] + +**Source:** CL #8 + +No validation for: empty name, zero CPUs, zero memory, empty kernel path. Each +driver rediscovers these inconsistently or not at all. + +**Files:** `src/vm.rs:59` + +**Fix:** Add `VmConfig::validate() -> Result<(), VmError>` checking all invariants. +Call it as the first operation in `VmManager::start()`. + +--- + +### 2.3 Bridge interface name not validated [MEDIUM] + +**Source:** CL #21 + +`ensure_bridge(name, ...)` passes unvalidated strings to `ip` and `iptables` +commands. + +**Files:** `src/network/bridge.rs` + +**Fix:** Validate `name` matches `^[a-zA-Z0-9_-]{1,15}$`. + +--- + +### 2.4 OCI opaque whiteout doesn't remove regular files [MEDIUM] + +**Source:** CX High #6, CX2 #7 + +Opaque whiteout uses `remove_dir_all()` on each child, which may fail on regular +files (platform-dependent). Stale files produce an incorrect merged rootfs. + +**Files:** `src/oci/store.rs:314-324` + +**Fix:** Check `file_type()` and dispatch to `remove_file` for files, +`remove_dir_all` for directories. + +--- + +### 2.5 Previously fixed security issues [FIXED] + +- `virtiofsd` launched with `--sandbox=none` — **(fixed)** CX High #5 +- Port forwarding binds 0.0.0.0 by default — **(fixed)** CX Medium #7 +- Cloud-init injection surfaces — **(fixed)** CX Medium #8 +- Image downloads not verified — **(fixed)** CX Medium #10 +- Raw PID exposure in public API — **(fixed)** CX Unsafe Surfaces #3 +- Raw FD in `NetworkAttachment` — **(fixed)** CX Unsafe Surfaces #1-2 + +--- + +## 3. Resource Management + +### 3.1 Non-atomic file writes (crash corruption) [HIGH] + +**Source:** CL #6 + +`download_file` (`setup/image.rs:303`) and `put_blob` (`oci/store.rs:114`) write +directly to the target path. A crash mid-write leaves a corrupt file that passes +existence checks on restart. + +**Files:** `src/setup/image.rs:303`, `src/oci/store.rs:114` + +**Fix:** Write to temp file in same directory, `fsync`, atomic `rename`. + +--- + +### 3.2 `SwitchPort` FD leak on error paths [HIGH] + +**Source:** CL #5 + +`create_socketpair()` returns raw FDs. If the lock acquisition fails before +insertion, the switch-side FD leaks. `SwitchPort` stores a bare `RawFd` with no +RAII. + +**Files:** `src/network/switch.rs:96-120` + +**Fix:** Wrap both FDs in `OwnedFd` immediately. Store `OwnedFd` in `SwitchPort`. + +--- + +### 3.3 `NSFileHandle` FD leak (`closeOnDealloc: NO`) [LOW] + +**Source:** CL #30, CX Unsafe Surfaces #4 + +FDs transferred via `into_raw_fd()` but NSFileHandle told not to close on dealloc. +FDs leak when the ObjC object is deallocated. + +**Files:** `src/ffi/apple_vz/base.rs:171` + +**Fix:** Pass `YES` for `closeOnDealloc`. + +--- + +## 4. Correctness + +### 4.1 WHP driver is dead/unwired code + test suite broken [HIGH] + +**Source:** CL #1-4, CX2 #1-2 + +`whp.rs` and `boot.rs` exist as source files but are not declared as modules in +`driver/mod.rs`. `tests/vm_manager.rs` references WHP types (`VmState::Paused`, +wrong `VmHandle` fields, `pause`/`resume` methods), causing `cargo test --no-run` +to fail with 40 errors. The library itself compiles fine — the breakage is confined +to tests. + +**Files:** `src/driver/mod.rs`, `src/driver/whp.rs`, `src/driver/boot.rs`, +`tests/vm_manager.rs`, `Cargo.toml` + +**Fix (two options):** +- **Option A (wire it in):** Add module declarations, Cargo dependencies, platform + factory entry, fix `VmHandle` field names, add `VmState::Paused`, extend + `VmDriver` trait or use separate impl block for `pause`/`resume`. +- **Option B (defer):** Remove WHP references from `tests/vm_manager.rs` so tests + compile. Keep `whp.rs`/`boot.rs` as dormant source for future integration. + +--- + +### 4.2 Page tables only support 1 GB of guest memory [HIGH] + +**Source:** CL #13 + +`setup_page_tables` creates a single PD with max 512 entries (1 GB). VMs with +`memory_mb > 1024` will crash on memory access above 1 GB. + +**Files:** `src/driver/boot.rs:76` + +**Fix:** Support multiple PD tables (one per PDPT entry). Or validate `memory_mb <= +1024` with a clear error explaining the current limit. + +--- + +### 4.3 `DefaultHasher` produces unstable MAC addresses [HIGH] + +**Source:** CL #12 + +`generate_mac()` uses `std::collections::hash_map::DefaultHasher`, which is +explicitly not stable across Rust versions. A toolchain update silently changes +MAC addresses, breaking networking and DHCP leases. + +**Files:** `src/driver/cloud_hv.rs:443-456` + +**Fix:** Use SHA-256 truncated to 3 bytes, or a fixed-seed SipHash. The result must +be deterministic and stable across compiler versions. + +--- + +### 4.4 Integer overflow on `memory_mb * 1024 * 1024` [MEDIUM] + +**Source:** CL #7 + +On 32-bit targets, `usize` overflows for `memory_mb >= 4096`. No validation that +values are non-zero. + +**Files:** `src/driver/apple_vz.rs:96`, `src/driver/whp.rs:182` + +**Fix:** Use `u64` for byte calculations. Validate `memory_mb > 0` and `cpus > 0` +in `VmConfig::validate()`. + +--- + +### 4.5 `unsanitize_name` corrupts names with underscores [MEDIUM] + +**Source:** CL #25 + +Fallback heuristic turns `"my_image"` into `"my/image"`. + +**Files:** `src/oci/store.rs:375-381` + +**Fix:** Remove the fallback heuristic. Only unsanitize via `_slash_`/`_colon_`. + +--- + +### 4.6 Unbounded `serial_buffer` in WHP vCPU loop [MEDIUM] + +**Source:** CL #23 + +Accumulates all serial output forever. Unbounded memory growth. + +**Files:** `src/driver/whp.rs:687` + +**Fix:** Stop accumulating after `VMRS_READY` is found, or use a ring buffer. + +--- + +### 4.7 Duplicated `check_ready_marker` + O(n) re-read [MEDIUM] + +**Source:** CL #10, CL #14 + +Identical function in both drivers reads entire serial log on every `state()` poll. + +**Files:** `src/driver/apple_vz.rs:422-438`, `src/driver/cloud_hv.rs:519-536` + +**Fix:** Extract to `driver/mod.rs`. Use incremental reading (track last offset). + +--- + +## 5. Reliability + +### 5.1 No HTTP timeouts [MEDIUM] + +**Source:** CL #19 + +HTTP clients in `setup/image.rs` and `oci/registry.rs` have no timeouts. Downloads +can hang forever. + +**Files:** `src/setup/image.rs:167`, `src/oci/registry.rs:84-87` + +**Fix:** Set `connect_timeout(30s)` and `timeout(300s)` on both clients. + +--- + +### 5.2 `wait_all_ready` blocks the thread [MEDIUM] + +**Source:** CL #16 + +Uses `thread::sleep(1s)` in a loop. Blocks tokio runtime if called from async code. + +**Files:** `src/vm.rs:196-242` + +**Fix:** Add `async fn wait_all_ready_async()` using `tokio::time::sleep`. Add doc +comment on the sync version warning about blocking. + +--- + +### 5.3 Seed ISO temp dir collision [LOW] + +**Source:** CL #32 + +Two concurrent processes creating seed ISOs for the same hostname clobber each +other. + +**Files:** `src/setup/seed.rs:95` + +**Fix:** Use `tempfile::tempdir_in()`. + +--- + +## 6. Observability + +### 6.1 No manager-level logging for lifecycle operations [MEDIUM] + +**Source:** CL #39-41, CX Missing Logging #1-2 + +`stop()`, `kill()`, `stop_by_handle()`, `kill_by_handle()` have no logging at the +manager layer. Boot failure cleanup is silent. + +**Files:** `src/vm.rs` + +**Fix:** Add `tracing::info!` for stop/kill at the manager level. Add +`tracing::warn!` for boot failure cleanup. + +--- + +### 6.2 `wait_all_ready` doesn't log progress [LOW] + +**Source:** CL #40 + +Multi-minute wait with no periodic status update. + +**Files:** `src/vm.rs:196-242` + +**Fix:** Log pending VMs every 10 seconds. + +--- + +### 6.3 Logging inconsistency (structured vs format strings) [LOW] + +**Source:** CL #29 + +Driver modules use structured fields; OCI/setup modules use format strings. + +**Fix:** Standardize on structured tracing fields throughout. + +--- + +## 7. Error Handling & Types + +### 7.1 No `From` conversions between error types [MEDIUM] + +**Source:** CL #22 + +`OciError` and `SetupError` can't convert to `VmError`. Awkward to compose +operations. + +**Fix:** Add `From for VmError` and `From for VmError`. + +--- + +### 7.2 Stringly-typed error enums [LOW] + +**Source:** CX Weak Error Handling #1, CL #22 + +`VmError`, `SetupError`, `OciError` use `String` detail fields. Weakens matching. + +**Fix:** Long-term, add typed sub-variants. Short-term, accept as pragmatic. + +--- + +### 7.3 `VmState` missing `Eq` derive [LOW] + +**Source:** CL #26 + +**Fix:** Add `Eq` to the derive list. + +--- + +### 7.4 Missing `#[must_use]` on key types [LOW] + +**Source:** CL #31 + +`VmHandle`, `VmState`, `PreparedImage`, `ImageManifest`. + +**Fix:** Add `#[must_use]` attribute. + +--- + +## 8. Platform & Cross-Compilation + +### 8.1 `config.rs` Unix-only types not cfg-gated [MEDIUM] + +**Source:** CL #24 + +`VmSocketEndpoint` uses `std::os::fd::*` which doesn't exist on Windows. + +**Files:** `src/config.rs:3` + +**Fix:** Gate behind `#[cfg(unix)]`. + +--- + +### 8.2 Network modules not platform-gated [LOW] + +**Source:** CL #38 + +`switch` (macOS) and `bridge` (Linux) compiled on all platforms. + +**Fix:** Gate with `#[cfg(target_os = "...")]`. + +--- + +### 8.3 `clone_disk` no-op on unsupported platforms [LOW] + +**Source:** CL #36 + +Silently returns `Ok(())` without doing anything. + +**Fix:** Add fallback `std::fs::copy` or return error. + +--- + +## 9. Design Debt + +### 9.1 `VmDriver` trait is sync in an async-heavy crate [DESIGN] + +**Source:** CL #42 + +Trait methods block. Mixed with tokio throughout. + +**Action:** Document blocking nature. Consider `async_trait` in v0.2. + +--- + +### 9.2 `ImageStore` blocks tokio from async context [DESIGN] + +**Source:** CL #43 + +Sync filesystem I/O called from `async fn pull()`. + +**Fix:** Wrap heavy I/O in `tokio::task::spawn_blocking`. + +--- + +### 9.3 Lock poisoning boilerplate [DESIGN] + +**Source:** CL #45 + +Same `.map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))` pattern +15+ times. + +**Fix:** Helper trait: +```rust +trait PoisonRecover { + fn or_poison(self) -> Result; +} +``` + +--- + +### 9.4 Apple FFI constructors don't return `Result` [DESIGN] + +**Source:** CX Unsafe Surfaces #5 + +ObjC `alloc`/`init` can return nil but wrappers don't check. + +**Action:** Long-term, add nil checks and return `Option` or `Result`. + +--- + +## 10. Testing & CI + +### 10.1 Test suite doesn't compile [BLOCKING] + +**Source:** CX2 #1 + +`cargo test --no-run` fails with 40 errors in `tests/vm_manager.rs`. + +**Fix:** Remove or gate WHP-specific test code. + +--- + +### 10.2 Tests silently self-skip [MEDIUM] + +**Source:** CX Test Analysis #2 + +`vm_lifecycle`, `seed_iso`, `oci_pull` return early on missing assets. Green CI can +mean "not exercised". + +**Status:** Partially addressed (workflow-dispatch gating added per CX remediation). + +--- + +### 10.3 No adversarial input testing [MEDIUM] + +**Source:** CX Test Analysis #7 + +Only partial coverage for env-name rejection. No tests for hostile process commands, +health checks, volume tags, NIC config, SSH keys, OCI whiteouts. + +**Fix:** Add fuzz-style unit tests for `seed.rs` validators and `store.rs` +extraction. + +--- + +--- + +# Remediation Plan + +## Phase 0: Unblock CI (1 PR) + +**Goal:** `cargo test` compiles and passes. + +| # | Task | Files | Finding | +|---|------|-------|---------| +| 0.1 | Remove or `#[cfg(windows)]`-gate WHP references in `tests/vm_manager.rs` | `tests/vm_manager.rs` | 10.1, 4.1 | +| 0.2 | Verify `cargo test` and `cargo clippy` pass | CI | — | + +**Acceptance:** `cargo test --no-run` succeeds on macOS and Linux. + +--- + +## Phase 1: Silent Success & State Divergence (1-2 PRs) + +**Goal:** The library never returns `Ok` for an operation that failed. + +| # | Task | Files | Finding | +|---|------|-------|---------| +| 1.1 | Apple VZ: replace `sleep(500ms)` with `mpsc::channel` in completion handler; `boot()` blocks on result, returns `Err` on failure | `src/driver/apple_vz.rs` | 1.1 | +| 1.2 | Fix ghost reservation: clean up placeholder if `create_dir_all` fails | `src/vm.rs` | 1.2 | +| 1.3 | `wait_for_pid_exit`: escalate to SIGKILL on timeout, return `Err` if still alive | `src/driver/cloud_hv.rs` | 1.3 | +| 1.4 | Remove `registry.remove()` from `state()` in Apple VZ driver | `src/driver/apple_vz.rs` | 1.4 | +| 1.5 | Switch: set `running = false` when forwarding loop exits | `src/network/switch.rs` | 1.5 | +| 1.6 | `clone_disk`: validate target file size against base before skipping | `src/vm.rs` | 1.6 | + +**Acceptance:** No API path returns `Ok` when the underlying operation failed. +Add regression tests for 1.1-1.3. + +--- + +## Phase 2: Security Hardening (1 PR) + +**Goal:** User-supplied strings cannot escape their intended scope. + +| # | Task | Files | Finding | +|---|------|-------|---------| +| 2.1 | Add `VmConfig::validate()` — name regex, non-zero cpus/memory, non-empty kernel | `src/config.rs`, `src/vm.rs` | 2.1, 2.2 | +| 2.2 | Validate bridge interface names | `src/network/bridge.rs` | 2.3 | +| 2.3 | Fix OCI opaque whiteout to use `remove_file` for files | `src/oci/store.rs` | 2.4 | +| 2.4 | Fix `unsanitize_name` heuristic | `src/oci/store.rs` | 4.5 | + +**Acceptance:** Unit tests for path-traversal names rejected, whiteout on files, +`unsanitize_name("my_image") == "my_image"`. + +--- + +## Phase 3: Crash Safety & Resource Management (1 PR) + +**Goal:** No corrupt state after a crash. No FD leaks. + +| # | Task | Files | Finding | +|---|------|-------|---------| +| 3.1 | Atomic writes: temp file + fsync + rename in `download_file` and `put_blob` | `src/setup/image.rs`, `src/oci/store.rs` | 3.1 | +| 3.2 | `SwitchPort`: store `OwnedFd` instead of `RawFd` | `src/network/switch.rs` | 3.2 | +| 3.3 | `NSFileHandle`: pass `closeOnDealloc: YES` | `src/ffi/apple_vz/base.rs` | 3.3 | +| 3.4 | Add HTTP timeouts to both clients | `src/setup/image.rs`, `src/oci/registry.rs` | 5.1 | + +**Acceptance:** Interrupted download leaves no partial file. Switch FDs cleaned up +on all paths. + +--- + +## Phase 4: Correctness (1-2 PRs) + +**Goal:** Boot protocol and MAC generation are correct. + +| # | Task | Files | Finding | +|---|------|-------|---------| +| 4.1 | Replace `DefaultHasher` with SHA-256-truncated MAC generation | `src/driver/cloud_hv.rs` | 4.3 | +| 4.2 | Extend page tables to support >1GB (multiple PDs via PDPT entries), or validate `memory_mb <= 1024` | `src/driver/boot.rs` | 4.2 | +| 4.3 | Use `u64` for memory byte calculations | `src/driver/apple_vz.rs`, `src/driver/whp.rs` | 4.4 | +| 4.4 | Cap or ring-buffer `serial_buffer` in WHP | `src/driver/whp.rs` | 4.6 | +| 4.5 | Extract shared `check_ready_marker` to `driver/mod.rs`, add incremental reading | `src/driver/mod.rs`, `src/driver/apple_vz.rs`, `src/driver/cloud_hv.rs` | 4.7 | + +**Acceptance:** VMs with 2GB+ RAM boot on WHP. MAC addresses stable across rustc +versions (add test). Serial log polling is O(delta) not O(n). + +--- + +## Phase 5: Observability & Ergonomics (1 PR) + +**Goal:** Operational visibility and developer ergonomics. + +| # | Task | Files | Finding | +|---|------|-------|---------| +| 5.1 | Add manager-level logging for stop/kill/boot-failure | `src/vm.rs` | 6.1 | +| 5.2 | Log progress in `wait_all_ready` every 10s | `src/vm.rs` | 6.2 | +| 5.3 | Standardize structured tracing fields in OCI/setup modules | `src/oci/*.rs`, `src/setup/*.rs` | 6.3 | +| 5.4 | Add `From` and `From` for `VmError` | `src/driver/mod.rs` | 7.1 | +| 5.5 | Add `Eq` derive to `VmState` | `src/config.rs` | 7.3 | +| 5.6 | Add `#[must_use]` to key types | `src/config.rs`, `src/oci/store.rs` | 7.4 | +| 5.7 | Lock poisoning helper trait | `src/driver/mod.rs` or utility module | 9.3 | +| 5.8 | Add `async fn wait_all_ready_async` + doc warning on sync version | `src/vm.rs` | 5.2 | + +**Acceptance:** Operational logs cover full lifecycle. Error types compose cleanly. + +--- + +## Phase 6: Platform Gating & WHP Integration (1-2 PRs) + +**Goal:** Clean cross-platform compilation and Windows support foundation. + +| # | Task | Files | Finding | +|---|------|-------|---------| +| 6.1 | Gate `VmSocketEndpoint` and FD imports behind `#[cfg(unix)]` | `src/config.rs` | 8.1 | +| 6.2 | Gate `network::switch` behind `#[cfg(target_os = "macos")]`, `network::bridge` behind `#[cfg(target_os = "linux")]` | `src/network/mod.rs` | 8.2 | +| 6.3 | `clone_disk`: add `#[cfg(not(...))]` fallback with `std::fs::copy` | `src/vm.rs` | 8.3 | +| 6.4 | Wire WHP: add `pub mod boot;`, `#[cfg(windows)] pub mod whp;`, fix `VmHandle` fields, add `VmState::Paused`, add `windows` crate dep, update `create_platform_driver()` | Multiple | 4.1 | + +**Acceptance:** `cargo check --target x86_64-pc-windows-msvc` succeeds (with +`--no-default-features` if needed). macOS and Linux unaffected. + +--- + +## Phase 7: Testing & CI Hardening (1 PR) + +**Goal:** Tests exercise the code that matters, CI catches regressions. + +| # | Task | Files | Finding | +|---|------|-------|---------| +| 7.1 | Add unit tests for `VmConfig::validate()` — path traversal, empty name, zero resources | `src/config.rs` | 2.1, 2.2 | +| 7.2 | Add unit tests for `unsanitize_name` edge cases | `src/oci/store.rs` | 4.5 | +| 7.3 | Add unit test for OCI opaque whiteout on files | `src/oci/store.rs` | 2.4 | +| 7.4 | Add adversarial input tests for `seed.rs` — hostile commands, mount points, SSH keys | `src/setup/seed.rs` | 10.3 | +| 7.5 | Add MAC generation stability test (hash known input, assert fixed output) | `src/driver/cloud_hv.rs` | 4.3 | +| 7.6 | Add `cargo-deny` or `cargo-audit` step to CI | `.github/workflows/ci.yml` | CX Workflow #5 | + +**Acceptance:** `cargo test` exercises validation, whiteout, MAC stability, and +hostile seed inputs. + +--- + +## Summary + +| Phase | PRs | Findings addressed | Risk reduced | +|-------|-----|--------------------|--------------| +| **0** | 1 | 1 | CI unblocked | +| **1** | 1-2 | 6 | Silent success / state divergence | +| **2** | 1 | 4 | Path traversal, injection, data corruption | +| **3** | 1 | 4 | Crash safety, FD leaks, hangs | +| **4** | 1-2 | 5 | Boot protocol, MAC stability, memory | +| **5** | 1 | 8 | Observability, ergonomics | +| **6** | 1-2 | 4 | Cross-platform, Windows foundation | +| **7** | 1 | 6 | Test coverage for security-critical paths | +| **Total** | **8-11** | **38** | — | + +Phases 0-3 are the highest priority and should be done in order. Phases 4-7 can be +parallelized. diff --git a/codex-review.md b/codex-review.md new file mode 100644 index 0000000..72e0485 --- /dev/null +++ b/codex-review.md @@ -0,0 +1,109 @@ +# Codex Review + +Date: 2026-03-14 + +Scope: +- Static review of the core abstraction, platform drivers, FFI boundary, setup, OCI, and networking modules under `src/` +- Build verification with `cargo check` +- Test/build verification with `cargo test --no-run` + +Verification summary: +- `cargo check` succeeds. +- `cargo test --no-run` fails with compile errors in the shipped test suite, especially [`tests/vm_manager.rs`](/Users/soheilalizadeh/vm-rs/tests/vm_manager.rs) and [`tests/ffi_smoke.rs`](/Users/soheilalizadeh/vm-rs/tests/ffi_smoke.rs). + +## Findings + +### 1. Critical: the published API, FFI surface, and test suite are materially out of sync + +This repository does not currently validate the module it claims to ship. The driver trait has only `boot/stop/kill/state`, but the test suite still expects `pause/resume` and extra config/handle fields such as `vsock`, `machine_id`, and `efi_variable_store` ([`src/driver/mod.rs#L20`](/Users/soheilalizadeh/vm-rs/src/driver/mod.rs#L20), [`tests/vm_manager.rs#L49`](/Users/soheilalizadeh/vm-rs/tests/vm_manager.rs#L49), [`tests/vm_manager.rs#L162`](/Users/soheilalizadeh/vm-rs/tests/vm_manager.rs#L162)). The Apple FFI tests also import modules and types that are not exported or no longer exist, for example `ffi::apple_vz::platform` is referenced by tests but not re-exported from the module root ([`src/ffi/apple_vz/mod.rs#L18`](/Users/soheilalizadeh/vm-rs/src/ffi/apple_vz/mod.rs#L18), [`tests/ffi_smoke.rs#L18`](/Users/soheilalizadeh/vm-rs/tests/ffi_smoke.rs#L18)). + +Impact: +- CI cannot prove the crate still matches its own documented/tested behavior. +- Consumers cannot trust examples, tests, or type names to reflect the real public surface. +- This is a correctness and maintenance failure before runtime behavior is even considered. + +### 2. High: Apple VZ leaks file descriptors at the FFI boundary + +`NSFileHandle::file_handle_with_fd` creates Objective-C file handles with `closeOnDealloc: NO` ([`src/ffi/apple_vz/base.rs#L168`](/Users/soheilalizadeh/vm-rs/src/ffi/apple_vz/base.rs#L168)). The Apple driver then hands ownership away with `into_raw_fd()` for `/dev/null`, serial logs, and socket-backed NICs ([`src/driver/apple_vz.rs#L138`](/Users/soheilalizadeh/vm-rs/src/driver/apple_vz.rs#L138), [`src/driver/apple_vz.rs#L166`](/Users/soheilalizadeh/vm-rs/src/driver/apple_vz.rs#L166)). After `into_raw_fd()`, Rust no longer owns those descriptors, and the ObjC side is explicitly told not to close them either. + +Impact: +- Long-lived processes can steadily leak FDs per VM start. +- Repeated VM lifecycle operations can eventually fail with `EMFILE`/resource exhaustion. +- This is a concrete unsafe-resource-management bug, not just a style issue. + +### 3. High: Apple VZ reports success before it knows whether startup actually succeeded + +The Apple driver inserts the VM into its registry before start, dispatches an async start block, logs any failure inside the completion callback, sleeps 500ms, and then returns `Ok(VmHandle { state: Starting, .. })` regardless of the callback result ([`src/driver/apple_vz.rs#L268`](/Users/soheilalizadeh/vm-rs/src/driver/apple_vz.rs#L268), [`src/driver/apple_vz.rs#L309`](/Users/soheilalizadeh/vm-rs/src/driver/apple_vz.rs#L309), [`src/vm.rs#L97`](/Users/soheilalizadeh/vm-rs/src/vm.rs#L97)). + +Impact: +- `VmManager::start()` can report success even when entitlement errors or framework start failures already happened. +- The failure is demoted to logging instead of being surfaced synchronously to the caller. +- This creates a silent-fallback style UX problem: callers think the VM booted when it never did. + +### 4. High: `VmManager::start` can leave a permanent ghost reservation on filesystem errors + +`VmManager::start()` reserves the VM name in the in-memory map before creating the VM directory ([`src/vm.rs#L60`](/Users/soheilalizadeh/vm-rs/src/vm.rs#L60)). If `create_dir_all` fails, the function returns early without removing the `Starting` placeholder ([`src/vm.rs#L86`](/Users/soheilalizadeh/vm-rs/src/vm.rs#L86)). Cleanup only exists for driver boot failures, not for earlier I/O failures ([`src/vm.rs#L97`](/Users/soheilalizadeh/vm-rs/src/vm.rs#L97)). + +Impact: +- One transient disk/permission error can make that VM name unusable until the manager process restarts. +- The user sees a misleading "already exists in state: starting" on retry. +- This is a correctness and UX bug in the abstraction layer itself. + +### 5. High: stopping a restored Cloud Hypervisor VM can return success while the VM is still running + +For restored handles, `CloudHvDriver::stop()` sends `SIGTERM`, waits with `wait_for_pid_exit`, and then returns `Ok(())` unconditionally ([`src/driver/cloud_hv.rs#L265`](/Users/soheilalizadeh/vm-rs/src/driver/cloud_hv.rs#L265)). But `wait_for_pid_exit()` only logs a warning on timeout and does not propagate failure ([`src/driver/cloud_hv.rs#L567`](/Users/soheilalizadeh/vm-rs/src/driver/cloud_hv.rs#L567)). + +Impact: +- A caller may believe a restored VM shut down cleanly when the VMM process is still alive. +- Follow-up operations can race a still-running guest and corrupt operator state. +- This is especially dangerous because restored-handle flows are explicitly for daemon restarts and recovery. + +### 6. Medium: the userspace switch can die permanently while `start()` keeps claiming it is running + +The forwarding loop exits if the network lock is poisoned ([`src/network/switch.rs#L260`](/Users/soheilalizadeh/vm-rs/src/network/switch.rs#L260)), but it never clears the `running` flag. `start()` only checks that atomic flag and returns `Ok(())` if it is already set ([`src/network/switch.rs#L126`](/Users/soheilalizadeh/vm-rs/src/network/switch.rs#L126)). That means the switch thread can disappear permanently while the object still reports itself as started. + +Impact: +- Inter-VM networking can silently blackhole traffic after an internal failure. +- The only panic detection happens in `stop()`, not when the failure occurs. +- This is both a reliability and observability gap. + +### 7. Medium: OCI opaque whiteout handling is incorrect and can leave deleted files behind + +When applying an opaque whiteout (`.wh..wh..opq`), the extractor iterates the target directory and calls `remove_dir_all` on every child ([`src/oci/store.rs#L314`](/Users/soheilalizadeh/vm-rs/src/oci/store.rs#L314)). That only works for directories. Regular files fail removal, are merely warned about, and remain in the merged rootfs. + +Impact: +- Layer application can produce the wrong filesystem contents. +- Container images that rely on opaque whiteouts for correctness can boot with stale files present. +- This is a correctness bug in image materialization. + +### 8. Medium: image and OCI downloads are unbounded in time and memory + +The image prep client uses `reqwest::Client::new()` with no timeout ([`src/setup/image.rs#L167`](/Users/soheilalizadeh/vm-rs/src/setup/image.rs#L167)), and the OCI client also builds a client without any timeout configuration ([`src/oci/registry.rs#L84`](/Users/soheilalizadeh/vm-rs/src/oci/registry.rs#L84)). Both paths then read full responses into memory with `bytes()`/`text()` before writing them to disk or processing them ([`src/setup/image.rs#L276`](/Users/soheilalizadeh/vm-rs/src/setup/image.rs#L276), [`src/setup/image.rs#L339`](/Users/soheilalizadeh/vm-rs/src/setup/image.rs#L339), [`src/oci/registry.rs#L294`](/Users/soheilalizadeh/vm-rs/src/oci/registry.rs#L294)). + +Impact: +- Large VM images and OCI layers can cause avoidable memory spikes. +- Slow or hung registries can stall operations indefinitely. +- For an infrastructure library, this is poor reliability and poor user experience. + +### 9. Medium: Windows support exists as code but is unreachable from the actual product surface + +The repository contains a WHP driver implementation ([`src/driver/whp.rs#L1`](/Users/soheilalizadeh/vm-rs/src/driver/whp.rs#L1)), but the driver module only exposes macOS and Linux backends ([`src/driver/mod.rs#L7`](/Users/soheilalizadeh/vm-rs/src/driver/mod.rs#L7)), and `create_platform_driver()` rejects every non-macOS/non-Linux target as unsupported ([`src/vm.rs#L310`](/Users/soheilalizadeh/vm-rs/src/vm.rs#L310)). + +Impact: +- The codebase advertises more platform ambition than the shipped API actually supports. +- Windows support is effectively dead code from a consumer perspective. +- For a "create VMs on any OS" abstraction, this is a product-level correctness gap. + +## Additional Notes + +- Logging is inconsistent in library code. `NSError::dump()` prints directly with `println!` ([`src/ffi/apple_vz/base.rs#L252`](/Users/soheilalizadeh/vm-rs/src/ffi/apple_vz/base.rs#L252)), and `NetworkSwitch::drop()` uses `eprintln!` instead of structured tracing ([`src/network/switch.rs#L178`](/Users/soheilalizadeh/vm-rs/src/network/switch.rs#L178)). That bypasses the caller's logging pipeline. +- The Linux bridge helper mutates global host networking policy by forcing `/proc/sys/net/ipv4/ip_forward = 1` and adding iptables NAT rules ([`src/network/bridge.rs#L40`](/Users/soheilalizadeh/vm-rs/src/network/bridge.rs#L40), [`src/network/bridge.rs#L111`](/Users/soheilalizadeh/vm-rs/src/network/bridge.rs#L111)), but deletion does not roll those changes back. The abstraction leaks host-global side effects. +- `unsafe impl Send` and `unsafe impl Sync` for `VZVirtualMachine` rely on a very thin justification comment ([`src/ffi/apple_vz/virtual_machine.rs#L234`](/Users/soheilalizadeh/vm-rs/src/ffi/apple_vz/virtual_machine.rs#L234)). For ObjC framework objects, that deserves a much stronger proof or a stricter thread-affinity wrapper. + +## Recommended Order Of Attack + +1. Make `cargo test --no-run` green or delete/replace stale tests so the repo can validate itself again. +2. Fix the Apple FD ownership bug and the async-start success reporting path. +3. Fix lifecycle correctness in `VmManager::start()` and restored-handle `stop()`. +4. Harden long-running helpers: switch liveness, streamed downloads, and OCI whiteout application. +5. Decide whether Windows support is in or out, then align the code, docs, and module exports. diff --git a/src/config.rs b/src/config.rs index 7f47ca4..b4051fe 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,7 +1,9 @@ //! VM configuration types — everything needed to boot and manage a VM. +#[cfg(unix)] use std::os::fd::{AsRawFd, FromRawFd, OwnedFd, RawFd}; use std::path::PathBuf; +#[cfg(unix)] use std::sync::Arc; /// Readiness marker written to the serial console when the VM is ready. @@ -9,9 +11,11 @@ use std::sync::Arc; pub const READY_MARKER: &str = "VMRS_READY"; /// Owned VM network endpoint backed by a Unix datagram socket file descriptor. +#[cfg(unix)] #[derive(Debug, Clone)] pub struct VmSocketEndpoint(Arc); +#[cfg(unix)] impl VmSocketEndpoint { pub fn new(fd: OwnedFd) -> Self { Self(Arc::new(fd)) @@ -28,6 +32,7 @@ impl VmSocketEndpoint { } } +#[cfg(unix)] impl AsRawFd for VmSocketEndpoint { fn as_raw_fd(&self) -> RawFd { self.0.as_raw_fd() @@ -106,6 +111,45 @@ pub struct VmConfig { /// Linux network namespace to run the VM in (optional). /// When set, the VMM process is spawned inside `ip netns exec `. pub netns: Option, + /// Enable vsock device for host-guest communication. + pub vsock: bool, + /// Persistent machine identifier (opaque bytes, driver-specific). + pub machine_id: Option>, + /// Path to EFI variable store for UEFI boot (optional). + pub efi_variable_store: Option, + /// Enable Rosetta translation layer (macOS only, Apple Silicon). + pub rosetta: bool, +} + +impl VmConfig { + /// Validate configuration invariants. + pub fn validate(&self) -> Result<(), crate::driver::VmError> { + use crate::driver::VmError; + if self.name.is_empty() { + return Err(VmError::InvalidConfig("VM name must not be empty".into())); + } + if self.name.len() > 128 { + return Err(VmError::InvalidConfig("VM name must be 128 characters or fewer".into())); + } + if !self.name.chars().all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.') { + return Err(VmError::InvalidConfig( + "VM name must contain only alphanumeric characters, hyphens, underscores, and dots".into(), + )); + } + if self.name.starts_with('.') || self.name.starts_with('-') { + return Err(VmError::InvalidConfig("VM name must not start with '.' or '-'".into())); + } + if self.cpus == 0 { + return Err(VmError::InvalidConfig("cpus must be at least 1".into())); + } + if self.memory_mb == 0 { + return Err(VmError::InvalidConfig("memory_mb must be at least 1".into())); + } + if self.kernel.as_os_str().is_empty() { + return Err(VmError::InvalidConfig("kernel path must not be empty".into())); + } + Ok(()) + } } /// Network attachment for a VM. @@ -113,6 +157,7 @@ pub struct VmConfig { pub enum NetworkAttachment { /// Owned socket endpoint for an L2 switch port (macOS). /// The endpoint is the VM's end of a socketpair — the switch holds the other end. + #[cfg(unix)] SocketPairFd(VmSocketEndpoint), /// TAP device name (Linux). Tap { name: String, mac: Option }, @@ -142,10 +187,13 @@ pub struct VmHandle { pub process: Option, /// Serial console log path. pub serial_log: PathBuf, + /// Persistent machine identifier (opaque bytes, driver-specific). + pub machine_id: Option>, } /// VM lifecycle state. -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone, PartialEq, Eq)] +#[must_use] pub enum VmState { /// VM is being created / booting. Starting, @@ -154,6 +202,8 @@ pub enum VmState { /// IP address assigned to the VM. ip: String, }, + /// VM is paused (execution suspended, state preserved). + Paused, /// VM was stopped gracefully. Stopped, /// VM failed to boot or crashed. @@ -168,6 +218,7 @@ impl std::fmt::Display for VmState { match self { VmState::Starting => write!(f, "starting"), VmState::Running { ip } => write!(f, "running ({})", ip), + VmState::Paused => write!(f, "paused"), VmState::Stopped => write!(f, "stopped"), VmState::Failed { reason } => write!(f, "failed: {}", reason), } @@ -215,4 +266,70 @@ mod tests { fn ready_marker_value() { assert_eq!(READY_MARKER, "VMRS_READY"); } + + #[test] + fn vm_state_display_paused() { + assert_eq!(VmState::Paused.to_string(), "paused"); + } + + fn test_vm_config(name: &str) -> VmConfig { + VmConfig { + name: name.into(), + namespace: "test".into(), + kernel: std::path::PathBuf::from("/tmp/kernel"), + initramfs: None, + root_disk: None, + data_disk: None, + seed_iso: None, + cpus: 1, + memory_mb: 256, + networks: vec![], + shared_dirs: vec![], + serial_log: std::path::PathBuf::from("/tmp/serial.log"), + cmdline: None, + netns: None, + vsock: false, + machine_id: None, + efi_variable_store: None, + rosetta: false, + } + } + + #[test] + fn validate_rejects_empty_name() { + let config = test_vm_config(""); + let err = config + .validate() + .expect_err("empty VM name should fail validation") + .to_string(); + assert!(err.contains("empty"), "expected 'empty' in error: {}", err); + } + + #[test] + fn validate_rejects_path_traversal() { + let config = test_vm_config("../etc"); + let err = config + .validate() + .expect_err("path traversal characters should fail validation") + .to_string(); + assert!(err.contains("alphanumeric") || err.contains("characters"), + "expected name validation error: {}", err); + } + + #[test] + fn validate_rejects_zero_cpus() { + let mut config = test_vm_config("good-name"); + config.cpus = 0; + let err = config + .validate() + .expect_err("zero CPUs should fail validation") + .to_string(); + assert!(err.contains("cpus"), "expected 'cpus' in error: {}", err); + } + + #[test] + fn validate_accepts_valid_config() { + let config = test_vm_config("my-vm.01"); + config.validate().expect("valid config should pass validation"); + } } diff --git a/src/driver/apple_vz.rs b/src/driver/apple_vz.rs index 5d05c81..724ffed 100644 --- a/src/driver/apple_vz.rs +++ b/src/driver/apple_vz.rs @@ -57,7 +57,7 @@ impl AppleVzDriver { // for the lifetime of the driver entry. match unsafe { vm.state() } { VZVirtualMachineState::VZVirtualMachineStateRunning => { - if let Some(ip) = check_ready_marker(&handle.serial_log) { + if let Some(ip) = super::check_ready_marker(&handle.serial_log) { VmState::Running { ip } } else { VmState::Starting @@ -274,25 +274,33 @@ impl VmDriver for AppleVzDriver { registry.insert(name.to_string(), vm.clone()); } - // Start VM on its dispatch queue + // Start VM on its dispatch queue with synchronous error reporting via channel + let (tx, rx) = std::sync::mpsc::channel::>(); let name_for_log = name.to_string(); let name_for_err = name.to_string(); let vm_for_start = vm.clone(); let dispatch_block = ConcreteBlock::new(move || { tracing::debug!("dispatch block running for VM '{}'", name_for_log); let name_err = name_for_err.clone(); + let tx_clone = tx.clone(); let completion_handler = ConcreteBlock::new(move |err: Id| { if err == NIL { tracing::info!("VM '{}' started successfully", name_err); + let _ = tx_clone.send(Ok(())); } else { // SAFETY: err is a valid NSError pointer from the framework. let error = unsafe { NSError(StrongPtr::retain(err)) }; - tracing::error!( - "VM '{}' start FAILED (code {}): {}", - name_err, + let msg = format!( + "code {}: {}", error.code(), error.localized_description().as_str() ); + tracing::error!( + "VM '{}' start FAILED {}", + name_err, + msg + ); + let _ = tx_clone.send(Err(msg)); } }); let completion_handler = completion_handler.copy(); @@ -306,8 +314,30 @@ impl VmDriver for AppleVzDriver { dispatch_async(queue, dispatch_block); } - // Give dispatch queue a moment to fire - std::thread::sleep(std::time::Duration::from_millis(500)); + // Wait for the completion handler to fire (up to 10 seconds) + match rx.recv_timeout(std::time::Duration::from_secs(10)) { + Ok(Ok(())) => {} + Ok(Err(error_msg)) => { + self.vms + .lock() + .map_err(|e| VmError::Hypervisor(format!("VM registry lock poisoned: {}", e)))? + .remove(name); + return Err(VmError::BootFailed { + name: name.clone(), + detail: format!("Apple VZ start failed: {}", error_msg), + }); + } + Err(_) => { + self.vms + .lock() + .map_err(|e| VmError::Hypervisor(format!("VM registry lock poisoned: {}", e)))? + .remove(name); + return Err(VmError::BootFailed { + name: name.clone(), + detail: "Apple VZ start completion handler did not fire within 10 seconds".into(), + }); + } + } Ok(VmHandle { name: name.clone(), @@ -315,6 +345,7 @@ impl VmDriver for AppleVzDriver { state: VmState::Starting, process: None, // Apple VZ runs in-process, no separate VM monitor PID serial_log: config.serial_log.clone(), + machine_id: None, }) } @@ -381,7 +412,7 @@ impl VmDriver for AppleVzDriver { } fn state(&self, handle: &VmHandle) -> Result { - let mut registry = self + let registry = self .vms .lock() .map_err(|e| VmError::Hypervisor(format!("VM registry lock poisoned: {}", e)))?; @@ -390,10 +421,6 @@ impl VmDriver for AppleVzDriver { None => VmState::Stopped, }; - if matches!(state, VmState::Stopped | VmState::Failed { .. }) { - registry.remove(&handle.name); - } - tracing::debug!(vm = %handle.name, state = %state, "Apple VZ VM state queried"); Ok(state) } @@ -414,98 +441,3 @@ fn resolve_path(path: &Path, label: &str) -> Result { .ok_or_else(|| VmError::InvalidConfig(format!("non-UTF8 {} path", label))) .map(|s| s.to_string()) } - -/// Check the console log for the readiness marker and extract the IP. -/// -/// The init script writes `VMRS_READY ` to the console -/// when the VM is fully booted and ready for connections. -fn check_ready_marker(log_path: &Path) -> Option { - let content = match fs::read_to_string(log_path) { - Ok(c) => c, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => return None, - Err(e) => { - tracing::warn!(path = %log_path.display(), "failed to read serial log: {}", e); - return None; - } - }; - let pos = content.find(crate::config::READY_MARKER)?; - let after = &content[pos + crate::config::READY_MARKER.len()..]; - let ip = after.split_whitespace().next()?.trim().to_string(); - if ip.is_empty() { - None - } else { - Some(ip) - } -} - -/// Watch a console log file for the VM readiness marker. -/// -/// Blocks until either the marker is found or the stop flag is set. -/// Returns the IP address if found. -pub fn watch_for_ready( - log_path: &Path, - stop: &std::sync::atomic::AtomicBool, -) -> Option { - use std::io::{Read, Seek, SeekFrom}; - use std::sync::atomic::Ordering; - - let mut last_size = 0u64; - let mut accumulated = String::new(); - - while !stop.load(Ordering::Relaxed) { - let file = match fs::File::open(log_path) { - Ok(f) => f, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - // Log file not created yet — normal during early boot - std::thread::sleep(std::time::Duration::from_secs(1)); - continue; - } - Err(e) => { - tracing::warn!(path = %log_path.display(), "failed to open serial log: {}", e); - std::thread::sleep(std::time::Duration::from_secs(1)); - continue; - } - }; - - let metadata = match file.metadata() { - Ok(m) => m, - Err(e) => { - tracing::warn!(path = %log_path.display(), "failed to read serial log metadata: {}", e); - std::thread::sleep(std::time::Duration::from_secs(1)); - continue; - } - }; - - let current_size = metadata.len(); - if current_size > last_size { - let mut file = file; - if let Err(e) = file.seek(SeekFrom::Start(last_size)) { - tracing::warn!(path = %log_path.display(), "failed to seek serial log: {}", e); - std::thread::sleep(std::time::Duration::from_secs(1)); - continue; - } - let mut buf = vec![0u8; (current_size - last_size) as usize]; - match file.read(&mut buf) { - Ok(n) => accumulated.push_str(&String::from_utf8_lossy(&buf[..n])), - Err(e) => { - tracing::warn!(path = %log_path.display(), "failed to read serial log: {}", e); - std::thread::sleep(std::time::Duration::from_secs(1)); - continue; - } - } - last_size = current_size; - - if let Some(pos) = accumulated.find(crate::config::READY_MARKER) { - let after = &accumulated[pos + crate::config::READY_MARKER.len()..]; - if let Some(ip) = after.split_whitespace().next() { - let ip = ip.trim().to_string(); - if !ip.is_empty() { - return Some(ip); - } - } - } - } - std::thread::sleep(std::time::Duration::from_secs(1)); - } - None -} diff --git a/src/driver/boot.rs b/src/driver/boot.rs index f534afa..f43cd2b 100644 --- a/src/driver/boot.rs +++ b/src/driver/boot.rs @@ -54,6 +54,14 @@ pub const GDT_DATA_SELECTOR: u16 = 0x10; /// `memory_mb` of physical address space 1:1 (virtual = physical). /// /// Layout: PML4[0] → PDPT[0] → PD[0..N] (2MB pages each) +/// +/// # Limitation +/// +/// A single Page Directory has 512 entries of 2 MB each, covering at most +/// 1 GB (1024 MB). Callers requesting more than 1024 MB of guest memory +/// must validate beforehand or the mapping will silently cap at 1 GB. +/// Support for multiple Page Directories (>1 GB) will be added in a future +/// release. pub fn setup_page_tables(memory: &mut [u8], memory_mb: usize) { let pml4 = PML4_ADDR as usize; let pdpt = PDPT_ADDR as usize; @@ -124,6 +132,14 @@ pub fn load_kernel_from_bytes( cmdline: &str, memory_mb: usize, ) -> Result { + if memory_mb > 1024 { + return Err(VmError::InvalidConfig(format!( + "direct boot currently supports up to 1024 MB guest memory (got {} MB); \ + support for larger memory will be added in a future release", + memory_mb + ))); + } + if kernel_data.len() < 0x250 { return Err(VmError::InvalidConfig( "kernel image too small for bzImage".into(), diff --git a/src/driver/cloud_hv.rs b/src/driver/cloud_hv.rs index 441d1c6..b15a849 100644 --- a/src/driver/cloud_hv.rs +++ b/src/driver/cloud_hv.rs @@ -252,6 +252,7 @@ impl VmDriver for CloudHvDriver { state: VmState::Starting, process: Some(identity), serial_log: config.serial_log.clone(), + machine_id: None, }) } @@ -276,7 +277,7 @@ impl VmDriver for CloudHvDriver { ), }); } - wait_for_pid_exit(process, std::time::Duration::from_secs(10)); + wait_for_pid_exit(process, &handle.name, std::time::Duration::from_secs(10))?; return Ok(()); } else { return Err(VmError::NotFound { @@ -370,7 +371,7 @@ impl VmDriver for CloudHvDriver { } else { return Ok(VmState::Stopped); } - if let Some(ip) = check_ready_marker(&handle.serial_log) { + if let Some(ip) = super::check_ready_marker(&handle.serial_log) { return Ok(VmState::Running { ip }); } return Ok(VmState::Starting); @@ -388,7 +389,7 @@ impl VmDriver for CloudHvDriver { } // Process alive — check serial log for readiness marker - if let Some(ip) = check_ready_marker(&handle.serial_log) { + if let Some(ip) = super::check_ready_marker(&handle.serial_log) { Ok(VmState::Running { ip }) } else { Ok(VmState::Starting) @@ -440,18 +441,11 @@ fn find_ch_binary() -> Result { /// Generate a deterministic MAC address from a VM name. /// Uses the QEMU OUI prefix (52:54:00) for locally administered addresses. fn generate_mac(name: &str) -> String { - use std::collections::hash_map::DefaultHasher; - use std::hash::{Hash, Hasher}; - - let mut hasher = DefaultHasher::new(); - name.hash(&mut hasher); - let hash = hasher.finish(); - + use sha2::{Digest, Sha256}; + let hash = Sha256::digest(name.as_bytes()); format!( "52:54:00:{:02x}:{:02x}:{:02x}", - (hash >> 16) as u8, - (hash >> 8) as u8, - hash as u8 + hash[0], hash[1], hash[2] ) } @@ -514,27 +508,6 @@ fn cleanup_virtiofsd(pids: &[u32]) { } } -/// Check the serial console log for the readiness marker. -/// The guest writes `VMRS_READY ` when boot completes. -fn check_ready_marker(log_path: &Path) -> Option { - let content = match std::fs::read_to_string(log_path) { - Ok(c) => c, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => return None, - Err(e) => { - tracing::warn!(path = %log_path.display(), "failed to read serial log: {}", e); - return None; - } - }; - let pos = content.find(crate::config::READY_MARKER)?; - let after = &content[pos + crate::config::READY_MARKER.len()..]; - let ip = after.split_whitespace().next()?.trim().to_string(); - if ip.is_empty() { - None - } else { - Some(ip) - } -} - /// Wait for a tracked child process to exit. /// If the process doesn't exit within the timeout, escalates to SIGKILL. fn wait_for_exit(mut child: Child, timeout: std::time::Duration) { @@ -564,16 +537,43 @@ fn wait_for_exit(mut child: Child, timeout: std::time::Duration) { } } -fn wait_for_pid_exit(process: &VmmProcess, timeout: std::time::Duration) { +fn wait_for_pid_exit( + process: &VmmProcess, + vm_name: &str, + timeout: std::time::Duration, +) -> Result<(), VmError> { let start = std::time::Instant::now(); while start.elapsed() < timeout { if !pid_exists(process) { tracing::debug!(pid = process.pid(), "process exited"); - return; + return Ok(()); } std::thread::sleep(std::time::Duration::from_millis(200)); } - tracing::warn!(pid = process.pid(), elapsed_ms = %timeout.as_millis(), "process did not exit within timeout"); + tracing::warn!(pid = process.pid(), elapsed_ms = %timeout.as_millis(), "process did not exit within timeout, sending SIGKILL"); + // SAFETY: Sending SIGKILL to a PID we validated still belongs to our process. + let ret = unsafe { libc::kill(process.pid() as i32, libc::SIGKILL) }; + if ret != 0 { + let errno = std::io::Error::last_os_error(); + return Err(VmError::StopFailed { + name: vm_name.to_string(), + detail: format!("failed to SIGKILL restored VM PID {}: {}", process.pid(), errno), + }); + } + // Brief wait for SIGKILL to take effect + let kill_deadline = std::time::Instant::now() + std::time::Duration::from_secs(2); + while std::time::Instant::now() < kill_deadline { + if !pid_exists(process) { + tracing::debug!(pid = process.pid(), "process exited after SIGKILL"); + return Ok(()); + } + std::thread::sleep(std::time::Duration::from_millis(100)); + } + tracing::error!(pid = process.pid(), "process still alive after SIGKILL"); + Err(VmError::StopFailed { + name: vm_name.to_string(), + detail: format!("restored VM PID {} remained alive after SIGKILL", process.pid()), + }) } fn pid_exists(process: &VmmProcess) -> bool { diff --git a/src/driver/mod.rs b/src/driver/mod.rs index 6f2fdbf..5a5404d 100644 --- a/src/driver/mod.rs +++ b/src/driver/mod.rs @@ -37,6 +37,26 @@ pub trait VmDriver: Send + Sync { /// Query current VM state. fn state(&self, handle: &VmHandle) -> Result; + + /// Pause a running VM (suspend execution, preserve state). + /// + /// Not all drivers support pause. The default returns an error. + fn pause(&self, handle: &VmHandle) -> Result<(), VmError> { + Err(VmError::Hypervisor(format!( + "pause is not supported by this driver for VM '{}'", + handle.name + ))) + } + + /// Resume a paused VM. + /// + /// Not all drivers support resume. The default returns an error. + fn resume(&self, handle: &VmHandle) -> Result<(), VmError> { + Err(VmError::Hypervisor(format!( + "resume is not supported by this driver for VM '{}'", + handle.name + ))) + } } /// VM operation errors. @@ -70,3 +90,34 @@ pub enum VmError { #[error("invalid config: {0}")] InvalidConfig(String), } + +impl From for VmError { + fn from(e: crate::oci::registry::OciError) -> Self { + VmError::Hypervisor(format!("OCI error: {}", e)) + } +} + +impl From for VmError { + fn from(e: crate::setup::SetupError) -> Self { + VmError::Hypervisor(format!("setup error: {}", e)) + } +} + +/// Check a serial console log for the VM readiness marker. +/// +/// The guest writes `VMRS_READY ` when boot completes. +/// Returns `Some(ip)` if found, `None` otherwise. +pub(crate) fn check_ready_marker(log_path: &std::path::Path) -> Option { + let content = match std::fs::read_to_string(log_path) { + Ok(c) => c, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return None, + Err(e) => { + tracing::warn!(path = %log_path.display(), "failed to read serial log: {}", e); + return None; + } + }; + let pos = content.find(crate::config::READY_MARKER)?; + let after = &content[pos + crate::config::READY_MARKER.len()..]; + let ip = after.split_whitespace().next()?.trim().to_string(); + if ip.is_empty() { None } else { Some(ip) } +} diff --git a/src/driver/whp.rs b/src/driver/whp.rs index afff742..bff9619 100644 --- a/src/driver/whp.rs +++ b/src/driver/whp.rs @@ -376,7 +376,7 @@ impl VmDriver for WhpDriver { name: name.clone(), namespace: config.namespace.clone(), state: VmState::Starting, - pid: None, // In-process, no separate PID + process: None, // In-process, no separate PID serial_log, machine_id: None, }) @@ -808,6 +808,7 @@ fn handle_io_port( if !ip.is_empty() { tracing::info!(vm = %vm_name, ip = %ip, "VM ready"); update_state(state, VmState::Running { ip }); + serial_buffer.clear(); } } } diff --git a/src/ffi/apple_vz/base.rs b/src/ffi/apple_vz/base.rs index a390334..596c615 100644 --- a/src/ffi/apple_vz/base.rs +++ b/src/ffi/apple_vz/base.rs @@ -6,7 +6,6 @@ use std::slice; use std::str; use block::Block; -use libc::c_void; use objc::rc::StrongPtr; use objc::runtime::{Object, BOOL, NO, YES}; use objc::{class, msg_send, sel, sel_impl}; @@ -17,8 +16,6 @@ extern "C" {} #[link(name = "Foundation", kind = "framework")] extern "C" { pub fn dispatch_queue_create(label: *const libc::c_char, attr: Id) -> Id; - // pub fn dispatch_sync(queue: Id, block: &Block<(), ()>); - pub fn dispatch_sync(queue: Id, block: *mut c_void); pub fn dispatch_async(queue: Id, block: &Block<(), ()>); // pub fn dispatch_async(queue: Id, block: *mut c_void); } @@ -168,7 +165,7 @@ impl NSFileHandle { pub unsafe fn file_handle_with_fd(fd: i32) -> NSFileHandle { let alloc: Id = msg_send![class!(NSFileHandle), alloc]; let p = StrongPtr::new( - msg_send![alloc, initWithFileDescriptor: fd closeOnDealloc: NO], + msg_send![alloc, initWithFileDescriptor: fd closeOnDealloc: YES], ); NSFileHandle(p) } @@ -251,11 +248,10 @@ impl NSError { pub fn dump(&self) { if *self.0 == NIL { - println!("NSError: nil"); + tracing::error!("NSError: nil"); return; } let code = self.code(); - println!("code: {}", code); // Helper: safely print an NSString that may be backed by nil fn safe_str(s: &NSString) -> Cow<'_, str> { @@ -263,12 +259,16 @@ impl NSError { } let desc = self.localized_description(); - println!("localizedDescription : {}", safe_str(&desc)); let reason = self.localized_failure_reason(); - println!("localizedFailureReason : {}", safe_str(&reason)); let suggestion = self.localized_recovery_suggestion(); - println!("localizedRecoverySuggestion : {}", safe_str(&suggestion)); let anchor = self.help_anchor(); - println!("helpAnchor : {}", safe_str(&anchor)); + tracing::error!( + code = code, + localized_description = %safe_str(&desc), + localized_failure_reason = %safe_str(&reason), + localized_recovery_suggestion = %safe_str(&suggestion), + help_anchor = %safe_str(&anchor), + "NSError dump" + ); } } diff --git a/src/ffi/apple_vz/mod.rs b/src/ffi/apple_vz/mod.rs index c09371b..dac72f4 100644 --- a/src/ffi/apple_vz/mod.rs +++ b/src/ffi/apple_vz/mod.rs @@ -20,6 +20,7 @@ pub mod boot_loader; pub mod entropy_device; pub mod memory_device; pub mod network_device; +pub mod platform; pub mod serial_port; pub mod shared_directory; pub mod socket_device; diff --git a/src/lib.rs b/src/lib.rs index d2e7577..bf370ea 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -25,8 +25,10 @@ pub mod setup; pub mod vm; // Re-exports for convenience +#[cfg(unix)] +pub use config::VmSocketEndpoint; pub use config::{ - NetworkAttachment, SharedDir, VmConfig, VmHandle, VmSocketEndpoint, VmState, VmmProcess, + NetworkAttachment, SharedDir, VmConfig, VmHandle, VmState, VmmProcess, READY_MARKER, }; pub use driver::{VmDriver, VmError}; diff --git a/src/network/bridge.rs b/src/network/bridge.rs index 5ce1cff..89c252a 100644 --- a/src/network/bridge.rs +++ b/src/network/bridge.rs @@ -9,10 +9,27 @@ use std::process::{Command, Stdio}; use crate::driver::VmError; +fn validate_interface_name(name: &str) -> Result<(), VmError> { + if name.is_empty() || name.len() > 15 { + return Err(VmError::InvalidConfig(format!( + "interface name must be 1-15 characters, got {} characters", + name.len() + ))); + } + if !name.chars().all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') { + return Err(VmError::InvalidConfig(format!( + "interface name '{}' contains invalid characters (only alphanumeric, hyphen, underscore allowed)", + name + ))); + } + Ok(()) +} + /// Ensure a Linux bridge exists with the given gateway IP. /// /// Idempotent — if the bridge already exists, only ensures it's up with the right IP. pub fn ensure_bridge(name: &str, gateway_ip: &str, subnet_cidr: &str) -> Result<(), VmError> { + validate_interface_name(name)?; // Create bridge if it doesn't exist if !link_exists(name) { run_ip(&["link", "add", name, "type", "bridge"])?; @@ -54,6 +71,7 @@ pub fn ensure_bridge(name: &str, gateway_ip: &str, subnet_cidr: &str) -> Result< /// Create a TAP device. pub fn create_tap(name: &str) -> Result<(), VmError> { + validate_interface_name(name)?; run_ip(&["tuntap", "add", "dev", name, "mode", "tap"])?; run_ip(&["link", "set", name, "up"])?; tracing::debug!(tap = %name, "TAP device created"); @@ -62,6 +80,7 @@ pub fn create_tap(name: &str) -> Result<(), VmError> { /// Add a TAP device to a bridge. pub fn add_to_bridge(tap: &str, bridge: &str) -> Result<(), VmError> { + validate_interface_name(tap)?; run_ip(&["link", "set", tap, "master", bridge])?; tracing::debug!(tap = %tap, bridge = %bridge, "TAP added to bridge"); Ok(()) diff --git a/src/network/mod.rs b/src/network/mod.rs index 2f047e0..6f78828 100644 --- a/src/network/mod.rs +++ b/src/network/mod.rs @@ -4,9 +4,12 @@ //! - **macOS**: `NetworkSwitch` — userspace L2 learning bridge via socketpairs //! - **Linux**: `bridge` module — Linux kernel bridge + TAP devices + iptables NAT +#[cfg(unix)] pub mod switch; pub mod port_forward; +#[cfg(target_os = "linux")] pub mod bridge; +#[cfg(unix)] pub use switch::NetworkSwitch; pub use port_forward::PortForwarder; diff --git a/src/network/switch.rs b/src/network/switch.rs index 3118a4c..5c260c4 100644 --- a/src/network/switch.rs +++ b/src/network/switch.rs @@ -58,12 +58,12 @@ impl std::fmt::Display for MacAddress { #[derive(Debug)] struct SwitchPort { /// The switch's end of the socketpair. The other end goes to the VM. - fd: RawFd, + fd: OwnedFd, } impl AsRawFd for SwitchPort { fn as_raw_fd(&self) -> RawFd { - self.fd + self.fd.as_raw_fd() } } @@ -115,8 +115,7 @@ impl NetworkSwitch { .map_err(|e| io::Error::other(format!("lock poisoned: {}", e)))?; mac_tables.entry(network_id.to_string()).or_default(); - // SAFETY: `socketpair` returned a fresh owned file descriptor for the VM side. - Ok(VmSocketEndpoint::new(unsafe { OwnedFd::from_raw_fd(vm_fd) })) + Ok(VmSocketEndpoint::new(vm_fd)) } /// Start the forwarding loop on a background thread. @@ -178,29 +177,12 @@ impl Default for NetworkSwitch { impl Drop for NetworkSwitch { fn drop(&mut self) { self.stop(); - match self.networks.lock() { - Ok(networks) => { - for ports in networks.values() { - for port in ports { - // SAFETY: Closing file descriptors we created in add_port(). - let ret = unsafe { libc::close(port.fd) }; - if ret != 0 { - // Log but don't panic in Drop — best we can do. - let err = std::io::Error::last_os_error(); - eprintln!("vm-rs: failed to close switch port fd {}: {}", port.fd, err); - } - } - } - } - Err(e) => { - eprintln!("vm-rs: network switch lock poisoned during drop: {}", e); - } - } + // OwnedFd in SwitchPort handles closing file descriptors automatically. } } /// Create a Unix SOCK_DGRAM socketpair. Returns (switch_fd, vm_fd). -fn create_socketpair() -> io::Result<(RawFd, RawFd)> { +fn create_socketpair() -> io::Result<(OwnedFd, OwnedFd)> { let mut fds = [0i32; 2]; // SAFETY: Standard POSIX socketpair call with valid fd array. let ret = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_DGRAM, 0, fds.as_mut_ptr()) }; @@ -208,25 +190,24 @@ fn create_socketpair() -> io::Result<(RawFd, RawFd)> { return Err(io::Error::last_os_error()); } + // SAFETY: socketpair just created these fresh file descriptors; wrapping + // them in OwnedFd transfers ownership so they are closed on drop. + let switch_fd = unsafe { OwnedFd::from_raw_fd(fds[0]) }; + let vm_fd = unsafe { OwnedFd::from_raw_fd(fds[1]) }; + // Set switch side non-blocking for the poll loop. // SAFETY: fcntl on file descriptors we just created in the socketpair above. unsafe { - let flags = libc::fcntl(fds[0], libc::F_GETFL); + let flags = libc::fcntl(switch_fd.as_raw_fd(), libc::F_GETFL); if flags == -1 { - let err = io::Error::last_os_error(); - libc::close(fds[0]); - libc::close(fds[1]); - return Err(err); + return Err(io::Error::last_os_error()); } - if libc::fcntl(fds[0], libc::F_SETFL, flags | libc::O_NONBLOCK) == -1 { - let err = io::Error::last_os_error(); - libc::close(fds[0]); - libc::close(fds[1]); - return Err(err); + if libc::fcntl(switch_fd.as_raw_fd(), libc::F_SETFL, flags | libc::O_NONBLOCK) == -1 { + return Err(io::Error::last_os_error()); } } - Ok((fds[0], fds[1])) + Ok((switch_fd, vm_fd)) } /// The main forwarding loop. Runs until `running` is set to false. @@ -266,11 +247,11 @@ fn forwarding_loop( let mut port_fds: HashMap> = HashMap::new(); for (net_id, ports) in nets.iter() { - let fds: Vec = ports.iter().map(|p| p.fd).collect(); + let fds: Vec = ports.iter().map(|p| p.fd.as_raw_fd()).collect(); port_fds.insert(net_id.clone(), fds); for (idx, port) in ports.iter().enumerate() { pollfds.push(libc::pollfd { - fd: port.fd, + fd: port.fd.as_raw_fd(), events: libc::POLLIN, revents: 0, }); @@ -377,6 +358,7 @@ fn forwarding_loop( } } } + running.store(false, Ordering::SeqCst); } /// Send a single Ethernet frame to a socket fd. Best-effort (drops if buffer full). diff --git a/src/oci/registry.rs b/src/oci/registry.rs index af0c9d7..6e69234 100644 --- a/src/oci/registry.rs +++ b/src/oci/registry.rs @@ -83,6 +83,8 @@ pub async fn pull(image: &str, store: &ImageStore) -> Result String { } fn unsanitize_name(s: &str) -> String { - let result = s.replace("_slash_", "/").replace("_colon_", ":"); - if result == s && s.contains('_') && !s.contains('/') { - return s.replacen('_', "/", 1); - } - result + s.replace("_slash_", "/").replace("_colon_", ":") } #[cfg(test)] diff --git a/src/setup/image.rs b/src/setup/image.rs index fdf1388..e21b498 100644 --- a/src/setup/image.rs +++ b/src/setup/image.rs @@ -54,6 +54,7 @@ pub struct ImageSpec { /// A prepared image — all files needed to boot a VM. #[derive(Debug, Clone)] +#[must_use] pub struct PreparedImage { /// Path to the kernel image. pub kernel: PathBuf, @@ -164,7 +165,11 @@ pub async fn prepare_image( std::fs::create_dir_all(&image_dir).map_err(SetupError::Io)?; let assets = resolve_image(spec)?; - let client = reqwest::Client::new(); + let client = reqwest::Client::builder() + .connect_timeout(std::time::Duration::from_secs(30)) + .timeout(std::time::Duration::from_secs(600)) + .build() + .map_err(|e| SetupError::AssetDownload(format!("failed to create HTTP client: {}", e)))?; let mut checksum_cache: HashMap> = HashMap::new(); for asset in &assets { @@ -300,7 +305,15 @@ async fn download_file( verify_bytes(&bytes, expected, url)?; } - std::fs::write(path, &bytes).map_err(SetupError::Io)?; + // Write to temp file then atomic rename to avoid corrupt files on crash + let tmp_path = path.with_extension("tmp"); + std::fs::write(&tmp_path, &bytes).map_err(SetupError::Io)?; + // fsync the file to ensure data is on disk before rename + { + let f = std::fs::File::open(&tmp_path).map_err(SetupError::Io)?; + f.sync_all().map_err(SetupError::Io)?; + } + std::fs::rename(&tmp_path, path).map_err(SetupError::Io)?; tracing::info!( path = %path.display(), bytes = bytes.len(), diff --git a/src/setup/seed.rs b/src/setup/seed.rs index 759a57d..e88d36b 100644 --- a/src/setup/seed.rs +++ b/src/setup/seed.rs @@ -5,9 +5,13 @@ //! SSH keys, network config, startup scripts, and health checks. use std::path::Path; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::{Duration, SystemTime, UNIX_EPOCH}; use super::SetupError; +static SEED_TMP_COUNTER: AtomicU64 = AtomicU64::new(0); + // --------------------------------------------------------------------------- // Types // --------------------------------------------------------------------------- @@ -89,10 +93,21 @@ pub struct HealthCheckConfig { /// On macOS: uses `hdiutil makehybrid` to create the ISO. /// On Linux: uses `genisoimage` or `mkisofs`. pub fn create_seed_iso(iso_path: &Path, config: &SeedConfig<'_>) -> Result<(), SetupError> { - let tmp_dir = iso_path + let parent = iso_path .parent() - .ok_or_else(|| SetupError::Config("no parent directory for ISO path".into()))? - .join(format!(".seed-{}", config.hostname)); + .ok_or_else(|| SetupError::Config("no parent directory for ISO path".into()))?; + let counter = SEED_TMP_COUNTER.fetch_add(1, Ordering::Relaxed); + let timestamp_nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or(Duration::ZERO) + .as_nanos(); + let tmp_dir = parent.join(format!( + ".seed-{}-{}-{}-{}", + config.hostname, + std::process::id(), + timestamp_nanos, + counter + )); std::fs::create_dir_all(&tmp_dir).map_err(SetupError::Io)?; diff --git a/src/vm.rs b/src/vm.rs index 48941a3..1c67e9a 100644 --- a/src/vm.rs +++ b/src/vm.rs @@ -57,6 +57,8 @@ impl VmManager { /// Boot a VM. Creates the VM directory and delegates to the driver. pub fn start(&self, config: &VmConfig) -> Result { + config.validate()?; + // Reserve the name up front so concurrent callers cannot boot the same VM twice. { let mut vms = self @@ -64,7 +66,7 @@ impl VmManager { .write() .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; if let Some(existing) = vms.get(&config.name) { - if existing.state != VmState::Stopped { + if !matches!(existing.state, VmState::Stopped | VmState::Failed { .. }) { return Err(VmError::BootFailed { name: config.name.clone(), detail: format!("VM already exists in state: {}", existing.state), @@ -79,13 +81,19 @@ impl VmManager { state: VmState::Starting, process: None, serial_log: config.serial_log.clone(), + machine_id: None, }, ); } // Ensure VM directory exists let vm_dir = self.vm_dir(&config.name); - std::fs::create_dir_all(&vm_dir).map_err(VmError::Io)?; + if let Err(e) = std::fs::create_dir_all(&vm_dir) { + let mut vms = self.vms.write() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + vms.remove(&config.name); + return Err(VmError::Io(e)); + } tracing::info!( vm = %config.name, @@ -97,6 +105,7 @@ impl VmManager { let handle = match self.driver.boot(config) { Ok(handle) => handle, Err(err) => { + tracing::warn!(vm = %config.name, error = %err, "VM boot failed, cleaning up reservation"); let mut vms = self .vms .write() @@ -117,6 +126,7 @@ impl VmManager { /// Stop a VM gracefully. pub fn stop(&self, name: &str) -> Result<(), VmError> { + tracing::info!(vm = %name, "stopping VM"); let handle = self.get_handle(name)?; self.driver.stop(&handle)?; @@ -130,6 +140,7 @@ impl VmManager { /// Force-kill a VM. pub fn kill(&self, name: &str) -> Result<(), VmError> { + tracing::info!(vm = %name, "force-killing VM"); let handle = self.get_handle(name)?; self.driver.kill(&handle)?; @@ -145,6 +156,7 @@ impl VmManager { /// Use this when the VM was started by a previous daemon process and isn't /// tracked in the in-memory map. pub fn stop_by_handle(&self, handle: &VmHandle) -> Result<(), VmError> { + tracing::info!(vm = %handle.name, "stopping VM by handle"); self.driver.stop(handle)?; let mut vms = self.vms.write().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; @@ -156,6 +168,7 @@ impl VmManager { /// Force-kill a VM using a pre-built handle (e.g. restored from persisted metadata). pub fn kill_by_handle(&self, handle: &VmHandle) -> Result<(), VmError> { + tracing::info!(vm = %handle.name, "force-killing VM by handle"); self.driver.kill(handle)?; let mut vms = self.vms.write().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; @@ -165,6 +178,28 @@ impl VmManager { Ok(()) } + /// Pause a running VM. + pub fn pause(&self, name: &str) -> Result<(), VmError> { + let handle = self.get_handle(name)?; + self.driver.pause(&handle)?; + let mut vms = self.vms.write().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + if let Some(h) = vms.get_mut(name) { + h.state = VmState::Paused; + } + Ok(()) + } + + /// Resume a paused VM. + pub fn resume(&self, name: &str) -> Result<(), VmError> { + let handle = self.get_handle(name)?; + self.driver.resume(&handle)?; + let mut vms = self.vms.write().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + if let Some(h) = vms.get_mut(name) { + h.state = VmState::Starting; + } + Ok(()) + } + /// Query current state of a VM. pub fn state(&self, name: &str) -> Result { let handle = self.get_handle(name)?; @@ -237,6 +272,20 @@ impl VmManager { return Ok(()); } + { + let pending: Vec = { + let vms = self.vms.read().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + vms.iter() + .filter(|(_, h)| !matches!(h.state, VmState::Running { .. })) + .map(|(name, _)| name.clone()) + .collect() + }; + let elapsed = start.elapsed().as_secs(); + if elapsed > 0 && elapsed.is_multiple_of(10) { + tracing::info!(pending = ?pending, elapsed_secs = start.elapsed().as_secs(), "waiting for VMs to become ready"); + } + } + std::thread::sleep(std::time::Duration::from_secs(1)); } } @@ -247,8 +296,19 @@ impl VmManager { /// On Linux: reflink (`cp --reflink=auto`), falls back to regular copy. pub fn clone_disk(base: &Path, target: &Path) -> Result<(), VmError> { if target.exists() { - tracing::debug!(target = %target.display(), "disk clone target already exists, skipping"); - return Ok(()); + if let (Ok(base_meta), Ok(target_meta)) = (std::fs::metadata(base), std::fs::metadata(target)) { + if base_meta.len() == target_meta.len() { + tracing::debug!(target = %target.display(), "disk clone target already exists with matching size, skipping"); + return Ok(()); + } + tracing::warn!( + target = %target.display(), + base_size = base_meta.len(), + target_size = target_meta.len(), + "disk clone target exists but size differs, re-cloning" + ); + std::fs::remove_file(target).map_err(VmError::Io)?; + } } // Ensure parent directory exists @@ -294,6 +354,11 @@ impl VmManager { } } + #[cfg(not(any(target_os = "macos", target_os = "linux")))] + { + std::fs::copy(base, target).map_err(VmError::Io)?; + } + Ok(()) } @@ -341,6 +406,8 @@ mod tests { release_rx: Mutex>>, } + struct FailedStateDriver; + impl VmDriver for Arc { fn boot(&self, config: &VmConfig) -> Result { self.boot_calls.fetch_add(1, Ordering::SeqCst); @@ -353,6 +420,7 @@ mod tests { state: VmState::Starting, process: None, serial_log: config.serial_log.clone(), + machine_id: None, }) } @@ -369,6 +437,33 @@ mod tests { } } + impl VmDriver for FailedStateDriver { + fn boot(&self, config: &VmConfig) -> Result { + Ok(VmHandle { + name: config.name.clone(), + namespace: config.namespace.clone(), + state: VmState::Starting, + process: None, + serial_log: config.serial_log.clone(), + machine_id: None, + }) + } + + fn stop(&self, _handle: &VmHandle) -> Result<(), VmError> { + Ok(()) + } + + fn kill(&self, _handle: &VmHandle) -> Result<(), VmError> { + Ok(()) + } + + fn state(&self, _handle: &VmHandle) -> Result { + Ok(VmState::Failed { + reason: "crashed".into(), + }) + } + } + fn test_config(base_dir: &Path) -> VmConfig { VmConfig { name: "test-vm".into(), @@ -385,6 +480,10 @@ mod tests { serial_log: base_dir.join("serial.log"), cmdline: None, netns: None, + vsock: false, + machine_id: None, + efi_variable_store: None, + rosetta: false, } } @@ -418,4 +517,18 @@ mod tests { assert_eq!(driver.boot_calls.load(Ordering::SeqCst), 1); } + + #[test] + fn restart_is_allowed_after_failed_state_is_observed() { + let tmp = tempfile::tempdir().expect("tempdir"); + let manager = VmManager::with_driver(Box::new(FailedStateDriver), tmp.path().to_path_buf()) + .expect("manager"); + let config = test_config(tmp.path()); + + manager.start(&config).expect("first boot"); + let state = manager.state(&config.name).expect("state query"); + assert!(matches!(state, VmState::Failed { .. })); + + manager.start(&config).expect("restart after failed state should be allowed"); + } } diff --git a/tests/ffi_smoke.rs b/tests/ffi_smoke.rs index 10105f0..6f2476a 100644 --- a/tests/ffi_smoke.rs +++ b/tests/ffi_smoke.rs @@ -45,7 +45,7 @@ mod apple_vz { "should be able to restore from valid data" ); - let restored = restored.unwrap(); + let restored = restored.expect("valid machine identifier bytes should roundtrip"); let restored_data = restored.data_representation(); assert_eq!( data, restored_data, @@ -75,225 +75,24 @@ mod apple_vz { // Should not crash — that's the test } - // ─── vsock (Socket Device) ────────────────────────────────────────── + // ─── VM Configuration Builder ──────────────────────────────────── #[test] - fn vsock_device_configuration_creates() { - use vm_rs::ffi::apple_vz::socket_device::VZVirtioSocketDeviceConfiguration; - let _config = VZVirtioSocketDeviceConfiguration::new(); - } - - #[test] - fn vsock_listener_creates() { - use vm_rs::ffi::apple_vz::socket_device::VZVirtioSocketListener; - let _listener = VZVirtioSocketListener::new(); - } - - #[test] - fn vsock_config_implements_trait() { - use vm_rs::ffi::apple_vz::socket_device::{ - VZSocketDeviceConfiguration, VZVirtioSocketDeviceConfiguration, - }; - let config = VZVirtioSocketDeviceConfiguration::new(); - let id = config.id(); - assert!(!id.is_null(), "vsock config Id should not be null"); - } - - // ─── UEFI Boot ───────────────────────────────────────────────────── - - #[test] - fn efi_variable_store_create_and_open() { - use vm_rs::ffi::apple_vz::boot_loader::{VZEFIBootLoader, VZEFIVariableStore}; - - let tmp = tempfile::tempdir().expect("tempdir"); - let store_path = tmp.path().join("nvram.bin"); - let store_path_str = store_path.to_str().unwrap(); - - // Create a new variable store - let store = VZEFIVariableStore::create(store_path_str); - assert!(store.is_ok(), "should create EFI variable store"); - let store = match store { - Ok(s) => s, - Err(_) => panic!("should create EFI variable store"), - }; - - // Create boot loader with the store - let _loader = VZEFIBootLoader::new(&store); - - // Re-open the existing store - let store2 = VZEFIVariableStore::open(store_path_str); - assert!(store2.is_ok(), "should open existing EFI variable store"); - } - - #[test] - fn efi_variable_store_open_nonexistent_fails() { - use vm_rs::ffi::apple_vz::boot_loader::VZEFIVariableStore; - - let result = VZEFIVariableStore::open("/nonexistent/path/nvram.bin"); - assert!(result.is_err(), "opening nonexistent store should fail"); - } - - // ─── Shared Directories (Rosetta + Multiple) ──────────────────────── - - #[test] - fn rosetta_availability_does_not_crash() { - use vm_rs::ffi::apple_vz::shared_directory::{ - VZLinuxRosettaAvailability, VZLinuxRosettaDirectoryShare, - }; - - let avail = VZLinuxRosettaDirectoryShare::availability(); - // On Intel: NotSupported, on AS: Installed or NotInstalled - assert!(matches!( - avail, - VZLinuxRosettaAvailability::NotSupported - | VZLinuxRosettaAvailability::NotInstalled - | VZLinuxRosettaAvailability::Installed - )); - } - - #[test] - fn multiple_directory_share_creates() { - use vm_rs::ffi::apple_vz::shared_directory::{ - VZMultipleDirectoryShare, VZSharedDirectory, VZVirtioFileSystemDeviceConfiguration, - }; - - let tmp = tempfile::tempdir().expect("tempdir"); - let dir_a = tmp.path().join("a"); - let dir_b = tmp.path().join("b"); - std::fs::create_dir_all(&dir_a).unwrap(); - std::fs::create_dir_all(&dir_b).unwrap(); - - let share_a = VZSharedDirectory::new(dir_a.to_str().unwrap(), true); - let share_b = VZSharedDirectory::new(dir_b.to_str().unwrap(), false); - let multi = VZMultipleDirectoryShare::new(&[("config", share_a), ("data", share_b)]); - - let mut device = VZVirtioFileSystemDeviceConfiguration::new("shares"); - device.set_share(multi); - // No crash = success - } - - #[test] - fn single_directory_share_with_trait() { - use vm_rs::ffi::apple_vz::shared_directory::{ - VZDirectoryShare, VZSharedDirectory, VZSingleDirectoryShare, - }; - - let tmp = tempfile::tempdir().expect("tempdir"); - let dir = VZSharedDirectory::new(tmp.path().to_str().unwrap(), true); - let share = VZSingleDirectoryShare::new(dir); - let id = share.id(); - assert!(!id.is_null(), "single share Id should not be null"); - } - - // ─── NVMe Storage ────────────────────────────────────────────────── - - #[test] - fn nvme_device_configuration_creates() { - use vm_rs::ffi::apple_vz::storage_device::{ - VZDiskImageStorageDeviceAttachmentBuilder, VZNVMExpressControllerDeviceConfiguration, - VZStorageDeviceConfiguration, - }; - - let tmp = tempfile::tempdir().expect("tempdir"); - let disk = tmp.path().join("test.img"); - // Create a minimal disk image (just needs to exist) - std::fs::write(&disk, vec![0u8; 4096]).unwrap(); - - let attachment = VZDiskImageStorageDeviceAttachmentBuilder::new() - .path(disk.to_str().unwrap()) - .read_only(true) - .build(); - let attachment = match attachment { - Ok(a) => a, - Err(_) => panic!("disk attachment should build"), - }; - - let nvme = VZNVMExpressControllerDeviceConfiguration::new(attachment); - let id = nvme.id(); - assert!(!id.is_null(), "NVMe config Id should not be null"); - } - - // ─── File Serial Port ────────────────────────────────────────────── - - #[test] - fn file_serial_port_attachment_creates() { - use vm_rs::ffi::apple_vz::serial_port::{ - VZFileSerialPortAttachment, VZSerialPortAttachment, - }; - - let tmp = tempfile::tempdir().expect("tempdir"); - let log_path = tmp.path().join("serial.log"); - // Create the file first - std::fs::write(&log_path, b"").unwrap(); - - let attachment = match VZFileSerialPortAttachment::new(log_path.to_str().unwrap(), false) { - Ok(a) => a, - Err(_) => panic!("VZFileSerialPortAttachment::new should succeed for a valid path"), - }; - let id = attachment.id(); - assert!(!id.is_null(), "file serial port Id should not be null"); - } - - #[test] - fn file_serial_port_attachment_append_mode() { - use vm_rs::ffi::apple_vz::serial_port::VZFileSerialPortAttachment; - - let tmp = tempfile::tempdir().expect("tempdir"); - let log_path = tmp.path().join("serial-append.log"); - std::fs::write(&log_path, b"existing content\n").unwrap(); - - let _attachment = match VZFileSerialPortAttachment::new(log_path.to_str().unwrap(), true) { - Ok(a) => a, - Err(_) => panic!("VZFileSerialPortAttachment::new should succeed in append mode"), - }; - // No crash in append mode = success - } - - // ─── NSDictionary construction ───────────────────────────────────── - - #[test] - fn nsdictionary_from_pairs() { - use vm_rs::ffi::apple_vz::base::{NSDictionary, NSString}; - - let key1 = NSString::new("hello"); - let val1 = NSString::new("world"); - let key2 = NSString::new("foo"); - let val2 = NSString::new("bar"); - - let dict = NSDictionary::from_pairs(&[(*key1.0, *val1.0), (*key2.0, *val2.0)]); - - let keys = dict.all_keys::(); - assert_eq!(keys.count(), 2, "dictionary should have 2 keys"); - } - - // ─── VM Configuration Builder with new features ──────────────────── - - #[test] - fn vm_config_builder_with_platform_and_vsock() { + #[ignore = "requires virtualization entitlement (SIGSEGV without it)"] + fn vm_config_builder_basic() { use vm_rs::ffi::apple_vz::{ entropy_device::VZVirtioEntropyDeviceConfiguration, memory_device::VZVirtioTraditionalMemoryBalloonDeviceConfiguration, - platform::{VZGenericMachineIdentifier, VZGenericPlatformConfiguration}, - socket_device::VZVirtioSocketDeviceConfiguration, virtual_machine::VZVirtualMachineConfigurationBuilder, }; - // Build a full config with new features (doesn't need real paths for the builder itself) - let mut platform = VZGenericPlatformConfiguration::new(); - let machine_id = VZGenericMachineIdentifier::new(); - platform.set_machine_identifier(&machine_id); - - let vsock = VZVirtioSocketDeviceConfiguration::new(); let entropy = VZVirtioEntropyDeviceConfiguration::new(); let balloon = VZVirtioTraditionalMemoryBalloonDeviceConfiguration::new(); - // We can't fully build without valid paths for the boot loader, - // but we CAN verify the builder accepts the new types + // Build a config with the types that exist let _builder = VZVirtualMachineConfigurationBuilder::new() .cpu_count(2) .memory_size(512 * 1024 * 1024) - .platform(platform) - .socket_devices(vec![vsock]) .entropy_devices(vec![entropy]) .memory_balloon_devices(vec![balloon]); // Build would fail validation without a boot loader, but the builder diff --git a/tests/vm_lifecycle.rs b/tests/vm_lifecycle.rs index 84330cb..fcd8d16 100644 --- a/tests/vm_lifecycle.rs +++ b/tests/vm_lifecycle.rs @@ -68,6 +68,10 @@ fn make_config(name: &str, kernel: &Path, initramfs: &Path) -> VmConfig { serial_log: base.join("serial.log"), cmdline: Some("console=hvc0".to_string()), netns: None, + vsock: false, + machine_id: None, + efi_variable_store: None, + rosetta: false, } } diff --git a/tests/vm_manager.rs b/tests/vm_manager.rs index 6c5a54e..2ee2134 100644 --- a/tests/vm_manager.rs +++ b/tests/vm_manager.rs @@ -42,7 +42,10 @@ impl MockDriver { } fn set_fail_boot(&self, msg: &str) { - *self.fail_boot.lock().unwrap() = Some(msg.to_string()); + *self + .fail_boot + .lock() + .expect("fail_boot lock should not be poisoned") = Some(msg.to_string()); } } @@ -50,7 +53,12 @@ impl VmDriver for MockDriver { fn boot(&self, config: &VmConfig) -> Result { self.boot_count.fetch_add(1, Ordering::SeqCst); - if let Some(msg) = self.fail_boot.lock().unwrap().as_ref() { + if let Some(msg) = self + .fail_boot + .lock() + .expect("fail_boot lock should not be poisoned") + .as_ref() + { return Err(VmError::BootFailed { name: config.name.clone(), detail: msg.clone(), @@ -63,12 +71,15 @@ impl VmDriver for MockDriver { state: VmState::Running { ip: "10.0.0.99".into(), }, - pid: Some(99999), + process: None, serial_log: config.serial_log.clone(), machine_id: Some(vec![0xDE, 0xAD, 0xBE, 0xEF]), }; - self.vms.lock().unwrap().insert( + self.vms + .lock() + .expect("vms lock should not be poisoned") + .insert( config.name.clone(), MockVmState { state: VmState::Running { @@ -81,7 +92,7 @@ impl VmDriver for MockDriver { } fn stop(&self, handle: &VmHandle) -> Result<(), VmError> { - let mut vms = self.vms.lock().unwrap(); + let mut vms = self.vms.lock().expect("vms lock should not be poisoned"); match vms.get_mut(&handle.name) { Some(vm) => { vm.state = VmState::Stopped; @@ -94,7 +105,7 @@ impl VmDriver for MockDriver { } fn kill(&self, handle: &VmHandle) -> Result<(), VmError> { - let mut vms = self.vms.lock().unwrap(); + let mut vms = self.vms.lock().expect("vms lock should not be poisoned"); match vms.get_mut(&handle.name) { Some(vm) => { vm.state = VmState::Stopped; @@ -107,7 +118,7 @@ impl VmDriver for MockDriver { } fn state(&self, handle: &VmHandle) -> Result { - let vms = self.vms.lock().unwrap(); + let vms = self.vms.lock().expect("vms lock should not be poisoned"); match vms.get(&handle.name) { Some(vm) => Ok(vm.state.clone()), None => Ok(VmState::Stopped), @@ -115,7 +126,7 @@ impl VmDriver for MockDriver { } fn pause(&self, handle: &VmHandle) -> Result<(), VmError> { - let mut vms = self.vms.lock().unwrap(); + let mut vms = self.vms.lock().expect("vms lock should not be poisoned"); match vms.get_mut(&handle.name) { Some(vm) => { if !matches!(vm.state, VmState::Running { .. }) { @@ -131,7 +142,7 @@ impl VmDriver for MockDriver { } fn resume(&self, handle: &VmHandle) -> Result<(), VmError> { - let mut vms = self.vms.lock().unwrap(); + let mut vms = self.vms.lock().expect("vms lock should not be poisoned"); match vms.get_mut(&handle.name) { Some(vm) => { if vm.state != VmState::Paused { @@ -242,7 +253,9 @@ fn duplicate_boot_rejected() { let result = manager.start(&config); assert!(result.is_err(), "duplicate boot should fail"); - let err = result.unwrap_err().to_string(); + let err = result + .expect_err("duplicate boot should return an error") + .to_string(); assert!( err.contains("already exists"), "error should say 'already exists': {}", @@ -286,7 +299,10 @@ fn stop_nonexistent_vm() { let result = manager.stop("does-not-exist"); assert!(result.is_err()); - assert!(result.unwrap_err().to_string().contains("not found")); + assert!(result + .expect_err("stopping a nonexistent VM should fail") + .to_string() + .contains("not found")); } #[test] @@ -360,7 +376,7 @@ fn concurrent_boots_different_names() { let mut successes = 0; for h in handles { - if h.join().unwrap().is_ok() { + if h.join().expect("boot thread should not panic").is_ok() { successes += 1; } } @@ -502,7 +518,9 @@ fn boot_returns_machine_id() { "boot should return a machine_id" ); assert_eq!( - handle.machine_id.unwrap(), + handle + .machine_id + .expect("mock driver should populate machine_id"), vec![0xDE, 0xAD, 0xBE, 0xEF], "machine_id should match mock" ); @@ -537,7 +555,10 @@ fn config_machine_id_roundtrip() { let mut config = make_config("mock-mid"); let id = vec![1, 2, 3, 4, 5, 6, 7, 8]; config.machine_id = Some(id.clone()); - assert_eq!(config.machine_id.unwrap(), id); + assert_eq!( + config.machine_id.expect("machine_id should be present after assignment"), + id + ); } #[test] From 459bca5bdc7bd29966a856ac97e6402f63583351 Mon Sep 17 00:00:00 2001 From: Soheil Alizadeh Date: Sat, 14 Mar 2026 11:41:24 +0100 Subject: [PATCH 2/3] Harden VM lifecycle and validation paths --- src/config.rs | 36 ++++++-- src/driver/apple_vz.rs | 133 +++++++++++++++++++-------- src/driver/cloud_hv.rs | 120 ++++++++++++++---------- src/driver/mod.rs | 9 +- src/driver/whp.rs | 33 +++++++ src/ffi/apple_vz/base.rs | 12 ++- src/ffi/apple_vz/shared_directory.rs | 2 +- src/ffi/apple_vz/storage_device.rs | 10 +- src/ffi/apple_vz/virtual_machine.rs | 2 +- src/lib.rs | 3 +- src/network/bridge.rs | 34 +++++-- src/network/mod.rs | 8 +- src/network/port_forward.rs | 13 +-- src/network/switch.rs | 44 +++++---- src/oci/registry.rs | 82 ++++++++++++----- src/oci/store.rs | 70 ++++++++------ src/setup/image.rs | 61 +++++------- src/setup/seed.rs | 111 ++++++++++++++++------ src/vm.rs | 104 ++++++++++++++++----- tests/disk_clone.rs | 12 ++- tests/network_switch.rs | 27 +++++- tests/oci_pull.rs | 20 ++-- tests/seed_iso.rs | 22 ++++- tests/vm_manager.rs | 30 ++++-- 24 files changed, 681 insertions(+), 317 deletions(-) diff --git a/src/config.rs b/src/config.rs index b4051fe..c49b72d 100644 --- a/src/config.rs +++ b/src/config.rs @@ -129,24 +129,37 @@ impl VmConfig { return Err(VmError::InvalidConfig("VM name must not be empty".into())); } if self.name.len() > 128 { - return Err(VmError::InvalidConfig("VM name must be 128 characters or fewer".into())); + return Err(VmError::InvalidConfig( + "VM name must be 128 characters or fewer".into(), + )); } - if !self.name.chars().all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.') { + if !self + .name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.') + { return Err(VmError::InvalidConfig( - "VM name must contain only alphanumeric characters, hyphens, underscores, and dots".into(), + "VM name must contain only alphanumeric characters, hyphens, underscores, and dots" + .into(), )); } if self.name.starts_with('.') || self.name.starts_with('-') { - return Err(VmError::InvalidConfig("VM name must not start with '.' or '-'".into())); + return Err(VmError::InvalidConfig( + "VM name must not start with '.' or '-'".into(), + )); } if self.cpus == 0 { return Err(VmError::InvalidConfig("cpus must be at least 1".into())); } if self.memory_mb == 0 { - return Err(VmError::InvalidConfig("memory_mb must be at least 1".into())); + return Err(VmError::InvalidConfig( + "memory_mb must be at least 1".into(), + )); } if self.kernel.as_os_str().is_empty() { - return Err(VmError::InvalidConfig("kernel path must not be empty".into())); + return Err(VmError::InvalidConfig( + "kernel path must not be empty".into(), + )); } Ok(()) } @@ -312,8 +325,11 @@ mod tests { .validate() .expect_err("path traversal characters should fail validation") .to_string(); - assert!(err.contains("alphanumeric") || err.contains("characters"), - "expected name validation error: {}", err); + assert!( + err.contains("alphanumeric") || err.contains("characters"), + "expected name validation error: {}", + err + ); } #[test] @@ -330,6 +346,8 @@ mod tests { #[test] fn validate_accepts_valid_config() { let config = test_vm_config("my-vm.01"); - config.validate().expect("valid config should pass validation"); + config + .validate() + .expect("valid config should pass validation"); } } diff --git a/src/driver/apple_vz.rs b/src/driver/apple_vz.rs index 724ffed..812f43e 100644 --- a/src/driver/apple_vz.rs +++ b/src/driver/apple_vz.rs @@ -55,18 +55,20 @@ impl AppleVzDriver { fn vm_state(handle: &VmHandle, vm: &VZVirtualMachine) -> VmState { // SAFETY: VM references come from the driver's registry and stay alive // for the lifetime of the driver entry. - match unsafe { vm.state() } { - VZVirtualMachineState::VZVirtualMachineStateRunning => { - if let Some(ip) = super::check_ready_marker(&handle.serial_log) { - VmState::Running { ip } - } else { - VmState::Starting - } - } - VZVirtualMachineState::VZVirtualMachineStateStarting + let ready_ip = super::check_ready_marker(&handle.serial_log); + Self::map_native_state(unsafe { vm.state() }, ready_ip) + } + + fn map_native_state(state: VZVirtualMachineState, ready_ip: Option) -> VmState { + match state { + VZVirtualMachineState::VZVirtualMachineStateRunning => match ready_ip { + Some(ip) => VmState::Running { ip }, + None => VmState::Starting, + }, + VZVirtualMachineState::VZVirtualMachineStatePaused | VZVirtualMachineState::VZVirtualMachineStatePausing - | VZVirtualMachineState::VZVirtualMachineStateResuming - | VZVirtualMachineState::VZVirtualMachineStatePaused => VmState::Starting, + | VZVirtualMachineState::VZVirtualMachineStateResuming => VmState::Paused, + VZVirtualMachineState::VZVirtualMachineStateStarting => VmState::Starting, VZVirtualMachineState::VZVirtualMachineStateStopped => VmState::Stopped, VZVirtualMachineState::VZVirtualMachineStateError => VmState::Failed { reason: "Apple Virtualization.framework reported an internal error".into(), @@ -188,11 +190,9 @@ impl VmDriver for AppleVzDriver { .caching_mode(VZDiskImageCachingMode::Automatic) .sync_mode(VZDiskImageSynchronizationMode::Full) .build() - .map_err(|e| { - VmError::BootFailed { - name: name.clone(), - detail: format!("failed to attach root disk: code {}", e.code()), - } + .map_err(|e| VmError::BootFailed { + name: name.clone(), + detail: format!("failed to attach root disk: code {}", e.code()), })?; storage_devices.push(VZVirtioBlockDeviceConfiguration::new(root_attachment)); } @@ -202,11 +202,9 @@ impl VmDriver for AppleVzDriver { .path(seed_path) .read_only(true) .build() - .map_err(|e| { - VmError::BootFailed { - name: name.clone(), - detail: format!("failed to attach seed ISO: code {}", e.code()), - } + .map_err(|e| VmError::BootFailed { + name: name.clone(), + detail: format!("failed to attach seed ISO: code {}", e.code()), })?; storage_devices.push(VZVirtioBlockDeviceConfiguration::new(seed_attachment)); } @@ -256,8 +254,8 @@ impl VmDriver for AppleVzDriver { })?; // Create dispatch queue with unique label per VM - let label = std::ffi::CString::new(format!("rs.vm.{}", name)) - .map_err(|e| VmError::BootFailed { + let label = + std::ffi::CString::new(format!("rs.vm.{}", name)).map_err(|e| VmError::BootFailed { name: name.clone(), detail: format!("invalid VM name for queue label: {}", e), })?; @@ -295,11 +293,7 @@ impl VmDriver for AppleVzDriver { error.code(), error.localized_description().as_str() ); - tracing::error!( - "VM '{}' start FAILED {}", - name_err, - msg - ); + tracing::error!("VM '{}' start FAILED {}", name_err, msg); let _ = tx_clone.send(Err(msg)); } }); @@ -334,7 +328,8 @@ impl VmDriver for AppleVzDriver { .remove(name); return Err(VmError::BootFailed { name: name.clone(), - detail: "Apple VZ start completion handler did not fire within 10 seconds".into(), + detail: "Apple VZ start completion handler did not fire within 10 seconds" + .into(), }); } } @@ -364,12 +359,16 @@ impl VmDriver for AppleVzDriver { let mut vm_clone = vm.clone(); // SAFETY: vm_clone is a valid VZVirtualMachine reference from the registry. unsafe { - vm_clone.request_stop_with_error().map_err(|e| { - VmError::StopFailed { + vm_clone + .request_stop_with_error() + .map_err(|e| VmError::StopFailed { name: handle.name.clone(), - detail: format!("error code {}: {}", e.code(), e.localized_description().as_str()), - } - })?; + detail: format!( + "error code {}: {}", + e.code(), + e.localized_description().as_str() + ), + })?; } let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); @@ -378,7 +377,9 @@ impl VmDriver for AppleVzDriver { VmState::Stopped => { self.vms .lock() - .map_err(|e| VmError::Hypervisor(format!("VM registry lock poisoned: {}", e)))? + .map_err(|e| { + VmError::Hypervisor(format!("VM registry lock poisoned: {}", e)) + })? .remove(&handle.name); tracing::info!(vm = %handle.name, "Apple VZ VM stopped"); return Ok(()); @@ -386,7 +387,9 @@ impl VmDriver for AppleVzDriver { VmState::Failed { reason } => { self.vms .lock() - .map_err(|e| VmError::Hypervisor(format!("VM registry lock poisoned: {}", e)))? + .map_err(|e| { + VmError::Hypervisor(format!("VM registry lock poisoned: {}", e)) + })? .remove(&handle.name); return Err(VmError::StopFailed { name: handle.name.clone(), @@ -441,3 +444,61 @@ fn resolve_path(path: &Path, label: &str) -> Result { .ok_or_else(|| VmError::InvalidConfig(format!("non-UTF8 {} path", label))) .map(|s| s.to_string()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn paused_native_state_maps_to_paused() { + assert_eq!( + AppleVzDriver::map_native_state( + VZVirtualMachineState::VZVirtualMachineStatePaused, + None, + ), + VmState::Paused + ); + } + + #[test] + fn pausing_and_resuming_native_states_map_to_paused() { + assert_eq!( + AppleVzDriver::map_native_state( + VZVirtualMachineState::VZVirtualMachineStatePausing, + None, + ), + VmState::Paused + ); + assert_eq!( + AppleVzDriver::map_native_state( + VZVirtualMachineState::VZVirtualMachineStateResuming, + None, + ), + VmState::Paused + ); + } + + #[test] + fn running_without_ready_marker_stays_starting() { + assert_eq!( + AppleVzDriver::map_native_state( + VZVirtualMachineState::VZVirtualMachineStateRunning, + None, + ), + VmState::Starting + ); + } + + #[test] + fn running_with_ready_marker_maps_to_running() { + assert_eq!( + AppleVzDriver::map_native_state( + VZVirtualMachineState::VZVirtualMachineStateRunning, + Some("10.0.0.2".into()), + ), + VmState::Running { + ip: "10.0.0.2".into(), + } + ); + } +} diff --git a/src/driver/cloud_hv.rs b/src/driver/cloud_hv.rs index b15a849..93116ec 100644 --- a/src/driver/cloud_hv.rs +++ b/src/driver/cloud_hv.rs @@ -24,8 +24,8 @@ struct VmProcess { identity: VmmProcess, /// Child handle retained so we can reap exit status and avoid zombie confusion. child: Child, - /// TAP device name (for cleanup on stop). - tap_device: Option, + /// TAP device names (for cleanup on stop). + tap_devices: Vec, /// virtiofsd sidecar PIDs (for VirtioFS shared dirs, cleaned up on stop). virtiofsd_pids: Vec, } @@ -90,28 +90,33 @@ impl VmDriver for CloudHvDriver { // Memory — shared=on required when using VirtioFS if config.shared_dirs.is_empty() { - cmd.arg("--memory").arg(format!("size={}M", config.memory_mb)); + cmd.arg("--memory") + .arg(format!("size={}M", config.memory_mb)); } else { - cmd.arg("--memory").arg(format!("size={}M,shared=on", config.memory_mb)); + cmd.arg("--memory") + .arg(format!("size={}M,shared=on", config.memory_mb)); } // Disks: only attach if provided (initramfs boot needs no disks) if let Some(ref root_disk) = config.root_disk { - cmd.arg("--disk").arg(format!("path={}", root_disk.display())); + cmd.arg("--disk") + .arg(format!("path={}", root_disk.display())); } if let Some(ref seed_iso) = config.seed_iso { - cmd.arg("--disk").arg(format!("path={},readonly=on", seed_iso.display())); + cmd.arg("--disk") + .arg(format!("path={},readonly=on", seed_iso.display())); } if let Some(ref data_disk) = config.data_disk { - cmd.arg("--disk").arg(format!("path={}", data_disk.display())); + cmd.arg("--disk") + .arg(format!("path={}", data_disk.display())); } // Network - let mut tap_name = None; + let mut tap_devices = Vec::new(); for net in &config.networks { match net { NetworkAttachment::Tap { name: tap, mac } => { - tap_name = Some(tap.clone()); + tap_devices.push(tap.clone()); let mac_str = mac.clone().unwrap_or_else(|| generate_mac(name)); cmd.arg("--net").arg(format!("tap={},mac={}", tap, mac_str)); } @@ -124,7 +129,8 @@ impl VmDriver for CloudHvDriver { } // Serial console → file - cmd.arg("--serial").arg(format!("file={}", config.serial_log.display())); + cmd.arg("--serial") + .arg(format!("file={}", config.serial_log.display())); cmd.arg("--console").arg("off"); // VirtioFS shared directories via virtiofsd sidecar processes. @@ -215,32 +221,32 @@ impl VmDriver for CloudHvDriver { // Brief pause then check if process exited immediately (bad binary, permissions, etc.) std::thread::sleep(std::time::Duration::from_millis(100)); - if let Some(status) = process - .try_wait() - .map_err(|e| VmError::Io(e))? - { + if let Some(status) = process.try_wait().map_err(|e| VmError::Io(e))? { // Clean up virtiofsd processes since VM failed cleanup_virtiofsd(&virtiofsd_pids); return Err(VmError::BootFailed { name: name.clone(), detail: format!( "cloud-hypervisor process (PID {}) exited immediately with {}. Check {}", - pid, status, vmm_log_path.display() + pid, + status, + vmm_log_path.display() ), }); } // Track { - let mut vms = self.vms.lock().map_err(|e| { - VmError::Hypervisor(format!("lock poisoned: {}", e)) - })?; + let mut vms = self + .vms + .lock() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; vms.insert( name.clone(), VmProcess { child: process, identity: identity.clone(), - tap_device: tap_name, + tap_devices, virtiofsd_pids, }, ); @@ -258,9 +264,10 @@ impl VmDriver for CloudHvDriver { fn stop(&self, handle: &VmHandle) -> Result<(), VmError> { tracing::info!(vm = %handle.name, "requesting graceful stop via Cloud Hypervisor"); - let mut vms = self.vms.lock().map_err(|e| { - VmError::Hypervisor(format!("lock poisoned: {}", e)) - })?; + let mut vms = self + .vms + .lock() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; let process = if let Some(process) = vms.remove(&handle.name) { process } else if let Some(ref process) = handle.process { @@ -301,25 +308,33 @@ impl VmDriver for CloudHvDriver { wait_for_exit(process.child, std::time::Duration::from_secs(10)); } - cleanup_tap(&process.tap_device); + cleanup_taps(&process.tap_devices); cleanup_virtiofsd(&process.virtiofsd_pids); Ok(()) } fn kill(&self, handle: &VmHandle) -> Result<(), VmError> { tracing::warn!(vm = %handle.name, "force-killing Cloud Hypervisor VM"); - let mut vms = self.vms.lock().map_err(|e| { - VmError::Hypervisor(format!("lock poisoned: {}", e)) - })?; + let mut vms = self + .vms + .lock() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; - let (identity, mut child, virtiofsd_pids) = if let Some(process) = vms.remove(&handle.name) { - cleanup_tap(&process.tap_device); - (process.identity, Some(process.child), process.virtiofsd_pids) + let (identity, mut child, virtiofsd_pids) = if let Some(process) = vms.remove(&handle.name) + { + cleanup_taps(&process.tap_devices); + ( + process.identity, + Some(process.child), + process.virtiofsd_pids, + ) } else if let Some(ref process) = handle.process { validate_cloud_hypervisor_process(process, &handle.name)?; (process.clone(), None, Vec::new()) } else { - return Err(VmError::NotFound { name: handle.name.clone() }); + return Err(VmError::NotFound { + name: handle.name.clone(), + }); }; let kill_result = if let Some(child) = child.as_mut() { @@ -354,9 +369,10 @@ impl VmDriver for CloudHvDriver { } fn state(&self, handle: &VmHandle) -> Result { - let mut vms = self.vms.lock().map_err(|e| { - VmError::Hypervisor(format!("lock poisoned: {}", e)) - })?; + let mut vms = self + .vms + .lock() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; let process = match vms.get_mut(&handle.name) { Some(p) => p, @@ -378,12 +394,7 @@ impl VmDriver for CloudHvDriver { } }; - if process - .child - .try_wait() - .map_err(VmError::Io)? - .is_some() - { + if process.child.try_wait().map_err(VmError::Io)?.is_some() { vms.remove(&handle.name); return Ok(VmState::Stopped); } @@ -434,7 +445,8 @@ fn find_ch_binary() -> Result { Err(VmError::InvalidConfig( "cloud-hypervisor binary not found on PATH or in /usr/bin, /usr/local/bin. \ - Install from https://github.com/cloud-hypervisor/cloud-hypervisor/releases".into(), + Install from https://github.com/cloud-hypervisor/cloud-hypervisor/releases" + .into(), )) } @@ -443,10 +455,7 @@ fn find_ch_binary() -> Result { fn generate_mac(name: &str) -> String { use sha2::{Digest, Sha256}; let hash = Sha256::digest(name.as_bytes()); - format!( - "52:54:00:{:02x}:{:02x}:{:02x}", - hash[0], hash[1], hash[2] - ) + format!("52:54:00:{:02x}:{:02x}:{:02x}", hash[0], hash[1], hash[2]) } /// Find the virtiofsd binary on PATH or well-known locations. @@ -464,7 +473,11 @@ fn find_virtiofsd() -> Result { { Ok(status) if status.success() => return Ok(PathBuf::from(name)), Ok(status) => { - tracing::warn!("virtiofsd candidate '{}' found but exited with {}", name, status); + tracing::warn!( + "virtiofsd candidate '{}' found but exited with {}", + name, + status + ); } Err(e) if e.kind() == std::io::ErrorKind::NotFound => { // Expected: candidate not at this location, try next @@ -557,7 +570,11 @@ fn wait_for_pid_exit( let errno = std::io::Error::last_os_error(); return Err(VmError::StopFailed { name: vm_name.to_string(), - detail: format!("failed to SIGKILL restored VM PID {}: {}", process.pid(), errno), + detail: format!( + "failed to SIGKILL restored VM PID {}: {}", + process.pid(), + errno + ), }); } // Brief wait for SIGKILL to take effect @@ -572,7 +589,10 @@ fn wait_for_pid_exit( tracing::error!(pid = process.pid(), "process still alive after SIGKILL"); Err(VmError::StopFailed { name: vm_name.to_string(), - detail: format!("restored VM PID {} remained alive after SIGKILL", process.pid()), + detail: format!( + "restored VM PID {} remained alive after SIGKILL", + process.pid() + ), }) } @@ -632,9 +652,9 @@ fn read_proc_start_time(pid: u32) -> Option { fields.get(19)?.parse().ok() } -/// Clean up a TAP device if one was created. -fn cleanup_tap(tap_device: &Option) { - if let Some(ref tap) = tap_device { +/// Clean up all TAP devices created for a VM. +fn cleanup_taps(tap_devices: &[String]) { + for tap in tap_devices { let status = Command::new("ip") .args(["tuntap", "del", "dev", tap, "mode", "tap"]) .stdout(Stdio::null()) diff --git a/src/driver/mod.rs b/src/driver/mod.rs index 5a5404d..63b63de 100644 --- a/src/driver/mod.rs +++ b/src/driver/mod.rs @@ -10,6 +10,9 @@ pub mod apple_vz; #[cfg(target_os = "linux")] pub mod cloud_hv; +#[cfg(target_os = "windows")] +pub mod whp; + use crate::config::{VmConfig, VmHandle, VmState}; /// Platform-agnostic VM lifecycle. @@ -119,5 +122,9 @@ pub(crate) fn check_ready_marker(log_path: &std::path::Path) -> Option { let pos = content.find(crate::config::READY_MARKER)?; let after = &content[pos + crate::config::READY_MARKER.len()..]; let ip = after.split_whitespace().next()?.trim().to_string(); - if ip.is_empty() { None } else { Some(ip) } + if ip.is_empty() { + None + } else { + Some(ip) + } } diff --git a/src/driver/whp.rs b/src/driver/whp.rs index bff9619..4a9f65f 100644 --- a/src/driver/whp.rs +++ b/src/driver/whp.rs @@ -113,6 +113,8 @@ struct WhpVm { _memory: GuestMemory, /// Shared VM state (read by driver, written by vCPU thread). state: Arc>, + /// State to restore after a successful resume. + resume_state: Option, /// Stop flag — signals the vCPU thread to exit. stop_flag: Arc, /// vCPU execution thread handle. @@ -359,6 +361,7 @@ impl VmDriver for WhpDriver { partition, _memory: memory, state: Arc::clone(&state), + resume_state: None, stop_flag: Arc::clone(&stop_flag), vcpu_thread: Some(vcpu_thread), serial_log: serial_log.clone(), @@ -455,6 +458,15 @@ impl VmDriver for WhpDriver { name: handle.name.clone(), })?; + let current_state = vm + .state + .read() + .map_err(|e| VmError::Hypervisor(format!("state lock poisoned: {e}")))? + .clone(); + if !matches!(current_state, VmState::Running { .. }) { + return Err(VmError::Hypervisor("can only pause a running VM".into())); + } + // Signal vCPU to stop running (but don't destroy partition) vm.stop_flag.store(true, Ordering::Release); @@ -465,6 +477,7 @@ impl VmDriver for WhpDriver { let _ = thread.join(); } + vm.resume_state = Some(current_state); if let Ok(mut state) = vm.state.write() { *state = VmState::Paused; } @@ -483,8 +496,24 @@ impl VmDriver for WhpDriver { name: handle.name.clone(), })?; + let current_state = vm + .state + .read() + .map_err(|e| VmError::Hypervisor(format!("state lock poisoned: {e}")))? + .clone(); + if current_state != VmState::Paused { + return Err(VmError::Hypervisor("can only resume a paused VM".into())); + } + if vm.vcpu_thread.is_some() { + return Err(VmError::Hypervisor(format!( + "VM '{}' already has an active vCPU thread", + handle.name + ))); + } + // Reset stop flag and spawn new vCPU thread vm.stop_flag.store(false, Ordering::Release); + let resumed_state = vm.resume_state.clone().unwrap_or(VmState::Starting); let sendable = SendablePartition(vm.partition); let state_clone = Arc::clone(&vm.state); @@ -500,6 +529,10 @@ impl VmDriver for WhpDriver { .map_err(|e| VmError::Hypervisor(format!("failed to spawn vCPU thread: {e}")))?; vm.vcpu_thread = Some(thread); + vm.resume_state = None; + if let Ok(mut state) = vm.state.write() { + *state = resumed_state; + } tracing::info!(vm = %handle.name, "VM resumed"); Ok(()) diff --git a/src/ffi/apple_vz/base.rs b/src/ffi/apple_vz/base.rs index 596c615..62d4e0b 100644 --- a/src/ffi/apple_vz/base.rs +++ b/src/ffi/apple_vz/base.rs @@ -17,7 +17,7 @@ extern "C" {} extern "C" { pub fn dispatch_queue_create(label: *const libc::c_char, attr: Id) -> Id; pub fn dispatch_async(queue: Id, block: &Block<(), ()>); -// pub fn dispatch_async(queue: Id, block: *mut c_void); + // pub fn dispatch_async(queue: Id, block: *mut c_void); } pub type Id = *mut Object; @@ -164,9 +164,7 @@ impl NSFileHandle { /// lifetime expected by the created Objective-C file handle. pub unsafe fn file_handle_with_fd(fd: i32) -> NSFileHandle { let alloc: Id = msg_send![class!(NSFileHandle), alloc]; - let p = StrongPtr::new( - msg_send![alloc, initWithFileDescriptor: fd closeOnDealloc: YES], - ); + let p = StrongPtr::new(msg_send![alloc, initWithFileDescriptor: fd closeOnDealloc: YES]); NSFileHandle(p) } } @@ -255,7 +253,11 @@ impl NSError { // Helper: safely print an NSString that may be backed by nil fn safe_str(s: &NSString) -> Cow<'_, str> { - if *s.0 == NIL { Cow::Borrowed("(nil)") } else { s.as_str() } + if *s.0 == NIL { + Cow::Borrowed("(nil)") + } else { + s.as_str() + } } let desc = self.localized_description(); diff --git a/src/ffi/apple_vz/shared_directory.rs b/src/ffi/apple_vz/shared_directory.rs index 77bcdb0..59554c3 100644 --- a/src/ffi/apple_vz/shared_directory.rs +++ b/src/ffi/apple_vz/shared_directory.rs @@ -3,8 +3,8 @@ use super::base::{Id, NSString}; use objc::rc::StrongPtr; -use objc::{class, msg_send, sel, sel_impl}; use objc::runtime::YES; +use objc::{class, msg_send, sel, sel_impl}; /// Trait for directory sharing configurations. pub trait VZDirectorySharingDeviceConfiguration { diff --git a/src/ffi/apple_vz/storage_device.rs b/src/ffi/apple_vz/storage_device.rs index c9db0e3..4497974 100644 --- a/src/ffi/apple_vz/storage_device.rs +++ b/src/ffi/apple_vz/storage_device.rs @@ -1,6 +1,6 @@ //! storage device module -use super::base::{Id, NIL, NSError, NSURL}; +use super::base::{Id, NSError, NIL, NSURL}; use objc::runtime::BOOL; use objc::{class, msg_send, sel, sel_impl}; @@ -102,8 +102,12 @@ impl VZDiskImageStorageDeviceAttachmentBuilder { pub fn build(self) -> Result { let read_only = if self.read_only { YES } else { NO }; if self.caching_mode.is_some() || self.sync_mode.is_some() { - let caching = self.caching_mode.unwrap_or(VZDiskImageCachingMode::Automatic) as isize; - let sync = self.sync_mode.unwrap_or(VZDiskImageSynchronizationMode::Full) as isize; + let caching = self + .caching_mode + .unwrap_or(VZDiskImageCachingMode::Automatic) as isize; + let sync = self + .sync_mode + .unwrap_or(VZDiskImageSynchronizationMode::Full) as isize; unsafe { VZDiskImageStorageDeviceAttachment::new_with_modes( self.path.as_str(), diff --git a/src/ffi/apple_vz/virtual_machine.rs b/src/ffi/apple_vz/virtual_machine.rs index b05fe77..549ceb9 100644 --- a/src/ffi/apple_vz/virtual_machine.rs +++ b/src/ffi/apple_vz/virtual_machine.rs @@ -1,7 +1,7 @@ //! virtual machine module use super::{ - base::{Id, NIL, NSArray, NSError}, + base::{Id, NSArray, NSError, NIL}, boot_loader::VZBootLoader, entropy_device::VZEntropyDeviceConfiguration, memory_device::VZMemoryBalloonDeviceConfiguration, diff --git a/src/lib.rs b/src/lib.rs index bf370ea..31ad2b3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -28,8 +28,7 @@ pub mod vm; #[cfg(unix)] pub use config::VmSocketEndpoint; pub use config::{ - NetworkAttachment, SharedDir, VmConfig, VmHandle, VmState, VmmProcess, - READY_MARKER, + NetworkAttachment, SharedDir, VmConfig, VmHandle, VmState, VmmProcess, READY_MARKER, }; pub use driver::{VmDriver, VmError}; pub use vm::VmManager; diff --git a/src/network/bridge.rs b/src/network/bridge.rs index 89c252a..095415c 100644 --- a/src/network/bridge.rs +++ b/src/network/bridge.rs @@ -16,7 +16,10 @@ fn validate_interface_name(name: &str) -> Result<(), VmError> { name.len() ))); } - if !name.chars().all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') { + if !name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') + { return Err(VmError::InvalidConfig(format!( "interface name '{}' contains invalid characters (only alphanumeric, hyphen, underscore allowed)", name @@ -81,6 +84,7 @@ pub fn create_tap(name: &str) -> Result<(), VmError> { /// Add a TAP device to a bridge. pub fn add_to_bridge(tap: &str, bridge: &str) -> Result<(), VmError> { validate_interface_name(tap)?; + validate_interface_name(bridge)?; run_ip(&["link", "set", tap, "master", bridge])?; tracing::debug!(tap = %tap, bridge = %bridge, "TAP added to bridge"); Ok(()) @@ -132,9 +136,17 @@ fn setup_nat(bridge: &str, subnet: &str) -> Result<(), VmError> { // Check if rule already exists let check = Command::new("iptables") .args([ - "-t", "nat", "-C", "POSTROUTING", - "-s", subnet, "!", "-o", bridge, - "-j", "MASQUERADE", + "-t", + "nat", + "-C", + "POSTROUTING", + "-s", + subnet, + "!", + "-o", + bridge, + "-j", + "MASQUERADE", ]) .stdout(Stdio::null()) .stderr(Stdio::null()) @@ -149,9 +161,17 @@ fn setup_nat(bridge: &str, subnet: &str) -> Result<(), VmError> { // Add the rule let status = Command::new("iptables") .args([ - "-t", "nat", "-A", "POSTROUTING", - "-s", subnet, "!", "-o", bridge, - "-j", "MASQUERADE", + "-t", + "nat", + "-A", + "POSTROUTING", + "-s", + subnet, + "!", + "-o", + bridge, + "-j", + "MASQUERADE", ]) .stdout(Stdio::null()) .stderr(Stdio::null()) diff --git a/src/network/mod.rs b/src/network/mod.rs index 6f78828..07c11ac 100644 --- a/src/network/mod.rs +++ b/src/network/mod.rs @@ -4,12 +4,12 @@ //! - **macOS**: `NetworkSwitch` — userspace L2 learning bridge via socketpairs //! - **Linux**: `bridge` module — Linux kernel bridge + TAP devices + iptables NAT -#[cfg(unix)] -pub mod switch; -pub mod port_forward; #[cfg(target_os = "linux")] pub mod bridge; +pub mod port_forward; +#[cfg(unix)] +pub mod switch; +pub use port_forward::PortForwarder; #[cfg(unix)] pub use switch::NetworkSwitch; -pub use port_forward::PortForwarder; diff --git a/src/network/port_forward.rs b/src/network/port_forward.rs index 553f6e5..c6d4733 100644 --- a/src/network/port_forward.rs +++ b/src/network/port_forward.rs @@ -45,12 +45,13 @@ impl PortForwarder { .parse() .map_err(|e| PortForwardError::InvalidTarget(format!("{}", e)))?; - let listener = TcpListener::bind(bind_addr) - .await - .map_err(|e| PortForwardError::BindFailed { - address: bind_addr, - detail: format!("{}", e), - })?; + let listener = + TcpListener::bind(bind_addr) + .await + .map_err(|e| PortForwardError::BindFailed { + address: bind_addr, + detail: format!("{}", e), + })?; tracing::info!(bind = %bind_addr, target = %target, "port forwarder started"); diff --git a/src/network/switch.rs b/src/network/switch.rs index 5c260c4..550e747 100644 --- a/src/network/switch.rs +++ b/src/network/switch.rs @@ -96,9 +96,7 @@ impl NetworkSwitch { pub fn add_port(&self, network_id: &str, _label: &str) -> io::Result { let (switch_fd, vm_fd) = create_socketpair()?; - let port = SwitchPort { - fd: switch_fd, - }; + let port = SwitchPort { fd: switch_fd }; let mut networks = self .networks @@ -202,7 +200,12 @@ fn create_socketpair() -> io::Result<(OwnedFd, OwnedFd)> { if flags == -1 { return Err(io::Error::last_os_error()); } - if libc::fcntl(switch_fd.as_raw_fd(), libc::F_SETFL, flags | libc::O_NONBLOCK) == -1 { + if libc::fcntl( + switch_fd.as_raw_fd(), + libc::F_SETFL, + flags | libc::O_NONBLOCK, + ) == -1 + { return Err(io::Error::last_os_error()); } } @@ -266,8 +269,7 @@ fn forwarding_loop( } // SAFETY: poll(2) on fds we own. pollfds array is valid and properly sized. - let ready = - unsafe { libc::poll(pollfds.as_mut_ptr(), pollfds.len() as libc::nfds_t, 50) }; + let ready = unsafe { libc::poll(pollfds.as_mut_ptr(), pollfds.len() as libc::nfds_t, 50) }; if ready <= 0 { continue; @@ -282,14 +284,8 @@ fn forwarding_loop( // Read one Ethernet frame // SAFETY: Reading from our own fd into a stack buffer of MAX_FRAME_SIZE. - let n = unsafe { - libc::recv( - pfd.fd, - buf.as_mut_ptr() as *mut libc::c_void, - buf.len(), - 0, - ) - }; + let n = + unsafe { libc::recv(pfd.fd, buf.as_mut_ptr() as *mut libc::c_void, buf.len(), 0) }; if n < MIN_FRAME_SIZE as isize { continue; @@ -387,8 +383,7 @@ mod tests { #[test] fn mac_from_bytes_valid() { - let mac = MacAddress::from_bytes(&[0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff]) - .expect("valid MAC"); + let mac = MacAddress::from_bytes(&[0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff]).expect("valid MAC"); assert_eq!(mac.0, [0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff]); } @@ -469,7 +464,9 @@ mod tests { #[test] fn switch_add_ports_different_networks() { let switch = NetworkSwitch::new(); - let fd1 = switch.add_port("frontend", "web").expect("add frontend port"); + let fd1 = switch + .add_port("frontend", "web") + .expect("add frontend port"); let fd2 = switch.add_port("backend", "db").expect("add backend port"); assert_ne!(fd1.as_raw_fd(), fd2.as_raw_fd()); } @@ -478,7 +475,9 @@ mod tests { fn switch_frame_delivery_same_network() { let switch = NetworkSwitch::new(); let fd1 = switch.add_port("net0", "sender").expect("add sender port"); - let fd2 = switch.add_port("net0", "receiver").expect("add receiver port"); + let fd2 = switch + .add_port("net0", "receiver") + .expect("add receiver port"); switch.start().expect("start switch"); // Build a minimal Ethernet frame: dst(6) + src(6) + ethertype(2) = 14 bytes @@ -516,7 +515,10 @@ mod tests { libc::MSG_DONTWAIT, ) }; - assert_eq!(recvd, 14, "broadcast frame should be forwarded to the other port"); + assert_eq!( + recvd, 14, + "broadcast frame should be forwarded to the other port" + ); assert_eq!(&buf[..14], &frame[..14]); switch.stop(); @@ -526,7 +528,9 @@ mod tests { fn switch_no_cross_network_forwarding() { let switch = NetworkSwitch::new(); let fd1 = switch.add_port("net-a", "sender").expect("add sender port"); - let fd2 = switch.add_port("net-b", "isolated").expect("add isolated port"); + let fd2 = switch + .add_port("net-b", "isolated") + .expect("add isolated port"); switch.start().expect("start switch"); // Broadcast frame from net-a diff --git a/src/oci/registry.rs b/src/oci/registry.rs index 6e69234..9cde5e3 100644 --- a/src/oci/registry.rs +++ b/src/oci/registry.rs @@ -118,7 +118,10 @@ pub async fn pull(image: &str, store: &ImageStore) -> Result Result ImageRef { let first_part = image_no_digest.split('/').next().unwrap_or(image_no_digest); // A registry is present if the first path segment contains a dot OR a colon (port) - let has_registry = image_no_digest.contains('/') - && (first_part.contains('.') || first_part.contains(':')); + let has_registry = + image_no_digest.contains('/') && (first_part.contains('.') || first_part.contains(':')); let (registry, rest) = if has_registry { let slash = image_no_digest.find('/').expect("checked above"); @@ -193,13 +202,19 @@ async fn authenticate( "https://auth.docker.io/token?service=registry.docker.io&scope=repository:{}:pull", image_ref.repository ); - let resp = client.get(&url).send().await + let resp = client + .get(&url) + .send() + .await .map_err(|e| OciError::Http(format!("auth request failed: {}", e)))? .error_for_status() .map_err(|e| OciError::Auth(format!("auth failed: {}", e)))?; - let token_resp: TokenResponse = resp.json().await + let token_resp: TokenResponse = resp + .json() + .await .map_err(|e| OciError::Auth(format!("invalid auth response: {}", e)))?; - let token = token_resp.into_token() + let token = token_resp + .into_token() .ok_or_else(|| OciError::Auth("no token in auth response".into()))?; tracing::debug!(registry = %image_ref.registry, repository = %image_ref.repository, "using bearer token auth"); Ok(RegistryAuth::Bearer(token)) @@ -208,7 +223,10 @@ async fn authenticate( if let Some(basic_auth) = read_docker_config_auth(&image_ref.registry) { tracing::debug!(registry = %image_ref.registry, "found Docker config credentials"); let url = format!("https://{}/v2/", image_ref.registry); - let resp = client.get(&url).send().await + let resp = client + .get(&url) + .send() + .await .map_err(|e| OciError::Http(format!("registry probe failed: {}", e)))?; if resp.status() == reqwest::StatusCode::UNAUTHORIZED { if let Some(header_value) = resp.headers().get("www-authenticate") { @@ -222,11 +240,16 @@ async fn authenticate( .header("Authorization", format!("Basic {}", basic_auth)) .send() .await - .map_err(|e| OciError::Auth(format!("token exchange failed: {}", e)))? + .map_err(|e| { + OciError::Auth(format!("token exchange failed: {}", e)) + })? .error_for_status() - .map_err(|e| OciError::Auth(format!("token exchange failed: {}", e)))?; - let token_resp: TokenResponse = resp.json().await - .map_err(|e| OciError::Auth(format!("invalid token response: {}", e)))?; + .map_err(|e| { + OciError::Auth(format!("token exchange failed: {}", e)) + })?; + let token_resp: TokenResponse = resp.json().await.map_err(|e| { + OciError::Auth(format!("invalid token response: {}", e)) + })?; if let Some(token) = token_resp.into_token() { tracing::debug!(registry = %image_ref.registry, "registry exchanged Docker config credentials for bearer token"); return Ok(RegistryAuth::Bearer(token)); @@ -269,12 +292,17 @@ async fn fetch_manifest( req = req.header("Authorization", header); } - let resp = req.send().await + let resp = req + .send() + .await .map_err(|e| OciError::Http(format!("manifest fetch failed: {}", e)))? .error_for_status() .map_err(|e| OciError::Http(format!("manifest fetch failed: {}", e)))?; - Ok(resp.bytes().await - .map_err(|e| OciError::Http(format!("failed to read manifest: {}", e)))?.to_vec()) + Ok(resp + .bytes() + .await + .map_err(|e| OciError::Http(format!("failed to read manifest: {}", e)))? + .to_vec()) } async fn fetch_blob( @@ -293,19 +321,28 @@ async fn fetch_blob( req = req.header("Authorization", header); } - let resp = req.send().await + let resp = req + .send() + .await .map_err(|e| OciError::Http(format!("blob fetch failed: {}", e)))? .error_for_status() .map_err(|e| OciError::Http(format!("blob fetch failed: {}", e)))?; - Ok(resp.bytes().await - .map_err(|e| OciError::Http(format!("failed to read blob: {}", e)))?.to_vec()) + Ok(resp + .bytes() + .await + .map_err(|e| OciError::Http(format!("failed to read blob: {}", e)))? + .to_vec()) } fn resolve_platform_manifest(manifest_list_bytes: &[u8]) -> Result { let list: ManifestList = serde_json::from_slice(manifest_list_bytes) .map_err(|e| OciError::ManifestParse(format!("invalid manifest list JSON: {}", e)))?; - let target_arch = if cfg!(target_arch = "aarch64") { "arm64" } else { "amd64" }; + let target_arch = if cfg!(target_arch = "aarch64") { + "arm64" + } else { + "amd64" + }; // Exact match: linux + target architecture for entry in &list.manifests { @@ -358,13 +395,16 @@ fn read_docker_config_auth(registry: &str) -> Option { }; let auths = config.auths?; - let auth_entry = auths.get(registry) + let auth_entry = auths + .get(registry) .or_else(|| auths.get(&format!("https://{}", registry))) .or_else(|| auths.get(&format!("https://{}/v2/", registry))) .or_else(|| auths.get(&format!("https://{}/v1/", registry))); let auth_str = auth_entry?.auth.as_deref()?; - if auth_str.is_empty() { return None; } + if auth_str.is_empty() { + return None; + } Some(auth_str.to_string()) } diff --git a/src/oci/store.rs b/src/oci/store.rs index 43152ed..13b3fb0 100644 --- a/src/oci/store.rs +++ b/src/oci/store.rs @@ -125,7 +125,8 @@ impl ImageStore { /// Read a blob's bytes. pub fn get_blob(&self, digest: &str) -> Result, OciError> { let path = self.blob_path(digest); - std::fs::read(&path).map_err(|e| OciError::Blob(format!("failed to read blob {}: {}", digest, e))) + std::fs::read(&path) + .map_err(|e| OciError::Blob(format!("failed to read blob {}: {}", digest, e))) } /// Save a manifest for an image reference. @@ -200,7 +201,9 @@ impl ImageStore { let raw: RawManifest = serde_json::from_slice(data) .map_err(|e| OciError::ManifestParse(format!("invalid JSON: {}", e)))?; - let media_type = raw.media_type.as_deref() + let media_type = raw + .media_type + .as_deref() .or_else(|| raw.schema_version.map(|_| "")) .unwrap_or(""); @@ -208,11 +211,13 @@ impl ImageStore { return Err(OciError::ManifestList); } - let config_digest = raw.config + let config_digest = raw + .config .and_then(|c| c.digest) .ok_or_else(|| OciError::ManifestParse("missing config digest".into()))?; - let layers = raw.layers + let layers = raw + .layers .ok_or_else(|| OciError::ManifestParse("missing layers".into()))?; let layer_digests: Vec = layers .into_iter() @@ -249,9 +254,13 @@ impl ImageStore { let port = key .split('/') .next() - .ok_or_else(|| OciError::ManifestParse(format!("invalid exposed port '{}'", key)))? + .ok_or_else(|| { + OciError::ManifestParse(format!("invalid exposed port '{}'", key)) + })? .parse::() - .map_err(|e| OciError::ManifestParse(format!("invalid exposed port '{}': {}", key, e)))?; + .map_err(|e| { + OciError::ManifestParse(format!("invalid exposed port '{}': {}", key, e)) + })?; ports.push(port); } ports @@ -270,11 +279,7 @@ impl ImageStore { } /// Extract all layers of an image into a target directory (for rootfs preparation). - pub fn extract_layers( - &self, - manifest: &ImageManifest, - target: &Path, - ) -> Result<(), OciError> { + pub fn extract_layers(&self, manifest: &ImageManifest, target: &Path) -> Result<(), OciError> { std::fs::create_dir_all(target)?; for (i, digest) in manifest.layer_digests.iter().enumerate() { @@ -322,11 +327,20 @@ impl ImageStore { if let Some(parent) = path.parent() { let full_parent = target.join(parent); if full_parent.exists() { - let entries = std::fs::read_dir(&full_parent) - .map_err(|e| OciError::Blob(format!("opaque whiteout read_dir failed for {}: {}", full_parent.display(), e)))?; + let entries = std::fs::read_dir(&full_parent).map_err(|e| { + OciError::Blob(format!( + "opaque whiteout read_dir failed for {}: {}", + full_parent.display(), + e + )) + })?; for child in entries.flatten() { let child_path = child.path(); - let remove_result = if child.file_type().map(|ft| ft.is_dir()).unwrap_or(false) { + let remove_result = if child + .file_type() + .map(|ft| ft.is_dir()) + .unwrap_or(false) + { std::fs::remove_dir_all(&child_path) } else { std::fs::remove_file(&child_path) @@ -353,7 +367,10 @@ impl ImageStore { // Skip absolute paths and path traversal. // Check each component individually — "foo..bar" is safe, "../foo" is not. let has_traversal = path.components().any(|c| { - matches!(c, std::path::Component::ParentDir | std::path::Component::RootDir) + matches!( + c, + std::path::Component::ParentDir | std::path::Component::RootDir + ) }); if has_traversal { tracing::warn!(path = %path_str, "skipping tar entry with path traversal"); @@ -417,8 +434,8 @@ mod tests { ] }"#; - let manifest = ImageStore::parse_manifest(manifest_json.as_bytes()) - .expect("manifest should parse"); + let manifest = + ImageStore::parse_manifest(manifest_json.as_bytes()).expect("manifest should parse"); assert_eq!(manifest.config_digest, "sha256:abc123"); assert_eq!(manifest.layer_digests.len(), 2); } @@ -434,8 +451,7 @@ mod tests { } }"#; - let config = ImageStore::parse_config(config_json.as_bytes()) - .expect("config should parse"); + let config = ImageStore::parse_config(config_json.as_bytes()).expect("config should parse"); assert_eq!(config.cmd, vec!["nginx", "-g", "daemon off;"]); assert_eq!(config.env.len(), 2); assert_eq!(config.exposed_ports, vec![80]); @@ -446,7 +462,9 @@ mod tests { let tmp = tempfile::tempdir().expect("tempdir"); let store = ImageStore::new(tmp.path()).expect("store"); let path = store.blob_path("sha256:abc123def456"); - assert!(path.to_string_lossy().ends_with("blobs/sha256/abc123def456")); + assert!(path + .to_string_lossy() + .ends_with("blobs/sha256/abc123def456")); } #[test] @@ -480,15 +498,15 @@ mod tests { "config": { "digest": "sha256:cfg" }, "layers": [{}, { "digest": "sha256:layer2" }] }"#; - let err = ImageStore::parse_manifest(manifest_json.as_bytes()).expect_err("missing digest should fail"); + let err = ImageStore::parse_manifest(manifest_json.as_bytes()) + .expect_err("missing digest should fail"); assert!(err.to_string().contains("missing layer digest")); } #[test] fn parse_config_minimal() { let config_json = r#"{"config": {}}"#; - let config = ImageStore::parse_config(config_json.as_bytes()) - .expect("config should parse"); + let config = ImageStore::parse_config(config_json.as_bytes()).expect("config should parse"); assert!(config.cmd.is_empty()); assert!(config.env.is_empty()); assert!(config.exposed_ports.is_empty()); @@ -502,8 +520,7 @@ mod tests { "Cmd": ["nginx"] } }"#; - let config = ImageStore::parse_config(config_json.as_bytes()) - .expect("config should parse"); + let config = ImageStore::parse_config(config_json.as_bytes()).expect("config should parse"); assert_eq!(config.entrypoint, vec!["/docker-entrypoint.sh"]); assert_eq!(config.cmd, vec!["nginx"]); } @@ -515,8 +532,7 @@ mod tests { "ExposedPorts": { "80/tcp": {}, "443/tcp": {}, "8080/tcp": {} } } }"#; - let config = ImageStore::parse_config(config_json.as_bytes()) - .expect("config should parse"); + let config = ImageStore::parse_config(config_json.as_bytes()).expect("config should parse"); let mut ports = config.exposed_ports.clone(); ports.sort(); assert_eq!(ports, vec![80, 443, 8080]); diff --git a/src/setup/image.rs b/src/setup/image.rs index e21b498..1255271 100644 --- a/src/setup/image.rs +++ b/src/setup/image.rs @@ -114,22 +114,14 @@ fn resolve_ubuntu(version: &str, arch: Arch) -> Result, SetupErr Ok(vec![ ImageAsset { filename: "vmlinuz", - source_filename: format!( - "ubuntu-{version}-server-cloudimg-{arch_str}-vmlinuz-generic" - ), - url: format!( - "{unpacked}/ubuntu-{version}-server-cloudimg-{arch_str}-vmlinuz-generic" - ), + source_filename: format!("ubuntu-{version}-server-cloudimg-{arch_str}-vmlinuz-generic"), + url: format!("{unpacked}/ubuntu-{version}-server-cloudimg-{arch_str}-vmlinuz-generic"), checksum_url: Some(format!("{unpacked}/SHA256SUMS")), }, ImageAsset { filename: "initramfs", - source_filename: format!( - "ubuntu-{version}-server-cloudimg-{arch_str}-initrd-generic" - ), - url: format!( - "{unpacked}/ubuntu-{version}-server-cloudimg-{arch_str}-initrd-generic" - ), + source_filename: format!("ubuntu-{version}-server-cloudimg-{arch_str}-initrd-generic"), + url: format!("{unpacked}/ubuntu-{version}-server-cloudimg-{arch_str}-initrd-generic"), checksum_url: Some(format!("{unpacked}/SHA256SUMS")), }, ImageAsset { @@ -176,8 +168,13 @@ pub async fn prepare_image( let path = image_dir.join(asset.filename); let expected_sha256 = match asset.checksum_url.as_deref() { Some(checksum_url) => Some( - expected_sha256(&client, checksum_url, &asset.source_filename, &mut checksum_cache) - .await?, + expected_sha256( + &client, + checksum_url, + &asset.source_filename, + &mut checksum_cache, + ) + .await?, ), None => None, }; @@ -278,13 +275,9 @@ async fn download_file( path: &Path, expected_sha256: Option<&str>, ) -> Result<(), SetupError> { - let resp = client - .get(url) - .send() - .await - .map_err(|e| { - SetupError::AssetDownload(format!("HTTP request failed for {}: {}", url, e)) - })?; + let resp = client.get(url).send().await.map_err(|e| { + SetupError::AssetDownload(format!("HTTP request failed for {}: {}", url, e)) + })?; if !resp.status().is_success() { return Err(SetupError::AssetDownload(format!( @@ -295,10 +288,7 @@ async fn download_file( } let bytes = resp.bytes().await.map_err(|e| { - SetupError::AssetDownload(format!( - "failed to read response body from {}: {}", - url, e - )) + SetupError::AssetDownload(format!("failed to read response body from {}: {}", url, e)) })?; if let Some(expected) = expected_sha256 { @@ -349,16 +339,12 @@ async fn fetch_checksum_manifest( client: &reqwest::Client, checksum_url: &str, ) -> Result, SetupError> { - let resp = client - .get(checksum_url) - .send() - .await - .map_err(|e| { - SetupError::AssetDownload(format!( - "failed to fetch checksum manifest {}: {}", - checksum_url, e - )) - })?; + let resp = client.get(checksum_url).send().await.map_err(|e| { + SetupError::AssetDownload(format!( + "failed to fetch checksum manifest {}: {}", + checksum_url, e + )) + })?; if !resp.status().is_success() { return Err(SetupError::AssetDownload(format!( @@ -393,10 +379,7 @@ fn parse_checksum_manifest(body: &str) -> Result, SetupE }; let (digest, path) = line.split_at(split_at); let digest = digest.trim(); - let filename = path - .trim() - .trim_start_matches('*') - .trim_start_matches("./"); + let filename = path.trim().trim_start_matches('*').trim_start_matches("./"); if digest.len() == 64 && digest.chars().all(|c| c.is_ascii_hexdigit()) { manifest.insert(filename.to_string(), digest.to_ascii_lowercase()); } diff --git a/src/setup/seed.rs b/src/setup/seed.rs index e88d36b..3e86395 100644 --- a/src/setup/seed.rs +++ b/src/setup/seed.rs @@ -93,6 +93,13 @@ pub struct HealthCheckConfig { /// On macOS: uses `hdiutil makehybrid` to create the ISO. /// On Linux: uses `genisoimage` or `mkisofs`. pub fn create_seed_iso(iso_path: &Path, config: &SeedConfig<'_>) -> Result<(), SetupError> { + if !is_safe_hostname(config.hostname) { + return Err(SetupError::Config(format!( + "invalid hostname '{}'", + config.hostname + ))); + } + let parent = iso_path .parent() .ok_or_else(|| SetupError::Config("no parent directory for ISO path".into()))?; @@ -111,33 +118,38 @@ pub fn create_seed_iso(iso_path: &Path, config: &SeedConfig<'_>) -> Result<(), S std::fs::create_dir_all(&tmp_dir).map_err(SetupError::Io)?; - // Write meta-data - let meta_data = format!( - "instance-id: {hostname}\nlocal-hostname: {hostname}\n", - hostname = config.hostname - ); - std::fs::write(tmp_dir.join("meta-data"), &meta_data).map_err(SetupError::Io)?; - - // Write user-data - let user_data = build_user_data(config)?; - std::fs::write(tmp_dir.join("user-data"), &user_data).map_err(SetupError::Io)?; - - // Write network-config (v2) - if !config.nics.is_empty() { - let network_config = build_network_config(config)?; - std::fs::write(tmp_dir.join("network-config"), &network_config) - .map_err(SetupError::Io)?; - } + let result = (|| { + // Write meta-data + let meta_data = format!( + "instance-id: {hostname}\nlocal-hostname: {hostname}\n", + hostname = config.hostname + ); + std::fs::write(tmp_dir.join("meta-data"), &meta_data).map_err(SetupError::Io)?; + + // Write user-data + let user_data = build_user_data(config)?; + std::fs::write(tmp_dir.join("user-data"), &user_data).map_err(SetupError::Io)?; + + // Write network-config (v2) + if !config.nics.is_empty() { + let network_config = build_network_config(config)?; + std::fs::write(tmp_dir.join("network-config"), &network_config) + .map_err(SetupError::Io)?; + } - // Create ISO - create_iso_image(iso_path, &tmp_dir)?; + // Create ISO + create_iso_image(iso_path, &tmp_dir) + })(); - // Clean up temp dir if let Err(e) = std::fs::remove_dir_all(&tmp_dir) { - tracing::warn!(path = %tmp_dir.display(), "failed to clean up seed ISO temp dir: {}", e); + tracing::warn!( + path = %tmp_dir.display(), + "failed to clean up seed ISO temp dir: {}", + e + ); } - Ok(()) + result } // --------------------------------------------------------------------------- @@ -155,7 +167,10 @@ fn build_user_data(config: &SeedConfig<'_>) -> Result { // Mount VirtioFS volumes for vol in &config.volumes { if !is_safe_mount_tag(&vol.tag) { - return Err(SetupError::Config(format!("invalid VirtioFS tag '{}'", vol.tag))); + return Err(SetupError::Config(format!( + "invalid VirtioFS tag '{}'", + vol.tag + ))); } if !is_safe_mount_point(&vol.mount_point) { return Err(SetupError::Config(format!( @@ -200,7 +215,10 @@ fn build_user_data(config: &SeedConfig<'_>) -> Result { } if let Some(ref wd) = proc.workdir { if !is_safe_mount_point(wd) { - return Err(SetupError::Config(format!("invalid working directory '{}'", wd))); + return Err(SetupError::Config(format!( + "invalid working directory '{}'", + wd + ))); } ud.push_str(&format!( " - cd {} && sh -lc {}\n", @@ -232,7 +250,8 @@ fn build_user_data(config: &SeedConfig<'_>) -> Result { let ip_cmd = "hostname -I | awk '{print $1}'"; ud.push_str(&format!( " - echo \"{} $({})\"\n", - crate::config::READY_MARKER, ip_cmd + crate::config::READY_MARKER, + ip_cmd )); Ok(ud) @@ -385,7 +404,11 @@ fn is_safe_mount_tag(s: &str) -> bool { } fn is_safe_mount_point(s: &str) -> bool { - s.starts_with('/') && !s.contains('\0') && !s.contains('\n') && !s.contains('\r') && s.len() <= 1024 + s.starts_with('/') + && !s.contains('\0') + && !s.contains('\n') + && !s.contains('\r') + && s.len() <= 1024 } fn is_safe_iface_name(s: &str) -> bool { @@ -411,7 +434,9 @@ fn validate_ssh_pubkey(s: &str) -> Result<(), SetupError> { return Err(SetupError::Config("SSH public key is empty".into())); }; let Some(key_material) = parts.next() else { - return Err(SetupError::Config("SSH public key is missing key material".into())); + return Err(SetupError::Config( + "SSH public key is missing key material".into(), + )); }; if !key_type.starts_with("ssh-") && !key_type.starts_with("ecdsa-") { @@ -421,7 +446,9 @@ fn validate_ssh_pubkey(s: &str) -> Result<(), SetupError> { ))); } if key_material.is_empty() { - return Err(SetupError::Config("SSH public key is missing key material".into())); + return Err(SetupError::Config( + "SSH public key is missing key material".into(), + )); } Ok(()) @@ -506,6 +533,29 @@ mod tests { assert!(!is_safe_hostname(&long)); } + #[test] + fn create_seed_iso_rejects_invalid_hostname_before_running_tools() { + let tmp = tempfile::tempdir().expect("tempdir"); + let iso_path = tmp.path().join("seed.iso"); + let config = SeedConfig { + hostname: "../bad-host", + ssh_pubkey: "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAItest test@test", + nics: vec![], + process: None, + volumes: vec![], + healthcheck: None, + extra_hosts: vec![], + }; + + let err = create_seed_iso(&iso_path, &config) + .expect_err("invalid hostname should fail before ISO tool invocation"); + assert!( + err.to_string().contains("invalid hostname"), + "expected invalid hostname error, got: {}", + err + ); + } + // ── is_safe_ip ─────────────────────────────────────────────────────── #[test] @@ -606,7 +656,10 @@ mod tests { process: Some(ProcessConfig { command: "/bin/app".into(), workdir: None, - env: vec![("GOOD".into(), "ok".into()), ("BAD;rm".into(), "evil".into())], + env: vec![ + ("GOOD".into(), "ok".into()), + ("BAD;rm".into(), "evil".into()), + ], }), volumes: vec![], healthcheck: None, diff --git a/src/vm.rs b/src/vm.rs index 1c67e9a..2a88480 100644 --- a/src/vm.rs +++ b/src/vm.rs @@ -89,7 +89,9 @@ impl VmManager { // Ensure VM directory exists let vm_dir = self.vm_dir(&config.name); if let Err(e) = std::fs::create_dir_all(&vm_dir) { - let mut vms = self.vms.write() + let mut vms = self + .vms + .write() .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; vms.remove(&config.name); return Err(VmError::Io(e)); @@ -117,7 +119,10 @@ impl VmManager { // Track the VM { - let mut vms = self.vms.write().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + let mut vms = self + .vms + .write() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; vms.insert(config.name.clone(), handle.clone()); } @@ -131,7 +136,10 @@ impl VmManager { self.driver.stop(&handle)?; // Update state - let mut vms = self.vms.write().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + let mut vms = self + .vms + .write() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; if let Some(h) = vms.get_mut(name) { h.state = VmState::Stopped; } @@ -144,7 +152,10 @@ impl VmManager { let handle = self.get_handle(name)?; self.driver.kill(&handle)?; - let mut vms = self.vms.write().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + let mut vms = self + .vms + .write() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; if let Some(h) = vms.get_mut(name) { h.state = VmState::Stopped; } @@ -159,7 +170,10 @@ impl VmManager { tracing::info!(vm = %handle.name, "stopping VM by handle"); self.driver.stop(handle)?; - let mut vms = self.vms.write().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + let mut vms = self + .vms + .write() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; if let Some(h) = vms.get_mut(&handle.name) { h.state = VmState::Stopped; } @@ -171,7 +185,10 @@ impl VmManager { tracing::info!(vm = %handle.name, "force-killing VM by handle"); self.driver.kill(handle)?; - let mut vms = self.vms.write().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + let mut vms = self + .vms + .write() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; if let Some(h) = vms.get_mut(&handle.name) { h.state = VmState::Stopped; } @@ -182,7 +199,10 @@ impl VmManager { pub fn pause(&self, name: &str) -> Result<(), VmError> { let handle = self.get_handle(name)?; self.driver.pause(&handle)?; - let mut vms = self.vms.write().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + let mut vms = self + .vms + .write() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; if let Some(h) = vms.get_mut(name) { h.state = VmState::Paused; } @@ -193,9 +213,13 @@ impl VmManager { pub fn resume(&self, name: &str) -> Result<(), VmError> { let handle = self.get_handle(name)?; self.driver.resume(&handle)?; - let mut vms = self.vms.write().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + let resumed_state = self.driver.state(&handle)?; + let mut vms = self + .vms + .write() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; if let Some(h) = vms.get_mut(name) { - h.state = VmState::Starting; + h.state = resumed_state; } Ok(()) } @@ -206,7 +230,10 @@ impl VmManager { let state = self.driver.state(&handle)?; // Update cached state - let mut vms = self.vms.write().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + let mut vms = self + .vms + .write() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; if let Some(h) = vms.get_mut(name) { h.state = state.clone(); } @@ -223,7 +250,10 @@ impl VmManager { /// List all tracked VMs. pub fn list(&self) -> Result, VmError> { - let vms = self.vms.read().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + let vms = self + .vms + .read() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; Ok(vms.values().cloned().collect()) } @@ -235,7 +265,10 @@ impl VmManager { loop { if start.elapsed() > timeout { let pending: Vec = { - let vms = self.vms.read().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + let vms = self + .vms + .read() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; vms.iter() .filter(|(_, h)| !matches!(h.state, VmState::Running { .. })) .map(|(name, _)| name.clone()) @@ -249,7 +282,10 @@ impl VmManager { let mut all_ready = true; let names: Vec = { - let vms = self.vms.read().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + let vms = self + .vms + .read() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; vms.keys().cloned().collect() }; @@ -274,7 +310,10 @@ impl VmManager { { let pending: Vec = { - let vms = self.vms.read().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + let vms = self + .vms + .read() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; vms.iter() .filter(|(_, h)| !matches!(h.state, VmState::Running { .. })) .map(|(name, _)| name.clone()) @@ -296,7 +335,9 @@ impl VmManager { /// On Linux: reflink (`cp --reflink=auto`), falls back to regular copy. pub fn clone_disk(base: &Path, target: &Path) -> Result<(), VmError> { if target.exists() { - if let (Ok(base_meta), Ok(target_meta)) = (std::fs::metadata(base), std::fs::metadata(target)) { + if let (Ok(base_meta), Ok(target_meta)) = + (std::fs::metadata(base), std::fs::metadata(target)) + { if base_meta.len() == target_meta.len() { tracing::debug!(target = %target.display(), "disk clone target already exists with matching size, skipping"); return Ok(()); @@ -363,12 +404,13 @@ impl VmManager { } fn get_handle(&self, name: &str) -> Result { - let vms = self.vms.read().map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; - vms.get(name) - .cloned() - .ok_or_else(|| VmError::NotFound { - name: name.to_string(), - }) + let vms = self + .vms + .read() + .map_err(|e| VmError::Hypervisor(format!("lock poisoned: {}", e)))?; + vms.get(name).cloned().ok_or_else(|| VmError::NotFound { + name: name.to_string(), + }) } } @@ -384,7 +426,12 @@ fn create_platform_driver() -> Result, VmError> { Ok(Box::new(crate::driver::cloud_hv::CloudHvDriver::new())) } - #[cfg(not(any(target_os = "macos", target_os = "linux")))] + #[cfg(target_os = "windows")] + { + Ok(Box::new(crate::driver::whp::WhpDriver::new())) + } + + #[cfg(not(any(target_os = "macos", target_os = "linux", target_os = "windows")))] { Err(VmError::Hypervisor(format!( "unsupported platform: {}", @@ -509,11 +556,16 @@ mod tests { std::thread::sleep(std::time::Duration::from_millis(10)); } - let err = manager.start(&config).expect_err("second concurrent start should fail"); + let err = manager + .start(&config) + .expect_err("second concurrent start should fail"); assert!(err.to_string().contains("already exists")); release_tx.send(()).expect("release first boot"); - boot_thread.join().expect("join").expect("first boot should succeed"); + boot_thread + .join() + .expect("join") + .expect("first boot should succeed"); assert_eq!(driver.boot_calls.load(Ordering::SeqCst), 1); } @@ -529,6 +581,8 @@ mod tests { let state = manager.state(&config.name).expect("state query"); assert!(matches!(state, VmState::Failed { .. })); - manager.start(&config).expect("restart after failed state should be allowed"); + manager + .start(&config) + .expect("restart after failed state should be allowed"); } } diff --git a/tests/disk_clone.rs b/tests/disk_clone.rs index fbfc4b1..2c10d57 100644 --- a/tests/disk_clone.rs +++ b/tests/disk_clone.rs @@ -36,14 +36,22 @@ fn clone_disk_idempotent() { VmManager::clone_disk(&base, &target).expect("second clone"); let content = std::fs::read(&target).expect("read clone"); - assert_eq!(content, b"original", "idempotent clone should NOT overwrite"); + assert_eq!( + content, b"original", + "idempotent clone should NOT overwrite" + ); } #[test] fn clone_disk_creates_parent_dirs() { let tmp = tempfile::tempdir().expect("tempdir"); let base = tmp.path().join("base.img"); - let target = tmp.path().join("deep").join("nested").join("dir").join("clone.img"); + let target = tmp + .path() + .join("deep") + .join("nested") + .join("dir") + .join("clone.img"); std::fs::write(&base, b"data").expect("write base"); diff --git a/tests/network_switch.rs b/tests/network_switch.rs index 4bff680..069f547 100644 --- a/tests/network_switch.rs +++ b/tests/network_switch.rs @@ -105,7 +105,11 @@ fn unicast_to_learned_mac() { assert_eq!(n_b, unicast.len() as isize, "B should receive unicast"); let n_c = recv_raw(&fd_c, &mut buf); - assert!(n_c <= 0, "C should NOT receive unicast destined for B, got {} bytes", n_c); + assert!( + n_c <= 0, + "C should NOT receive unicast destined for B, got {} bytes", + n_c + ); switch.stop(); } @@ -115,7 +119,9 @@ fn unicast_to_learned_mac() { #[test] fn different_networks_are_isolated() { let switch = NetworkSwitch::new(); - let fd_frontend = switch.add_port("frontend", "web").expect("add frontend port"); + let fd_frontend = switch + .add_port("frontend", "web") + .expect("add frontend port"); let fd_backend = switch.add_port("backend", "db").expect("add backend port"); switch.start().expect("start switch"); @@ -126,7 +132,10 @@ fn different_networks_are_isolated() { let mut buf = vec![0u8; 1518]; let n = recv_raw(&fd_backend, &mut buf); - assert!(n <= 0, "backend network should NOT receive frames from frontend network"); + assert!( + n <= 0, + "backend network should NOT receive frames from frontend network" + ); switch.stop(); } @@ -250,11 +259,19 @@ fn multiple_networks_operate_independently() { // net-a: a2 should receive frame_a let n = recv_raw(&fd_a2, &mut buf); - assert_eq!(n, frame_a.len() as isize, "a2 should receive net-a broadcast"); + assert_eq!( + n, + frame_a.len() as isize, + "a2 should receive net-a broadcast" + ); // net-b: b2 should receive frame_b let n = recv_raw(&fd_b2, &mut buf); - assert_eq!(n, frame_b.len() as isize, "b2 should receive net-b broadcast"); + assert_eq!( + n, + frame_b.len() as isize, + "b2 should receive net-b broadcast" + ); // Cross-check: a2 should NOT get net-b's frame let n = recv_raw(&fd_a2, &mut buf); diff --git a/tests/oci_pull.rs b/tests/oci_pull.rs index df74c90..0958b60 100644 --- a/tests/oci_pull.rs +++ b/tests/oci_pull.rs @@ -29,10 +29,7 @@ fn parse_ghcr_image() { #[test] fn parse_ecr_image() { let r = parse_image_ref("123456789012.dkr.ecr.us-east-1.amazonaws.com/my-app:v1.0"); - assert_eq!( - r.registry, - "123456789012.dkr.ecr.us-east-1.amazonaws.com" - ); + assert_eq!(r.registry, "123456789012.dkr.ecr.us-east-1.amazonaws.com"); assert_eq!(r.repository, "my-app"); assert_eq!(r.tag, "v1.0"); } @@ -49,7 +46,10 @@ async fn pull_alpine_image_from_dockerhub() { let result = vm_rs::oci::pull("alpine:3.20", &store).await; let manifest = result.expect("pull alpine"); - assert!(!manifest.layer_digests.is_empty(), "alpine should have layers"); + assert!( + !manifest.layer_digests.is_empty(), + "alpine should have layers" + ); assert!( !manifest.config_digest.is_empty(), "should have config digest" @@ -129,16 +129,18 @@ async fn pull_nonexistent_image_fails() { let tmp = tempfile::tempdir().expect("tempdir"); let store = ImageStore::new(tmp.path()).expect("store"); - let result = - vm_rs::oci::pull("library/this-image-does-not-exist-12345:v999", &store).await; + let result = vm_rs::oci::pull("library/this-image-does-not-exist-12345:v999", &store).await; match result { Err(e) => { let msg = e.to_string(); // Should be an auth or HTTP error, not a panic assert!( - msg.contains("HTTP") || msg.contains("auth") || msg.contains("404") - || msg.contains("UNAUTHORIZED") || msg.contains("NAME_UNKNOWN"), + msg.contains("HTTP") + || msg.contains("auth") + || msg.contains("404") + || msg.contains("UNAUTHORIZED") + || msg.contains("NAME_UNKNOWN"), "expected HTTP/auth error, got: {}", msg ); diff --git a/tests/seed_iso.rs b/tests/seed_iso.rs index feaaf49..beebf0c 100644 --- a/tests/seed_iso.rs +++ b/tests/seed_iso.rs @@ -3,7 +3,9 @@ //! These tests create actual ISO files and verify their contents. //! Requires: hdiutil (macOS) or genisoimage/mkisofs (Linux). -use vm_rs::setup::{create_seed_iso, HealthCheckConfig, NicConfig, ProcessConfig, SeedConfig, VolumeMountConfig}; +use vm_rs::setup::{ + create_seed_iso, HealthCheckConfig, NicConfig, ProcessConfig, SeedConfig, VolumeMountConfig, +}; fn has_iso_tool() -> bool { #[cfg(target_os = "macos")] @@ -144,11 +146,21 @@ fn seed_iso_cleans_up_temp_dir() { create_seed_iso(&iso_path, &config).expect("ISO creation failed"); - // The temp dir .seed- should be cleaned up - let seed_tmp = tmp.path().join(".seed-cleanup-test"); + let leftovers: Vec<_> = std::fs::read_dir(tmp.path()) + .expect("read_dir") + .filter_map(|entry| entry.ok()) + .filter_map(|entry| { + let file_name = entry.file_name(); + let file_name = file_name.to_str()?; + file_name + .starts_with(".seed-cleanup-test-") + .then_some(file_name.to_string()) + }) + .collect(); assert!( - !seed_tmp.exists(), - "temp dir should be cleaned up after ISO creation" + leftovers.is_empty(), + "temp dirs should be cleaned up after ISO creation, found: {:?}", + leftovers ); } diff --git a/tests/vm_manager.rs b/tests/vm_manager.rs index 2ee2134..9b2220f 100644 --- a/tests/vm_manager.rs +++ b/tests/vm_manager.rs @@ -80,13 +80,13 @@ impl VmDriver for MockDriver { .lock() .expect("vms lock should not be poisoned") .insert( - config.name.clone(), - MockVmState { - state: VmState::Running { - ip: "10.0.0.99".into(), + config.name.clone(), + MockVmState { + state: VmState::Running { + ip: "10.0.0.99".into(), + }, }, - }, - ); + ); Ok(handle) } @@ -428,9 +428,17 @@ fn resume_paused_vm_returns_to_running() { .resume("mock-resume") .expect("resume should succeed"); - // After resume, the manager sets state to Starting (waiting for ready marker). - // But the mock driver already set it to Running internally. - // The next state() call will pick up Running from the driver. + let listed = manager.list().expect("list"); + let listed_vm = listed + .into_iter() + .find(|vm| vm.name == "mock-resume") + .expect("resumed VM should still be tracked"); + assert!( + matches!(listed_vm.state, VmState::Running { .. }), + "cached handle state should be Running after resume, got: {}", + listed_vm.state + ); + let state = manager.state("mock-resume").expect("state"); assert!( matches!(state, VmState::Running { .. }), @@ -556,7 +564,9 @@ fn config_machine_id_roundtrip() { let id = vec![1, 2, 3, 4, 5, 6, 7, 8]; config.machine_id = Some(id.clone()); assert_eq!( - config.machine_id.expect("machine_id should be present after assignment"), + config + .machine_id + .expect("machine_id should be present after assignment"), id ); } From 788d3a1f63639be70378be3f64c064d7a7d906c5 Mon Sep 17 00:00:00 2001 From: Soheil Alizadeh Date: Sat, 14 Mar 2026 11:46:19 +0100 Subject: [PATCH 3/3] Fix CI lint and Windows driver wiring --- src/driver/cloud_hv.rs | 10 ++++++++-- src/driver/mod.rs | 3 +++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/driver/cloud_hv.rs b/src/driver/cloud_hv.rs index 93116ec..78aa132 100644 --- a/src/driver/cloud_hv.rs +++ b/src/driver/cloud_hv.rs @@ -38,6 +38,12 @@ pub struct CloudHvDriver { vms: Mutex>, } +impl Default for CloudHvDriver { + fn default() -> Self { + Self::new() + } +} + impl CloudHvDriver { pub fn new() -> Self { Self { @@ -198,7 +204,7 @@ impl VmDriver for CloudHvDriver { name: name.clone(), detail: format!("failed to create VMM log file: {}", e), })?; - let vmm_log_stderr = vmm_log.try_clone().map_err(|e| VmError::Io(e))?; + let vmm_log_stderr = vmm_log.try_clone().map_err(VmError::Io)?; let mut process = cmd .stdout(vmm_log) .stderr(vmm_log_stderr) @@ -221,7 +227,7 @@ impl VmDriver for CloudHvDriver { // Brief pause then check if process exited immediately (bad binary, permissions, etc.) std::thread::sleep(std::time::Duration::from_millis(100)); - if let Some(status) = process.try_wait().map_err(|e| VmError::Io(e))? { + if let Some(status) = process.try_wait().map_err(VmError::Io)? { // Clean up virtiofsd processes since VM failed cleanup_virtiofsd(&virtiofsd_pids); return Err(VmError::BootFailed { diff --git a/src/driver/mod.rs b/src/driver/mod.rs index 63b63de..8431dac 100644 --- a/src/driver/mod.rs +++ b/src/driver/mod.rs @@ -10,6 +10,9 @@ pub mod apple_vz; #[cfg(target_os = "linux")] pub mod cloud_hv; +#[cfg(target_os = "windows")] +pub mod boot; + #[cfg(target_os = "windows")] pub mod whp;