Fix 21 bugs from codebase review (Waves 1+2)

[ejc3]

Summary

Systematic codebase review identified 57 issues across fcvm, fuse-pipe, and fc-agent. This PR fixes 21 of them across two waves plus review feedback.

Wave 1 — 8 quick wins

• Wire protocol truncation (Bump actions/checkout from 4 to 6 #7): Written { size } changed from u32 to u64 to prevent silent truncation on copy_file_range returns >4GB
• Loopback IP exhaustion (CI: Add cargo symlinks for lint tests under sudo #19): bail! instead of silently reusing 127.0.0.2 (would cause IP conflicts)
• xattr correctness (CI: Debug logging and caching (3/4) #24): Removed security.capability fast-path that returned ENODATA for all files
• e2fsck exit code (Add automatic prerequisite setup to inception tests #30): Check exit code before resize2fs — exit >= 4 means uncorrectable errors
• slirp pipe deadlock (Fix formatting in test_btrfs_fuse.rs #40): stdout to Stdio::null(), stderr stays piped for diagnostics
• truncate exit code (Fix CI: Update GitHub Actions, fix issues, add test logging #45): Check success in create_disk_from_dir
• parse_size overflow (Enable VHE mode and recursive nested virtualization #49): checked_mul prevents silent overflow
• Dead code (Enable nested virtualization with performance benchmarks #50): Deleted unused mount_vsock_with_readers in fc-agent

Wave 2 — 10 correctness and safety fixes

• UFFD server auto-exit (Paths, cleanup, and test features #10): Use CancellationToken instead of exiting when connections drop to zero — server must stay alive for new clones
• Health monitor false positive (Prepare repo for open source release #11): Return None (unknown) instead of Some(true) when fcvm binary not found
• Health monitor no fast polling revert ([ABANDONED] Embed fc-agent binary + --base-image flag #12): Reset to startup interval when VM becomes unhealthy
• Hardcoded port 80 (Simplify CI to 3 make targets #13): Use url.port().unwrap_or(80) for rootless health checks
• Slirp port forwarding (Add XDG config file support for cargo install #14): bail! on error response instead of silent warning
• Bridged cleanup (Rust packaging for cargo install #15): Collect all errors instead of stopping on first failure
• Clone setup leak (Add inception support: KVM-enabled kernel and /dev/kvm creation #20): Clean up data_dir and state file on failure
• Thread-local groups panic safety (CI: Base infrastructure and test tiers (1/4) #22): Drop guard ensures CURRENT_GROUPS cleanup on panic
• Shell injection (ARM64 nested virtualization (NV2) support for inception #28): Replace bash -c with direct Command args for TAP device verification
• set_var unsoundness (Add FUSE remap_file_range support for FICLONE/FICLONERANGE #26): Pass max_write as parameter instead of std::env::set_var in multi-threaded context

Review feedback — 3 fixes

• e2fsck signal-kill (Add automatic prerequisite setup to inception tests #30): unwrap_or(1) → unwrap_or(8) so signal-killed process is treated as fatal
• slirp stderr (Fix formatting in test_btrfs_fuse.rs #40): Keep stderr piped for error diagnostics (only read after exit, no deadlock)
• Stale README: Removed FCVM_NO_XATTR_FASTPATH env var docs after fast-path removal

Test plan

```
cargo test -p fuse-pipe --lib # 42 pass
cargo test -p fcvm --lib # 48 pass
cargo test -p exec-proto # 6 pass
cargo build --release # clean
cargo fmt -- --check # clean
```

[claude-claude]

🔍 Claude Review

SEVERITY: medium

Findings

[MEDIUM] e2fsck signal-kill mishandled — unwrap_or(1) lets corrupted filesystems through (src/storage/disk.rs:207)

The new e2fsck exit code check uses:

if e2fsck_output.status.code().unwrap_or(1) >= 4 {

On Unix, ExitStatus::code() returns None when a process is killed by a signal (SIGKILL, SIGSEGV, etc.). With unwrap_or(1), a signal-killed e2fsck is treated as exit code 1 ("errors corrected"), which passes the >= 4 check and allows resize2fs to proceed on an unchecked (potentially corrupted) filesystem. The default should be >= 4 (e.g., unwrap_or(8)) so signal-terminated processes are treated as fatal errors.

[LOW] Stale README reference to removed FCVM_NO_XATTR_FASTPATH env var (README.md:880)

The PR correctly removed the security.capability xattr fast-path from fuse-pipe/src/client/fuse.rs, but README.md line 880 still documents the FCVM_NO_XATTR_FASTPATH environment variable. This is now dead documentation since the fast-path no longer exists.

[LOW] VolumeResponse::Written u32→u64 is cosmetic — FUSE ABI caps at u32

The wire protocol field was widened to u64, but reply.written(size as u32) truncates back to u32 at the FUSE interface (the kernel ABI struct fuse_write_out uses u32). The change prevents no real truncation since individual FUSE write/copy_file_range syscalls never return >4GB. This is not a bug but the PR description's framing as a truncation fix is slightly misleading.

Summary

The e2fsck exit code improvement is valuable but has a bug: signal-killed processes bypass the error check due to unwrap_or(1). The stale README reference is minor cleanup. Overall the PR is a solid set of defensive fixes.

---

Review by Claude | Run

[claude-claude]

🔧 Auto-Fix Created

I found issues and created a fix PR: #269

Fixes included:

1. e2fsck signal-kill default: unwrap_or(1) → unwrap_or(8) so signal-killed processes are treated as fatal errors
2. Stale README reference: Removed FCVM_NO_XATTR_FASTPATH env var documentation (fast-path was removed in this PR)

Please review and merge the fix PR first, then this PR.

View Claude Run

[claude-claude]

🔧 CI Auto-Fix

Created fix PR: #270

The CI failure was caused by a rustfmt formatting issue in src/commands/podman.rs. The bail! macro call needed to be split across multiple lines according to rustfmt's formatting rules.

View Claude Run

[claude-claude]

🔍 Claude Review

SEVERITY: medium

Findings

[MEDIUM] Slirp stderr→null breaks error diagnostic path (src/network/slirp.rs:346-347, 365)

The PR changes slirp4netns stdout/stderr from Stdio::piped() to Stdio::null() to prevent a pipe-buffer deadlock. However, the error handling path at line 363-372 calls child.wait_with_output().await? to capture stderr when slirp4netns dies before becoming ready. With stderr now directed to /dev/null, output.stderr will always be empty, so the diagnostic will always be the unhelpful "slirp4netns exited before becoming ready (no stderr)" instead of showing the actual error message.

Fix: Keep stderr as Stdio::piped() (it's only read on the error path after the process exits, so no deadlock risk) or change stdout only to Stdio::null(). The deadlock risk was only from unconsumed piped output on long-running processes — stderr captured after wait_with_output() on a dead process is safe.

[MEDIUM] e2fsck signal-kill mishandled — previously reported, fix still pending

As noted in previous review, unwrap_or(1) on line 207 lets signal-killed e2fsck bypass the error check. Fix PR #269 is still open/unmerged.

[LOW] Stale README reference — previously reported, fix still pending

As noted in previous review, FCVM_NO_XATTR_FASTPATH env var is still documented in README line 878 after the fast-path was removed. Fix PR #269 covers this.

Summary

The slirp pipe-deadlock fix inadvertently breaks error diagnostics by sending stderr to null while the error handler still tries to read it. The e2fsck signal-kill issue from the previous review remains unfixed (PR #269 pending).

---

Review by Claude | Run

[claude-claude]

🔧 Auto-Fix Created

I found issues and created a fix PR: #271

Fixes included:

1. Slirp stderr diagnostics: Kept stderr as Stdio::piped() so error messages are captured when slirp4netns dies at startup (stdout remains Stdio::null() — no deadlock risk)
2. e2fsck signal-kill default: unwrap_or(1) → unwrap_or(8) so signal-killed processes are treated as fatal
3. Stale README: Removed FCVM_NO_XATTR_FASTPATH env var documentation

Note: This PR supersedes #269 which covered fixes 2 and 3 above, plus adds the new slirp stderr fix.

Please review and merge the fix PR first, then this PR.

View Claude Run