diff --git a/CLAUDE-REVIEW.md b/CLAUDE-REVIEW.md deleted file mode 100644 index 3fac732..0000000 --- a/CLAUDE-REVIEW.md +++ /dev/null @@ -1,1075 +0,0 @@ -# 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 deleted file mode 100644 index f20af79..0000000 --- a/COMBINED-REVIEW.md +++ /dev/null @@ -1,735 +0,0 @@ -# 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 deleted file mode 100644 index 72e0485..0000000 --- a/codex-review.md +++ /dev/null @@ -1,109 +0,0 @@ -# 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.