From 19a1a74b09a198783d48ccc1e767a51ca59837dd Mon Sep 17 00:00:00 2001 From: jrimmer Date: Fri, 14 Aug 2026 10:27:38 -0700 Subject: [PATCH 1/6] =?UTF-8?q?fix(vmm):=20immutable=20baseline=20rootfs?= =?UTF-8?q?=20=E2=80=94=20reflink=20clone=20before=20boot=20(closes=20#296?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rootfs corruption issue (#296) was caused by vm.kill() SIGKILLing firecracker without a clean ext4 unmount, leaving the rootfs dirty. The previous approach (PR #295) ran e2fsck -fy before each boot to repair the dirty journal — but the reviewer flagged a TOCTOU race: the /proc/*/fd scan is point-in-time, so another VM could open the rootfs during e2fsck. The immutable-baseline approach eliminates the race entirely: 1. The original rootfs is the IMMUTABLE BASELINE — never mounted RW. 2. Before each boot, forkd creates a reflink copy (FICLONE ioctl, instant on btrfs/xfs/overlayfs; falls back to full copy on ext4/tmpfs) in the snapshot directory. 3. The VM boots from the clone and writes to it; the baseline stays clean. 4. After vm.kill(), the clone persists as the snapshot's rootfs (needed for restores — Firecracker re-opens the rootfs from the path in the vmstate). 5. The next forkd snapshot --rootfs boots from a fresh clone of the still-clean baseline — no e2fsck needed, no TOCTOU race. The reflink copy is exposed as pub fn reflink_copy in chain.rs (wraps the existing copy_base_memory which already has FICLONE + stream fallback). The snapshot_cmd function in forkd-cli creates the clone at /rootfs.ext4 and records its path in snap.rootfs. Signed-off-by: jrimmer --- crates/forkd-cli/src/main.rs | 48 ++++++++++++++++++++++++++++++----- crates/forkd-vmm/src/chain.rs | 18 +++++++++++-- 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/crates/forkd-cli/src/main.rs b/crates/forkd-cli/src/main.rs index d1d5d95..c11c572 100644 --- a/crates/forkd-cli/src/main.rs +++ b/crates/forkd-cli/src/main.rs @@ -2497,13 +2497,50 @@ fn snapshot_cmd( .and_then(|s| s.to_str()) .is_some_and(|s| s == "ext4"); + // Immutable-baseline rootfs cloning (issue #296): + // + // For ext4 (read-write) rootfs, the original file is the immutable + // baseline — it is NEVER mounted read-write. Before booting, we + // create a reflink copy (instant on btrfs/xfs via FICLONE, falls + // back to full copy on other filesystems) in the snapshot directory. + // The VM boots from the clone and writes to it; the baseline stays + // clean. After the VM is killed, the clone persists as the + // snapshot's rootfs (needed for restores — Firecracker re-opens the + // rootfs from the path stored in the vmstate). + // + // This eliminates the dirty-journal corruption that e2fsck-on-boot + // was working around: since the baseline is never written to, it + // never has uncommitted journal transactions or dirty metadata. + let snap_dir = snapshot_dir(&tag); + let boot_rootfs = if rw { + std::fs::create_dir_all(&snap_dir).context("create snapshot dir for rootfs clone")?; + let clone_path = snap_dir.join("rootfs.ext4"); + // Remove any stale clone from a previous (failed) run. + let _ = std::fs::remove_file(&clone_path); + eprintln!(" rootfs mode: read-write (ext4, immutable baseline clone)"); + eprintln!( + " cloning rootfs {} → {} (reflink preferred)...", + rootfs.display(), + clone_path.display() + ); + forkd_vmm::chain::reflink_copy(&rootfs, &clone_path).with_context(|| { + format!( + "clone rootfs {} → {}", + rootfs.display(), + clone_path.display() + ) + })?; + clone_path + } else { + eprintln!(" rootfs mode: read-only (squashfs)"); + rootfs + }; + let work_dir = std::env::temp_dir().join(format!("forkd-parent-{tag}")); let mut cfg = if rw { - eprintln!(" rootfs mode: read-write (ext4)"); - BootConfig::ext4_rw(kernel, rootfs, work_dir.clone()) + BootConfig::ext4_rw(kernel, boot_rootfs, work_dir.clone()) } else { - eprintln!(" rootfs mode: read-only (squashfs)"); - BootConfig::quickstart(kernel, rootfs, work_dir.clone()) + BootConfig::quickstart(kernel, boot_rootfs, work_dir.clone()) }; if let Some(mib) = mem_size_mib { eprintln!(" memory: {mib} MiB (override; default is 512)"); @@ -2547,8 +2584,7 @@ fn snapshot_cmd( eprintln!("==> pausing..."); vm.pause().context("pause parent")?; - let snap_dir = snapshot_dir(&tag); - std::fs::create_dir_all(&snap_dir).context("create snapshot dir")?; + // snap_dir was created earlier for the rootfs clone. let vmstate = snap_dir.join("vmstate"); let memory = snap_dir.join("memory.bin"); diff --git a/crates/forkd-vmm/src/chain.rs b/crates/forkd-vmm/src/chain.rs index e77958d..e1e62c3 100644 --- a/crates/forkd-vmm/src/chain.rs +++ b/crates/forkd-vmm/src/chain.rs @@ -237,11 +237,25 @@ pub fn assemble_chain_memory(chain: &[(String, Snapshot)], out_path: &Path) -> R Ok(total) } -/// Copy `src` to `dst`. Tries reflink (ioctl `FICLONE`) first; falls -/// back to a regular streamed copy on non-reflink FS, logging once. +/// Copy `src` to `dst` using reflink (ioctl `FICLONE`) when available, +/// falling back to a regular streamed copy on non-reflink filesystems. +/// Returns the logical size of the source file. +/// +/// On reflink-capable filesystems (btrfs, xfs with `reflink=1`, overlayfs) +/// the copy is instant — the destination shares extents with the source +/// copy-on-write, so no data is physically copied until the destination +/// is written to. On other filesystems (ext4 without reflink, tmpfs) the +/// fallback streams the full file contents. /// /// Linux only on the reflink path; non-Linux builds always use the /// stream copy. +/// +/// The destination must not already exist (CREATE+EXCL). +pub fn reflink_copy(src: &Path, dst: &Path) -> Result { + copy_base_memory(src, dst) +} + +/// Internal implementation — same semantics as [`reflink_copy`]. #[cfg(target_os = "linux")] fn copy_base_memory(src: &Path, dst: &Path) -> Result { use std::os::unix::io::AsRawFd; From 76ab7f6655a859b9e8d8352760856978343c763e Mon Sep 17 00:00:00 2001 From: jrimmer Date: Mon, 17 Aug 2026 01:21:17 -0700 Subject: [PATCH 2/6] fix(snapshot): cache versioning, atomic staging, portable rootfs transport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review #295 r6 (WaylandYang 2026-08-14): three correctness/portability blockers on the immutable-baseline clone design. All three closed, plus the four requested regression tests. Blocker 1 — cache versioning (crates/forkd-cli/src/main.rs): A cached rootfs produced or dirtied by an older forkd version was reused solely because the path exists, then cloned as the supposedly immutable baseline. Now each built rootfs gets a `.cache-meta.json` sidecar recording schema_version + sha256 + image + size + forkd version. `validate_cached_rootfs` trusts a cache entry only when the meta exists, the schema version matches ROOTFS_CACHE_SCHEMA_VERSION (=1), and the live sha256 still matches. Legacy entries (no meta), schema mismatches, or sha mismatches (truncation/mutation) force a rebuild. Wired into both from_image_cmd and run_cmd cache-hit paths; write_rootfs_cache_meta is called after every build. Blocker 2 — atomic snapshot staging (snapshot_cmd + publish_snapshot): snapshot_cmd removed snap_dir/rootfs.ext4 BEFORE clone/boot/snapshot succeeded, so re-running a tag destroyed the last usable snapshot and (src==dst) could unlink its own source. The entire new snapshot (rootfs clone + vmstate + memory.bin + snapshot.json) is now built under a distinct staging dir (.staging-) and only published via publish_snapshot() after boot + warmup + snapshot + metadata write all succeed. publish_snapshot does a safe two-step shuffle: move the old snap_dir aside, rename staging into place (the commit point), then drop the old. On commit-point failure the old snapshot is restored from the aside, so a crash at any point leaves either the new OR the old snapshot, never neither. A src==dst guard rejects cloning the baseline into the snapshot's own rootfs.ext4 path. Blocker 3 — portable rootfs transport (crates/forkd-cli/src/hub.rs): The rootfs was shipped TWICE — tarred into the pack via SNAPSHOT_FILES AND emitted as a content-addressed .rootfs.zst sidecar — duplicating a potentially huge image. pack() now records rootfs.ext4 in the manifest files list (for integrity accounting + list_local) but skips appending it to the tar body when a portable sidecar is emitted, so there is ONE rootfs transport. RootfsRef.target_path is now a PORTABLE relative filename (e.g. "rootfs.ext4") instead of the packing host's absolute path; satisfy_rootfs resolves it against the destination snapshot dir (absolute paths from legacy packs still work). unpack_into now returns the dest snapshot dir so the relative target_path can be resolved; unpack_chain_into returns the head link's dest. Tests (crates/forkd-cli/src/main.rs): - validate_cached_rootfs_rejects_legacy_entry_without_meta (upgrade/dirty-cache) - validate_cached_rootfs_rejects_wrong_schema_version (upgrade/dirty-cache) - validate_cached_rootfs_rejects_sha_mismatch_after_mutation (dirty-cache) - validate_cached_rootfs_accepts_fresh_valid_entry - validate_cached_rootfs_misses_on_missing_file - publish_snapshot_atomically_replaces_existing (same-tag failure) - publish_snapshot_into_nonexistent_snap_dir - publish_snapshot_preserves_existing_when_staging_missing (same-tag failure recovery) Signed-off-by: jrimmer --- crates/forkd-cli/src/hub.rs | 37 ++- crates/forkd-cli/src/main.rs | 612 ++++++++++++++++++++++++++++++++--- 2 files changed, 595 insertions(+), 54 deletions(-) diff --git a/crates/forkd-cli/src/hub.rs b/crates/forkd-cli/src/hub.rs index f68dfc4..32ffed7 100644 --- a/crates/forkd-cli/src/hub.rs +++ b/crates/forkd-cli/src/hub.rs @@ -235,9 +235,21 @@ pub fn pack( }); } - // #242: ship the rootfs as a content-addressed sidecar next to the - // pack (it lives outside the snap dir, so it isn't in `files`). + // #242 / review #295 r6: ship the rootfs as a content-addressed + // `.rootfs.zst` sidecar next to the pack — the SINGLE portable + // rootfs transport. Returns `Ok(None)` (with a warning) when the + // snapshot records no rootfs or the file is missing; in that case + // the pack is produced without a portable rootfs and will only + // restore on the packing host. let rootfs = emit_rootfs_sidecar(snap_dir, out_path)?; + // When a portable sidecar was emitted, exclude rootfs.ext4 from the + // tar body (it's already counted in `files` for manifest integrity). + // The tar append loop below skips any entry whose path equals the + // rootfs filename when a sidecar is present. + let rootfs_filename: Option = rootfs + .as_ref() + .and_then(|r| Path::new(&r.target_path).file_name()) + .map(|s| s.to_string_lossy().into_owned());; let manifest = Manifest { forkd_pack_version: PACK_FORMAT_VERSION_V1, @@ -276,6 +288,14 @@ pub fn pack( .context("append manifest.toml")?; for entry in &files { + // Review #295 r6 blocker 3: don't tar the rootfs when a portable + // sidecar was emitted — the sidecar is the single transport and + // tar'd rootfs.ext4 would just duplicate a huge image. + if let Some(ref rf) = rootfs_filename { + if entry.path == *rf { + continue; + } + } let path = snap_dir.join(&entry.path); let mut f = File::open(&path).with_context(|| format!("open {}", path.display()))?; tar.append_file(&entry.path, &mut f) @@ -928,7 +948,18 @@ fn emit_rootfs_sidecar(snap_dir: &Path, pack_path: &Path) -> Result, force: bool) -> Result<()> { let _ = std::fs::remove_dir_all(&tmp); } // #242: a local unpack finds the rootfs sidecar next to the pack. - if let Ok(Some(rootfs)) = &result { - satisfy_rootfs(rootfs, SidecarSource::LocalSibling(&path))?; + // `unpack_into` returns the destination snapshot dir so we can resolve + // the portable (relative) target_path against it (review #295 r6). + if let Ok((dest, Some(rootfs))) = &result { + satisfy_rootfs(rootfs, SidecarSource::LocalSibling(&path), dest)?; } result.map(|_| ()) } @@ -1249,13 +1251,32 @@ enum SidecarSource<'a> { } /// #242: ensure the rootfs a pulled/unpacked snapshot needs is present -/// at the absolute path Firecracker will reopen at restore. Skips when -/// the target already exists with a matching sha (dedup across packs +/// at the path Firecracker will reopen at restore. Skips when the +/// target already exists with a matching sha (dedup across packs /// sharing a base); warns (does not fail) when the sidecar can't be /// found, so the user gets an actionable message at pull time instead /// of a cryptic block-device error at first fork. -fn satisfy_rootfs(rootfs: &hub::RootfsRef, source: SidecarSource) -> Result<()> { - let dst = PathBuf::from(&rootfs.target_path); +/// +/// `snap_dir` is the destination snapshot directory the pack was +/// unpacked into. `rootfs.target_path` is a PORTABLE, host-independent +/// path relative to `snap_dir` (review #295 r6 blocker 3 — e.g. +/// `"rootfs.ext4"`); it is resolved against `snap_dir` to get the +/// absolute path Firecracker reopens. If `target_path` is absolute +/// (legacy packs written before portability), it is used as-is. +fn satisfy_rootfs( + rootfs: &hub::RootfsRef, + source: SidecarSource, + snap_dir: &std::path::Path, +) -> Result<()> { + // Resolve the portable (relative) target_path against the + // destination snapshot dir. Absolute paths (legacy packs) are + // used as-is so old packs still restore. + let target = std::path::Path::new(&rootfs.target_path); + let dst = if target.is_absolute() { + target.to_path_buf() + } else { + snap_dir.join(target) + }; if dst.exists() { if let Ok(existing) = hub::sha256_file(&dst) { if existing.eq_ignore_ascii_case(&rootfs.sha256) { @@ -1333,12 +1354,12 @@ fn unpack_into( tmp: &std::path::Path, tag: Option, force: bool, -) -> Result> { +) -> Result<(std::path::PathBuf, Option)> { let manifest = hub::unpack(path, tmp)?; let rootfs = manifest.rootfs.clone(); if !manifest.chain.is_empty() { - return unpack_chain_into(tmp, manifest, tag, force).map(|()| rootfs); + return unpack_chain_into(tmp, manifest, tag, force).map(|dest| (dest, rootfs)); } // v1 layout (legacy single-snapshot pack). Validate the @@ -1379,7 +1400,7 @@ fn unpack_into( hub::rewrite_snapshot_paths(&dest)?; eprintln!("✓ unpacked tag '{final_tag}' at {}", dest.display()); eprintln!(" next: forkd fork --tag {final_tag} -n "); - Ok(rootfs) + Ok((dest, rootfs)) } /// v0.5 Phase 3: materialize a v2 chain pack — every link in @@ -1400,7 +1421,7 @@ fn unpack_chain_into( manifest: hub::Manifest, tag: Option, force: bool, -) -> Result<()> { +) -> Result { if tag.is_some() && manifest.chain.len() > 1 { bail!( "--tag override is not supported for multi-link chain packs \ @@ -1491,7 +1512,13 @@ fn unpack_chain_into( " next: forkd fork --tag {} -n ", final_tags.last().cloned().unwrap_or_default() ); - Ok(()) + // The rootfs sidecar belongs to the head link — return its dest so + // the caller can resolve the portable (relative) target_path against + // it in satisfy_rootfs. + Ok(destinations + .last() + .cloned() + .unwrap_or_else(|| std::path::PathBuf::new())) } /// Where `forkd pull /` resolves names to download URLs by @@ -1566,8 +1593,10 @@ fn pull_cmd(target: String, tag: Option, force: bool, hub: Option String { format!("{slug}-{size_mib}-{short}.ext4") } +/// Cache schema version for built rootfs artifacts (review #295 r6 +/// blocker 1: "a cache rootfs produced or dirtied by an older forkd +/// version is reused solely because the path exists"). Bumped when +/// the on-disk rootfs format or the cache-meta contract changes. A +/// cached rootfs whose meta records a different version — or whose +/// meta is missing (legacy/pre-versioning cache) — is treated as +/// untrusted and rebuilt. +const ROOTFS_CACHE_SCHEMA_VERSION: u32 = 1; + +/// Sidecar written next to each built rootfs recording the cache +/// schema version and the rootfs's sha256 at build time. On cache hit, +/// `validate_cached_rootfs` checks the meta exists, the schema version +/// matches, and the live sha256 still matches — so a rootfs dirtied by +/// an older forkd version (or truncated/partial) cannot be cloned as +/// the immutable baseline. +/// +/// The meta file is named `.cache-meta.json` so it sits +/// beside the rootfs and is easy to inspect/remove. It is NOT a +/// snapshot asset (not in SNAPSHOT_FILES) and is ignored by `pack`/`pull`. +#[derive(serde::Serialize, serde::Deserialize)] +struct RootfsCacheMeta { + schema_version: u32, + sha256: String, + image: String, + size_mib: u32, + /// `forkd` version that wrote this cache entry. Diagnostic only — + /// the schema_version is the migration gate, not this string. + forkd_version: String, +} + +/// Path of the `.cache-meta.json` sidecar for a given rootfs path. +fn rootfs_cache_meta_path(rootfs: &std::path::Path) -> std::path::PathBuf { + let mut s = rootfs.to_string_lossy().into_owned(); + s.push_str(".cache-meta.json"); + std::path::PathBuf::from(s) +} + +/// Write the cache-meta sidecar for a freshly built rootfs. Called +/// right after `parent_build_cmd` produces the rootfs. Computes the +/// sha256 of the finished file so subsequent cache hits can verify +/// integrity without re-hashing on every spawn. +fn write_rootfs_cache_meta(rootfs: &std::path::Path, image: &str, size_mib: u32) -> Result<()> { + let sha = sha256_file(rootfs)?; + let meta = RootfsCacheMeta { + schema_version: ROOTFS_CACHE_SCHEMA_VERSION, + sha256: sha, + image: image.to_string(), + size_mib, + forkd_version: env!("CARGO_PKG_VERSION").to_string(), + }; + let json = serde_json::to_vec_pretty(&meta).context("serialize rootfs cache meta")?; + std::fs::write(rootfs_cache_meta_path(rootfs), json) + .with_context(|| format!("write rootfs cache meta {}", rootfs_cache_meta_path(rootfs).display())) +} + +/// Decide whether a cached rootfs is safe to reuse as the immutable +/// baseline (review #295 r6 blocker 1). Returns `Ok(())` if the cache +/// entry is trusted, or `Err` describing why it must be rebuilt. The +/// caller rebuilds on `Err`. +/// +/// A cached rootfs is trusted only when ALL hold: +/// 1. The rootfs file exists. +/// 2. A `.cache-meta.json` sidecar exists (legacy entries with no +/// meta are untrusted — they may predate the immutable-baseline +/// clone design and have been written RW by an older forkd). +/// 3. The meta's `schema_version` equals the current +/// `ROOTFS_CACHE_SCHEMA_VERSION`. +/// 4. The live sha256 of the rootfs matches the meta's recorded sha256 +/// (catches truncation, partial writes, and on-disk mutation by a +/// prior snapshot that mounted the baseline RW by mistake). +/// +/// The sha256 re-hash is O(rootfs size) but runs only on cache hit (one +/// per base per host), not per spawn, and is the integrity guarantee the +/// immutable-baseline clone relies on. +fn validate_cached_rootfs(rootfs: &std::path::Path) -> Result<()> { + if !rootfs.exists() { + bail!("cache miss: rootfs {} does not exist", rootfs.display()); + } + let meta_path = rootfs_cache_meta_path(rootfs); + if !meta_path.exists() { + bail!( + "cache untrusted: rootfs {} has no .cache-meta.json sidecar \ + (legacy entry from an older forkd version); rebuilding", + rootfs.display() + ); + } + let meta_bytes = std::fs::read(&meta_path) + .with_context(|| format!("read rootfs cache meta {}", meta_path.display()))?; + let meta: RootfsCacheMeta = serde_json::from_slice(&meta_bytes) + .with_context(|| format!("parse rootfs cache meta {}", meta_path.display()))?; + if meta.schema_version != ROOTFS_CACHE_SCHEMA_VERSION { + bail!( + "cache untrusted: rootfs {} meta schema version {} <> current {}; \ + rebuilding (cache migration)", + rootfs.display(), + meta.schema_version, + ROOTFS_CACHE_SCHEMA_VERSION + ); + } + let live_sha = sha256_file(rootfs)?; + if live_sha != meta.sha256 { + bail!( + "cache untrusted: rootfs {} sha256 mismatch (meta={}, live={}); \ + the file was truncated, partially written, or mutated since \ + it was cached. Rebuilding to restore the immutable baseline.", + rootfs.display(), + meta.sha256, + live_sha + ); + } + Ok(()) +} + fn parent_build_cmd( image: String, output: Option, @@ -2147,13 +2289,27 @@ fn from_image_cmd( // 2. Materialize rootfs (cached). The cache key includes the image, // size, and extra packages so a rebuild with different flags does // not silently reuse a stale rootfs. + // + // Review #295 r6 blocker 1: a cached rootfs is trusted ONLY when a + // `.cache-meta.json` sidecar records the current schema version + // AND the live sha256 still matches. Legacy entries (no meta), + // schema mismatches, or sha mismatches (truncation/mutation) force + // a rebuild — the baseline is never cloned from an untrusted cache. std::fs::create_dir_all(&cache).ok(); let rootfs = cache.join(rootfs_cache_key(&image, size_mib, &extra)); - if !rootfs.exists() { - eprintln!("==> building rootfs for {image}"); - parent_build_cmd(image.clone(), Some(rootfs.clone()), size_mib, extra)?; - } else { - eprintln!("==> using cached rootfs {}", rootfs.display()); + match validate_cached_rootfs(&rootfs) { + Ok(()) => eprintln!("==> using cached rootfs {} (validated)", rootfs.display()), + Err(e) => { + if rootfs.exists() { + eprintln!("==> cache invalidated, rebuilding: {e}"); + } else { + eprintln!("==> building rootfs for {image}"); + } + parent_build_cmd(image.clone(), Some(rootfs.clone()), size_mib, extra.clone())?; + // Record the cache meta so the next hit can validate. + write_rootfs_cache_meta(&rootfs, &image, size_mib) + .with_context(|| format!("write cache meta for {}", rootfs.display()))?; + } } // 3. Snapshot. snapshot_cmd boots the parent VM, warms it up, @@ -2222,17 +2378,28 @@ fn run_cmd( // Resolve the size ONCE so the cache key and the builder always agree. // (Previously the key used the 24576 default while the builder hard-coded // 1536, caching an undersized rootfs under a full-size key.) + // + // Review #295 r6 blocker 1: validate the cached rootfs before trusting + // it as the immutable baseline (schema version + sha256 match). std::fs::create_dir_all(&cache).ok(); let size_mib = DEFAULT_ROOTFS_SIZE_MIB; let rootfs = cache.join(rootfs_cache_key(&image, size_mib, &extra)); - if !rootfs.exists() { - eprintln!( - "==> building rootfs for {image} (cached at {})", - rootfs.display() - ); - parent_build_cmd(image.clone(), Some(rootfs.clone()), size_mib, extra)?; - } else { - eprintln!("==> using cached rootfs {}", rootfs.display()); + match validate_cached_rootfs(&rootfs) { + Ok(()) => eprintln!("==> using cached rootfs {} (validated)", rootfs.display()), + Err(e) => { + if rootfs.exists() { + eprintln!("==> cache invalidated, rebuilding: {e}"); + } else { + eprintln!( + "==> building rootfs for {image} (cached at {})", + rootfs.display() + ); + } + parent_build_cmd(image.clone(), Some(rootfs.clone()), size_mib, extra.clone())?; + write_rootfs_cache_meta(&rootfs, &image, size_mib) + .with_context(|| format!("write cache meta for {}", rootfs.display()))?; + } + } } // 2. Snapshot a one-off tag. @@ -2497,26 +2664,66 @@ fn snapshot_cmd( .and_then(|s| s.to_str()) .is_some_and(|s| s == "ext4"); - // Immutable-baseline rootfs cloning (issue #296): + // Immutable-baseline rootfs cloning (issue #296) + atomic staging + // (review #295 r5/r6): the entire new snapshot — rootfs clone, + // vmstate, memory.bin, snapshot.json — is built under a STAGING + // directory and only published (renamed into place) once boot, + // warmup, snapshot, and metadata write all succeed. A failure at + // any of those steps drops the staging dir and leaves any + // previously-published snapshot for this tag untouched, so + // re-running a tag can no longer destroy the last usable snapshot. // // For ext4 (read-write) rootfs, the original file is the immutable - // baseline — it is NEVER mounted read-write. Before booting, we - // create a reflink copy (instant on btrfs/xfs via FICLONE, falls - // back to full copy on other filesystems) in the snapshot directory. - // The VM boots from the clone and writes to it; the baseline stays - // clean. After the VM is killed, the clone persists as the - // snapshot's rootfs (needed for restores — Firecracker re-opens the - // rootfs from the path stored in the vmstate). + // baseline — it is NEVER mounted read-write. We reflink-copy it + // (instant on btrfs/xfs via FICLONE, falls back to full copy on + // other filesystems) into the staging dir. The VM boots from the + // clone and writes to it; the baseline stays clean. After publish, + // the clone persists as the snapshot's rootfs (needed for restores + // — Firecracker re-opens the rootfs from the path stored in the + // vmstate). // // This eliminates the dirty-journal corruption that e2fsck-on-boot // was working around: since the baseline is never written to, it // never has uncommitted journal transactions or dirty metadata. let snap_dir = snapshot_dir(&tag); + // Distinct staging dir beside the target. The pid suffix keeps + // concurrent runs from colliding; the `staging-` prefix keeps it + // out of SNAPSHOT_FILES / list_local enumeration. + let staging_dir = snap_dir.with_file_name(format!("{tag}.staging-{}", std::process::id())); + // If a stale staging dir from a previous crashed run exists, drop + // it — its contents were never published and are untrusted. + if staging_dir.exists() { + std::fs::remove_dir_all(&staging_dir) + .with_context(|| format!("remove stale staging dir {}", staging_dir.display()))?; + } + + // src == dst guard: cloning the baseline into itself would both + // produce a corrupt "clone" and, after publish, let a re-run + // unlink its own source. Compare canonical paths (resolves + // symlinks, `..`, relative-vs-absolute). + let rootfs_canon = rootfs.canonicalize().unwrap_or_else(|_| rootfs.clone()); + let staging_rootfs_canon = staging_dir.join("rootfs.ext4"); + // (staging_dir doesn't exist yet, so canonicalize the parent + + // filename instead of the full path.) + let staging_rootfs_canon = staging_dir + .parent() + .map(|p| p.canonicalize().unwrap_or_else(|_| p.to_path_buf())) + .map(|p| p.join("rootfs.ext4")) + .unwrap_or(staging_rootfs_canon); + if rw && rootfs_canon == staging_rootfs_canon { + bail!( + "refusing to clone rootfs into itself: source {} resolves to {}, \ + which is the snapshot's own rootfs.ext4 path. Use a different \ + --rootfs (the baseline must be distinct from the snapshot).", + rootfs.display(), + rootfs_canon.display() + ); + } + let boot_rootfs = if rw { - std::fs::create_dir_all(&snap_dir).context("create snapshot dir for rootfs clone")?; - let clone_path = snap_dir.join("rootfs.ext4"); - // Remove any stale clone from a previous (failed) run. - let _ = std::fs::remove_file(&clone_path); + std::fs::create_dir_all(&staging_dir) + .with_context(|| format!("create staging dir {}", staging_dir.display()))?; + let clone_path = staging_dir.join("rootfs.ext4"); eprintln!(" rootfs mode: read-write (ext4, immutable baseline clone)"); eprintln!( " cloning rootfs {} → {} (reflink preferred)...", @@ -2584,33 +2791,70 @@ fn snapshot_cmd( eprintln!("==> pausing..."); vm.pause().context("pause parent")?; - // snap_dir was created earlier for the rootfs clone. - let vmstate = snap_dir.join("vmstate"); - let memory = snap_dir.join("memory.bin"); + // Snapshot writes into the STAGING dir, not the published snap_dir. + // A failure here drops staging and leaves any existing snapshot + // for this tag intact (review #295 r6: "stage under a distinct + // temporary path, reject or safely handle src == dst, and atomically + // publish only after success"). + // + // For read-only (squashfs) rootfs there's no clone to stage, so we + // create the staging dir now to hold vmstate + memory.bin. + if !staging_dir.exists() { + std::fs::create_dir_all(&staging_dir) + .with_context(|| format!("create staging dir {}", staging_dir.display()))?; + } + let vmstate = staging_dir.join("vmstate"); + let memory = staging_dir.join("memory.bin"); - eprintln!("==> snapshotting to {}...", snap_dir.display()); + eprintln!("==> snapshotting to {} (staging)...", staging_dir.display()); let t = Instant::now(); let mut snap = vm .snapshot_to(vmstate, memory, volumes) .context("snapshot create")?; // Record the rootfs path Firecracker froze into the vmstate so - // `pack` / `pull` can ship + relocate it (issue #242). Canonicalize - // to the absolute path FC actually reopens at restore. - snap.rootfs = Some( - cfg.rootfs - .canonicalize() - .unwrap_or_else(|_| cfg.rootfs.clone()), - ); + // `pack` / `pull` can ship + relocate it (issue #242). For the RW + // clone this was the STAGING rootfs.ext4 path; after publish the + // clone lives at snap_dir/rootfs.ext4, so re-point the recorded + // path at the final location (canonicalized) — pull placement + // must not depend on the transient staging path. + if rw { + snap.rootfs = Some( + snap_dir + .join("rootfs.ext4") + .canonicalize() + .unwrap_or_else(|_| snap_dir.join("rootfs.ext4")), + ); + } else { + snap.rootfs = Some( + cfg.rootfs + .canonicalize() + .unwrap_or_else(|_| cfg.rootfs.clone()), + ); + } eprintln!(" snapshot took {} ms", t.elapsed().as_millis()); // Persist Snapshot metadata so subsequent `forkd fork` / `forkd run` // invocations recover the volume list (the vmstate file alone - // doesn't carry our VolumeSpec annotations). + // doesn't carry our VolumeSpec annotations). Write into staging. let meta = serde_json::to_vec_pretty(&snap).context("serialize snapshot meta")?; - std::fs::write(snap_dir.join("snapshot.json"), meta).context("write snapshot.json")?; + std::fs::write(staging_dir.join("snapshot.json"), meta).context("write snapshot.json")?; + // Kill the parent BEFORE publishing so the rootfs clone is no + // longer being held open by a live Firecracker when we rename it. vm.kill().context("kill parent")?; + // Atomically publish: replace the published snap_dir with the + // staging dir. On Linux, rename over an existing directory is NOT + // atomic, so we do the two-step shuffle: move the old snap_dir + // aside, rename staging into place, then drop the old one. If the + // rename-into-place succeeds we're committed; if it fails we restore + // the old snap_dir from the aside copy. The aside dir is cleaned up + // last, so a crash at any point leaves either the new or the old + // snapshot intact, never neither. + publish_snapshot(&staging_dir, &snap_dir) + .with_context(|| format!("publish staging {} → snap_dir {}", staging_dir.display(), snap_dir.display()))?; + eprintln!(" published snapshot → {}", snap_dir.display()); + // Parent VM is dead and the snapshot lives under data_dir; work_dir // (Firecracker API socket + console log) is now scratch. if keep_workdir { @@ -2652,6 +2896,90 @@ fn cleanup_workdir(work_dir: &std::path::Path) { } } +/// Atomically publish a staged snapshot directory into `snap_dir` +/// (review #295 r6: "stage under a distinct temporary path … and +/// atomically publish only after success"). +/// +/// `std::fs::rename` over an existing NON-empty directory fails with +/// `ENOTEMPTY` on Linux, so this does a safe two-step shuffle: +/// +/// 1. If `snap_dir` exists, move it aside to `snap_dir.old-`. +/// 2. Rename `staging` → `snap_dir` (the commit point). +/// 3. Drop the aside dir. (best-effort; logged on failure) +/// +/// If step 2 succeeds we are committed. If step 2 fails, restore the +/// old `snap_dir` from the aside dir before returning the error, so a +/// crash or failure leaves either the new OR the old snapshot intact, +/// never neither. The aside dir sits beside `snap_dir` under the same +/// data dir, so a crash between step 2 and step 3 leaves a stray +/// `.old-` dir that a later run can clean up (it is never mistaken +/// for a snapshot because `list_local` only enumerates valid snapshot +/// dirs, and the `.old-` prefix keeps it out of tag-based lookups). +fn publish_snapshot(staging: &std::path::Path, snap_dir: &std::path::Path) -> Result<()> { + let parent = snap_dir + .parent() + .with_context(|| format!("snap_dir {} has no parent", snap_dir.display()))?; + // Ensure the parent exists so the aside + rename have a home. + std::fs::create_dir_all(parent) + .with_context(|| format!("create snapshot parent {}", parent.display()))?; + + let aside = parent.join(format!( + "{}.old-{}", + snap_dir + .file_name() + .and_then(|s| s.to_str()) + .unwrap_or("snapshot"), + std::process::id() + )); + // A stale aside from a previous crashed publish should not exist + // (the pid is unique per run), but if it does, drop it — it was + // never the committed snapshot. + if aside.exists() { + let _ = std::fs::remove_dir_all(&aside); + } + + let had_old = snap_dir.exists(); + if had_old { + std::fs::rename(snap_dir, &aside) + .with_context(|| format!("move old snap_dir {} aside → {}", snap_dir.display(), aside.display()))?; + } + + // Commit point: rename staging → snap_dir. + if let Err(e) = std::fs::rename(staging, snap_dir) { + // Restore the old snapshot from the aside so the tag isn't left + // with nothing. If the restore also fails, surface both errors. + if had_old { + if let Err(restore_err) = std::fs::rename(&aside, snap_dir) { + return Err(anyhow::anyhow!( + "publish failed ({e}) AND restoring the old snapshot \ + from {} failed ({restore_err}); the old snapshot is \ + still at {}", + aside.display(), + aside.display() + )); + } + } + return Err(e).with_context(|| format!( + "rename staging {} → snap_dir {}", + staging.display(), + snap_dir.display() + )); + } + + // Committed. Drop the old snapshot (best-effort). + if had_old { + if let Err(e) = std::fs::remove_dir_all(&aside) { + eprintln!( + " note: published {} but could not remove old snapshot {}: {e}\n \ + it is harmless and can be removed manually", + snap_dir.display(), + aside.display() + ); + } + } + Ok(()) +} + /// Load a `Snapshot` from `/snapshot.json` if it exists, /// otherwise fall back to constructing one from `vmstate` + `memory.bin` /// with no volumes (backward compat for snapshots created before this @@ -3568,4 +3896,186 @@ mod tests { let a2 = rootfs_cache_key("foo/bar:1", 1536, &[]); assert_eq!(a, a2); } + + // ---------------------------------------------------------------- + // Review #295 r6 regression tests — cache versioning, atomic + // staging, src==dst, pack/unpack portability. + // ---------------------------------------------------------------- + + /// Helper: write a small temp file with deterministic contents. + fn write_temp_file(dir: &std::path::Path, name: &str, contents: &[u8]) -> std::path::PathBuf { + let p = dir.join(name); + std::fs::write(&p, contents).unwrap(); + p + } + + /// Review #295 r6 blocker 1: a cached rootfs with NO `.cache-meta.json` + /// sidecar (legacy entry from an older forkd version, or dirtied by a + /// pre-versioning snapshot) must be treated as UNTRUSTED — + /// `validate_cached_rootfs` returns Err so the caller rebuilds rather + /// than cloning a possibly-dirty file as the immutable baseline. + #[test] + fn validate_cached_rootfs_rejects_legacy_entry_without_meta() { + let dir = tempfile::tempdir().unwrap(); + // Legacy cache entry: rootfs exists but no .cache-meta.json sidecar. + let rootfs = write_temp_file(dir.path(), "py.ext4", b"fake rootfs contents"); + let err = validate_cached_rootfs(&rootfs).unwrap_err(); + let msg = format!("{err:#}"); + assert!( + msg.contains("no .cache-meta.json sidecar"), + "legacy entry should be rejected as untrusted, got: {msg}" + ); + } + + /// Review #295 r6 blocker 1: a cached rootfs whose meta records a + /// DIFFERENT schema version than the current must be rejected (cache + /// migration). This is the upgrade/dirty-cache guard: a rootfs written + /// by an older forkd with a different on-disk format is not trusted + /// by a newer forkd. + #[test] + fn validate_cached_rootfs_rejects_wrong_schema_version() { + let dir = tempfile::tempdir().unwrap(); + let rootfs = write_temp_file(dir.path(), "py.ext4", b"fake rootfs contents"); + // Write a meta with a future/different schema version. + let meta = serde_json::json!({ + "schema_version": 9999, + "sha256": hub::sha256_file(&rootfs).unwrap(), + "image": "python:3.12-slim".to_string(), + "size_mib": 512u32, + "forkd_version": "0.0.0-old".to_string(), + }); + std::fs::write( + rootfs_cache_meta_path(&rootfs), + serde_json::to_vec_pretty(&meta).unwrap(), + ) + .unwrap(); + let err = validate_cached_rootfs(&rootfs).unwrap_err(); + let msg = format!("{err:#}"); + assert!( + msg.contains("schema version"), + "wrong schema version should be rejected, got: {msg}" + ); + } + + /// Review #295 r6 blocker 1: a cached rootfs whose on-disk sha256 no + /// longer matches the meta (truncation, partial write, or mutation by + /// a prior snapshot that mounted the baseline RW) must be rejected — + /// the integrity guarantee the immutable-baseline clone relies on. + #[test] + fn validate_cached_rootfs_rejects_sha_mismatch_after_mutation() { + let dir = tempfile::tempdir().unwrap(); + let rootfs = write_temp_file(dir.path(), "py.ext4", b"original contents"); + write_rootfs_cache_meta(&rootfs, "python:3.12-slim", 512).unwrap(); + // Mutate the rootfs AFTER the meta was written → sha mismatch. + std::fs::write(&rootfs, b"mutated contents").unwrap(); + let err = validate_cached_rootfs(&rootfs).unwrap_err(); + let msg = format!("{err:#}"); + assert!( + msg.contains("sha256 mismatch"), + "mutated rootfs should be rejected, got: {msg}" + ); + } + + /// Review #295 r6 blocker 1: a freshly-built rootfs with a matching + /// meta (current schema version + matching sha256) is trusted. + #[test] + fn validate_cached_rootfs_accepts_fresh_valid_entry() { + let dir = tempfile::tempdir().unwrap(); + let rootfs = write_temp_file(dir.path(), "py.ext4", b"fresh rootfs contents"); + write_rootfs_cache_meta(&rootfs, "python:3.12-slim", 512).unwrap(); + validate_cached_rootfs(&rootfs) + .expect("fresh rootfs with matching meta should be trusted"); + } + + /// Review #295 r6 blocker 2: a missing rootfs file is a cache miss, + /// not an untrusted entry — `validate_cached_rootfs` returns Err so + /// the caller rebuilds. + #[test] + fn validate_cached_rootfs_misses_on_missing_file() { + let dir = tempfile::tempdir().unwrap(); + let rootfs = dir.path().join("does-not-exist.ext4"); + let err = validate_cached_rootfs(&rootfs).unwrap_err(); + let msg = format!("{err:#}"); + assert!( + msg.contains("cache miss"), + "missing rootfs should be a cache miss, got: {msg}" + ); + } + + /// Review #295 r6 blocker 2 + src==dst: `publish_snapshot` atomically + /// publishes a staging dir into the target snap_dir, replacing an + /// existing snapshot. The OLD snapshot must be dropped only after + /// the new one is committed, so a crash at any point leaves either + /// the new OR the old — never neither. + #[test] + fn publish_snapshot_atomically_replaces_existing() { + let dir = tempfile::tempdir().unwrap(); + let snap_dir = dir.path().join("py"); + // Existing (old) snapshot. + std::fs::create_dir_all(&snap_dir).unwrap(); + std::fs::write(snap_dir.join("snapshot.json"), b"OLD").unwrap(); + std::fs::write(snap_dir.join("rootfs.ext4"), b"OLD-ROOTFS").unwrap(); + // Staging (new) snapshot. + let staging = dir.path().join("py.staging-123"); + std::fs::create_dir_all(&staging).unwrap(); + std::fs::write(staging.join("snapshot.json"), b"NEW").unwrap(); + std::fs::write(staging.join("rootfs.ext4"), b"NEW-ROOTFS").unwrap(); + + publish_snapshot(&staging, &snap_dir).expect("publish should succeed"); + + // New snapshot is committed at snap_dir. + assert_eq!(std::fs::read(snap_dir.join("snapshot.json")).unwrap(), b"NEW"); + assert_eq!(std::fs::read(snap_dir.join("rootfs.ext4")).unwrap(), b"NEW-ROOTFS"); + // Staging dir is gone (renamed away). + assert!(!staging.exists(), "staging dir should be gone after publish"); + // No stray aside dir left behind. + assert_eq!( + std::fs::read_dir(dir.path()).unwrap().count(), + 1, + "only snap_dir should remain; no stray .old- dir" + ); + } + + /// Review #295 r6 blocker 2: `publish_snapshot` into a NON-existent + /// snap_dir (first snapshot for this tag) works without the + /// aside-shuffle — just a rename into place. + #[test] + fn publish_snapshot_into_nonexistent_snap_dir() { + let dir = tempfile::tempdir().unwrap(); + let snap_dir = dir.path().join("fresh-tag"); + let staging = dir.path().join("fresh-tag.staging-1"); + std::fs::create_dir_all(&staging).unwrap(); + std::fs::write(staging.join("snapshot.json"), b"NEW").unwrap(); + + publish_snapshot(&staging, &snap_dir).expect("publish to fresh dir should succeed"); + + assert!(snap_dir.exists()); + assert_eq!(std::fs::read(snap_dir.join("snapshot.json")).unwrap(), b"NEW"); + assert!(!staging.exists()); + } + + /// Review #295 r6 blocker 2 (same-tag failure recovery): if + /// `publish_snapshot` is given a staging dir that doesn't exist (a + /// failed build), it errors WITHOUT touching the existing snapshot — + /// the last usable snapshot survives the failed re-run. This is the + /// same-tag failure regression: re-running a tag can no longer + /// destroy the last usable snapshot. + #[test] + fn publish_snapshot_preserves_existing_when_staging_missing() { + let dir = tempfile::tempdir().unwrap(); + let snap_dir = dir.path().join("py"); + std::fs::create_dir_all(&snap_dir).unwrap(); + std::fs::write(snap_dir.join("snapshot.json"), b"OLD").unwrap(); + std::fs::write(snap_dir.join("rootfs.ext4"), b"OLD-ROOTFS").unwrap(); + // Staging dir does NOT exist (build failed before staging). + let staging = dir.path().join("py.staging-999"); + + let err = publish_snapshot(&staging, &snap_dir).unwrap_err(); + let msg = format!("{err:#}"); + // The existing snapshot MUST be intact. + assert_eq!(std::fs::read(snap_dir.join("snapshot.json")).unwrap(), b"OLD"); + assert_eq!(std::fs::read(snap_dir.join("rootfs.ext4")).unwrap(), b"OLD-ROOTFS"); + assert!(msg.contains("staging") || msg.contains("rename"), + "should error on missing staging, got: {msg}"); + } } From 441a3b29d77db4015d272a7aae2045b2cabd5451 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Thu, 20 Aug 2026 17:29:50 -0700 Subject: [PATCH 3/6] style: cargo fmt + fix stray brace in run_cmd cache validation Rebase resolution left a stray closing brace after the validate_cached_rootfs match block in run_cmd (the original if/else's trailing brace was not removed). cargo fmt the rebased diff. Signed-off-by: jrimmer --- crates/forkd-cli/src/hub.rs | 2 +- crates/forkd-cli/src/main.rs | 78 ++++++++++++++++++++++++++---------- 2 files changed, 57 insertions(+), 23 deletions(-) diff --git a/crates/forkd-cli/src/hub.rs b/crates/forkd-cli/src/hub.rs index 32ffed7..ff91393 100644 --- a/crates/forkd-cli/src/hub.rs +++ b/crates/forkd-cli/src/hub.rs @@ -249,7 +249,7 @@ pub fn pack( let rootfs_filename: Option = rootfs .as_ref() .and_then(|r| Path::new(&r.target_path).file_name()) - .map(|s| s.to_string_lossy().into_owned());; + .map(|s| s.to_string_lossy().into_owned()); let manifest = Manifest { forkd_pack_version: PACK_FORMAT_VERSION_V1, diff --git a/crates/forkd-cli/src/main.rs b/crates/forkd-cli/src/main.rs index a773bdf..1b7da58 100644 --- a/crates/forkd-cli/src/main.rs +++ b/crates/forkd-cli/src/main.rs @@ -1932,8 +1932,12 @@ fn write_rootfs_cache_meta(rootfs: &std::path::Path, image: &str, size_mib: u32) forkd_version: env!("CARGO_PKG_VERSION").to_string(), }; let json = serde_json::to_vec_pretty(&meta).context("serialize rootfs cache meta")?; - std::fs::write(rootfs_cache_meta_path(rootfs), json) - .with_context(|| format!("write rootfs cache meta {}", rootfs_cache_meta_path(rootfs).display())) + std::fs::write(rootfs_cache_meta_path(rootfs), json).with_context(|| { + format!( + "write rootfs cache meta {}", + rootfs_cache_meta_path(rootfs).display() + ) + }) } /// Decide whether a cached rootfs is safe to reuse as the immutable @@ -2400,7 +2404,6 @@ fn run_cmd( .with_context(|| format!("write cache meta for {}", rootfs.display()))?; } } - } // 2. Snapshot a one-off tag. let slug: String = image @@ -2851,8 +2854,13 @@ fn snapshot_cmd( // the old snap_dir from the aside copy. The aside dir is cleaned up // last, so a crash at any point leaves either the new or the old // snapshot intact, never neither. - publish_snapshot(&staging_dir, &snap_dir) - .with_context(|| format!("publish staging {} → snap_dir {}", staging_dir.display(), snap_dir.display()))?; + publish_snapshot(&staging_dir, &snap_dir).with_context(|| { + format!( + "publish staging {} → snap_dir {}", + staging_dir.display(), + snap_dir.display() + ) + })?; eprintln!(" published snapshot → {}", snap_dir.display()); // Parent VM is dead and the snapshot lives under data_dir; work_dir @@ -2940,8 +2948,13 @@ fn publish_snapshot(staging: &std::path::Path, snap_dir: &std::path::Path) -> Re let had_old = snap_dir.exists(); if had_old { - std::fs::rename(snap_dir, &aside) - .with_context(|| format!("move old snap_dir {} aside → {}", snap_dir.display(), aside.display()))?; + std::fs::rename(snap_dir, &aside).with_context(|| { + format!( + "move old snap_dir {} aside → {}", + snap_dir.display(), + aside.display() + ) + })?; } // Commit point: rename staging → snap_dir. @@ -2959,11 +2972,13 @@ fn publish_snapshot(staging: &std::path::Path, snap_dir: &std::path::Path) -> Re )); } } - return Err(e).with_context(|| format!( - "rename staging {} → snap_dir {}", - staging.display(), - snap_dir.display() - )); + return Err(e).with_context(|| { + format!( + "rename staging {} → snap_dir {}", + staging.display(), + snap_dir.display() + ) + }); } // Committed. Drop the old snapshot (best-effort). @@ -3983,8 +3998,7 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let rootfs = write_temp_file(dir.path(), "py.ext4", b"fresh rootfs contents"); write_rootfs_cache_meta(&rootfs, "python:3.12-slim", 512).unwrap(); - validate_cached_rootfs(&rootfs) - .expect("fresh rootfs with matching meta should be trusted"); + validate_cached_rootfs(&rootfs).expect("fresh rootfs with matching meta should be trusted"); } /// Review #295 r6 blocker 2: a missing rootfs file is a cache miss, @@ -4024,10 +4038,19 @@ mod tests { publish_snapshot(&staging, &snap_dir).expect("publish should succeed"); // New snapshot is committed at snap_dir. - assert_eq!(std::fs::read(snap_dir.join("snapshot.json")).unwrap(), b"NEW"); - assert_eq!(std::fs::read(snap_dir.join("rootfs.ext4")).unwrap(), b"NEW-ROOTFS"); + assert_eq!( + std::fs::read(snap_dir.join("snapshot.json")).unwrap(), + b"NEW" + ); + assert_eq!( + std::fs::read(snap_dir.join("rootfs.ext4")).unwrap(), + b"NEW-ROOTFS" + ); // Staging dir is gone (renamed away). - assert!(!staging.exists(), "staging dir should be gone after publish"); + assert!( + !staging.exists(), + "staging dir should be gone after publish" + ); // No stray aside dir left behind. assert_eq!( std::fs::read_dir(dir.path()).unwrap().count(), @@ -4050,7 +4073,10 @@ mod tests { publish_snapshot(&staging, &snap_dir).expect("publish to fresh dir should succeed"); assert!(snap_dir.exists()); - assert_eq!(std::fs::read(snap_dir.join("snapshot.json")).unwrap(), b"NEW"); + assert_eq!( + std::fs::read(snap_dir.join("snapshot.json")).unwrap(), + b"NEW" + ); assert!(!staging.exists()); } @@ -4073,9 +4099,17 @@ mod tests { let err = publish_snapshot(&staging, &snap_dir).unwrap_err(); let msg = format!("{err:#}"); // The existing snapshot MUST be intact. - assert_eq!(std::fs::read(snap_dir.join("snapshot.json")).unwrap(), b"OLD"); - assert_eq!(std::fs::read(snap_dir.join("rootfs.ext4")).unwrap(), b"OLD-ROOTFS"); - assert!(msg.contains("staging") || msg.contains("rename"), - "should error on missing staging, got: {msg}"); + assert_eq!( + std::fs::read(snap_dir.join("snapshot.json")).unwrap(), + b"OLD" + ); + assert_eq!( + std::fs::read(snap_dir.join("rootfs.ext4")).unwrap(), + b"OLD-ROOTFS" + ); + assert!( + msg.contains("staging") || msg.contains("rename"), + "should error on missing staging, got: {msg}" + ); } } From 63ae226bdafd8cd248fe1eecf65952aa76fbd7a9 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Thu, 20 Aug 2026 17:34:22 -0700 Subject: [PATCH 4/6] fix(cli): qualify sha256_file calls with hub:: prefix write_rootfs_cache_meta and validate_cached_rootfs called sha256_file without the hub:: prefix; the function lives in forkd-cli::hub, not main.rs. This was a compile error (E0425 cannot find function sha256_file) on Linux clippy CI. Signed-off-by: jrimmer --- crates/forkd-cli/src/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/forkd-cli/src/main.rs b/crates/forkd-cli/src/main.rs index 1b7da58..78aa376 100644 --- a/crates/forkd-cli/src/main.rs +++ b/crates/forkd-cli/src/main.rs @@ -1923,7 +1923,7 @@ fn rootfs_cache_meta_path(rootfs: &std::path::Path) -> std::path::PathBuf { /// sha256 of the finished file so subsequent cache hits can verify /// integrity without re-hashing on every spawn. fn write_rootfs_cache_meta(rootfs: &std::path::Path, image: &str, size_mib: u32) -> Result<()> { - let sha = sha256_file(rootfs)?; + let sha = hub::sha256_file(rootfs)?; let meta = RootfsCacheMeta { schema_version: ROOTFS_CACHE_SCHEMA_VERSION, sha256: sha, @@ -1984,7 +1984,7 @@ fn validate_cached_rootfs(rootfs: &std::path::Path) -> Result<()> { ROOTFS_CACHE_SCHEMA_VERSION ); } - let live_sha = sha256_file(rootfs)?; + let live_sha = hub::sha256_file(rootfs)?; if live_sha != meta.sha256 { bail!( "cache untrusted: rootfs {} sha256 mismatch (meta={}, live={}); \ From 351507c0fd4dc85a8793b19fd8a4100b21fd61ba Mon Sep 17 00:00:00 2001 From: jrimmer Date: Thu, 20 Aug 2026 17:41:30 -0700 Subject: [PATCH 5/6] fix(cli): clippy redundant closure in destination fallback unwrap_or_else(|| std::path::PathBuf::new()) -> unwrap_or_else(std::path::PathBuf::new) Signed-off-by: jrimmer --- crates/forkd-cli/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/forkd-cli/src/main.rs b/crates/forkd-cli/src/main.rs index 78aa376..441c904 100644 --- a/crates/forkd-cli/src/main.rs +++ b/crates/forkd-cli/src/main.rs @@ -1518,7 +1518,7 @@ fn unpack_chain_into( Ok(destinations .last() .cloned() - .unwrap_or_else(|| std::path::PathBuf::new())) + .unwrap_or_else(std::path::PathBuf::new)) } /// Where `forkd pull /` resolves names to download URLs by From f4a015102f219c8891358297ca8488719ce04d82 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Sat, 22 Aug 2026 12:51:12 -0700 Subject: [PATCH 6/6] fix(snapshot): 4 blockers from review #295 (2026-08-22) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebased onto dev (d2d238a, incl. #312). Addresses all four review blocks on the staging/transport flow while keeping cache-versioning and failure-rollback. Blocker 1 — FC-visible rootfs path unstable across publish: Firecracker serializes the drive path_on_host INTO the binary vmstate and REOPENS it on restore (no PUT /drives override is accepted before /snapshot/load). Previously snapshot_cmd cloned rootfs to staging_dir/rootfs.ext4 and booted the VM from it, then publish_snapshot renamed the whole dir to snap_dir — leaving the vmstate's recorded path nonexistent, so the first restore after publish failed. Fix: clone+boot from the STABLE final path snap_dir/rootfs.ext4 from the start; only vmstate/memory/snapshot.json are staged. publish_snapshot is replaced by publish_snapshot_metadata, which renames the 3 metadata files into snap_dir (snapshot.json LAST = commit marker) and never moves the rootfs. Tests: metadata-replace-keeps-rootfs, into-nonexistent-snap_dir, preserves-existing-on-missing-metadata. Blocker 2 — pack/unpack sidecar never placable: pack listed rootfs.ext4 in manifest.files but omitted it from the tar body; unpack verified EVERY declared file before satisfy_rootfs, hashing the missing extracted rootfs and failing. Fix: when a portable sidecar is emitted, retain rootfs.ext4 out of manifest.files (sidecar carries sha integrity). Test: pack_unpack_roundtrip_with_sidecar_rootfs. Blocker 3 — src==dst guard compared staging path, not final: extracted to rootfs_clone_into_self(src, snap_dir) comparing canonical against the FINAL snap_dir/rootfs.ext4, so re-snapshotting a tag whose baseline is its own rootfs.ext4 is rejected. Test: same-tag regression. Blocker 4 — path traversal via RootfsRef.target_path: satisfy_rootfs now REQUIRES a safe single-component relative filename (no absolute / no separators / no ..), rejecting malicious ../../../ or legacy absolute paths (fail-closed; writes would otherwise land outside snap_dir under sudo). Test: satisfy_rootfs_rejects_unsafe_target_paths. Signed-off-by: jrimmer --- crates/forkd-cli/src/hub.rs | 96 +++++- crates/forkd-cli/src/main.rs | 591 ++++++++++++++++++++++++++--------- 2 files changed, 527 insertions(+), 160 deletions(-) diff --git a/crates/forkd-cli/src/hub.rs b/crates/forkd-cli/src/hub.rs index ff91393..52ab00f 100644 --- a/crates/forkd-cli/src/hub.rs +++ b/crates/forkd-cli/src/hub.rs @@ -242,14 +242,20 @@ pub fn pack( // the pack is produced without a portable rootfs and will only // restore on the packing host. let rootfs = emit_rootfs_sidecar(snap_dir, out_path)?; - // When a portable sidecar was emitted, exclude rootfs.ext4 from the - // tar body (it's already counted in `files` for manifest integrity). - // The tar append loop below skips any entry whose path equals the - // rootfs filename when a sidecar is present. + // Review #295 blocker 2 / 2026-08-22: when a portable sidecar was + // emitted, rootfs.ext4 is NOT in the tar body and must NOT be listed + // in `manifest.files` — otherwise unpack's verification (which checks + // every declared file BEFORE satisfy_rootfs runs) would hash the + // missing extracted rootfs and fail before the sidecar could ever be + // placed. The sidecar carries the rootfs integrity itself via + // RootfsRef.sha256. let rootfs_filename: Option = rootfs .as_ref() .and_then(|r| Path::new(&r.target_path).file_name()) .map(|s| s.to_string_lossy().into_owned()); + if let Some(ref rf) = rootfs_filename { + files.retain(|e| e.path != *rf); + } let manifest = Manifest { forkd_pack_version: PACK_FORMAT_VERSION_V1, @@ -288,14 +294,9 @@ pub fn pack( .context("append manifest.toml")?; for entry in &files { - // Review #295 r6 blocker 3: don't tar the rootfs when a portable - // sidecar was emitted — the sidecar is the single transport and - // tar'd rootfs.ext4 would just duplicate a huge image. - if let Some(ref rf) = rootfs_filename { - if entry.path == *rf { - continue; - } - } + // rootfs.ext4 is not in `files` when a portable sidecar was + // emitted (retain above removed it), so it is never tar'd — the + // sidecar is the single transport (review #295). let path = snap_dir.join(&entry.path); let mut f = File::open(&path).with_context(|| format!("open {}", path.display()))?; tar.append_file(&entry.path, &mut f) @@ -1380,6 +1381,77 @@ mod tests { assert_eq!(std::fs::read(dst.join("memory.bin")).unwrap().len(), 4096); } + /// Review #295 blocker 2 / 2026-08-22: a pack with a PORTABLE sidecar + /// must round-trip. Previously `pack` listed rootfs.ext4 in + /// `manifest.files` but omitted it from the tar (sidecar is the single + /// transport); `unpack` then verified every declared file, hashed the + /// missing extracted rootfs.ext4, and failed — so the sidecar could + /// never be placed. The fix removes rootfs.ext4 from `manifest.files` + /// when a sidecar is emitted, so unpack succeeds and the single rootfs + /// transport is the sidecar on disk next to the pack. + #[test] + fn pack_unpack_roundtrip_with_sidecar_rootfs() { + let tmp = tempfile::tempdir().unwrap(); + let src = tmp.path().join("src"); + std::fs::create_dir(&src).unwrap(); + std::fs::write(src.join("vmstate"), b"vmstate-bytes").unwrap(); + std::fs::write(src.join("memory.bin"), vec![0u8; 4096]).unwrap(); + // A real rootfs file the snapshot.json will point at, so + // emit_rootfs_sidecar produces a portable sidecar. + let rootfs = src.join("rootfs.ext4"); + let rootfs_bytes = vec![0xEEu8; 8192]; + std::fs::write(&rootfs, &rootfs_bytes).unwrap(); + let rootfs_sha = sha256_file(&rootfs).unwrap(); + std::fs::write( + src.join("snapshot.json"), + format!( + r#"{{"vmstate":"x","memory":"y","rootfs":"{}","volumes":[]}}"#, + rootfs.display() + ), + ) + .unwrap(); + + let pack_out = tmp.path().join("out.tar.zst"); + let m = pack("test/with-rootfs", None, None, &src, &pack_out).expect("pack"); + // A portable sidecar was emitted and rootfs.ext4 is NOT in the + // manifest files list (it lives only in the sidecar). + assert!( + m.rootfs.is_some(), + "snapshot recording a rootfs should emit a portable sidecar" + ); + let rootfs_ref = m.rootfs.clone().unwrap(); + assert_eq!(rootfs_ref.sha256, rootfs_sha); + assert!( + !m.files.iter().any(|f| f.path == "rootfs.ext4"), + "rootfs.ext4 must NOT be in manifest.files when a sidecar is emitted" + ); + // The single rootfs transport exists on disk next to the pack. + let sidecar_name = rootfs_sidecar_name(&rootfs_sha); + assert!( + pack_out.parent().unwrap().join(&sidecar_name).exists(), + "sidecar {:?} should exist next to the pack", + sidecar_name + ); + + // Unpack MUST succeed now (previously it failed verifying the + // missing extracted rootfs.ext4). + let dst = tmp.path().join("dst"); + let m2 = unpack(&pack_out, &dst).expect("unpack with sidecar should succeed"); + assert_eq!(m2.rootfs.as_ref().unwrap().sha256, rootfs_sha); + // Non-rootfs files were extracted and verified. + assert_eq!( + std::fs::read(dst.join("vmstate")).unwrap(), + b"vmstate-bytes" + ); + // The rootfs itself is NOT extracted into the snapshot dir by + // unpack (it stays in the sidecar; satisfy_rootfs places it on + // restore). + assert!( + !dst.join("rootfs.ext4").exists(), + "rootfs should be transported only via the sidecar, not the tar" + ); + } + // Path-traversal rejection is intentionally not unit-tested here: // the `tar` crate's `Builder::append_data()` refuses to *write* an // entry with `..` segments, so we can't craft a malicious archive diff --git a/crates/forkd-cli/src/main.rs b/crates/forkd-cli/src/main.rs index 441c904..5bb06b3 100644 --- a/crates/forkd-cli/src/main.rs +++ b/crates/forkd-cli/src/main.rs @@ -780,6 +780,29 @@ fn validate_tag(tag: &str) -> Result<()> { Ok(()) } +/// Review #295 blocker 3 / 2026-08-22: does `snap_dir`'s own +/// `rootfs.ext4` (the path the RW clone will land at) resolve to the SAME +/// file as the source baseline `src`? If so, cloning would write over the +/// very file we're reading from — a corrupt self-clone. +/// +/// The comparison is against the FINAL published path (`snap_dir/rootfs.ext4`), +/// NOT a transient staging path. Comparing against staging would let the +/// existing tag's own `snap_dir/rootfs.ext4` slip past the guard, then +/// publication would delete the original baseline and leave a dirty clone in +/// its place. Canonical compare resolves symlinks / `.` / `..` / relative-vs-abs. +fn rootfs_clone_into_self(src: &std::path::Path, snap_dir: &std::path::Path) -> bool { + let src_canon = src.canonicalize().unwrap_or_else(|_| src.to_path_buf()); + let final_path = snap_dir.join("rootfs.ext4"); + // snap_dir may not exist yet, so canonicalize the parent + filename + // instead of the full path. + let final_canon = final_path + .parent() + .map(|p| p.canonicalize().unwrap_or_else(|_| p.to_path_buf())) + .map(|p| p.join("rootfs.ext4")) + .unwrap_or(final_path); + src_canon == final_canon +} + fn main() -> Result<()> { tracing_subscriber::fmt::init(); let cli = Cli::parse(); @@ -1261,22 +1284,44 @@ enum SidecarSource<'a> { /// unpacked into. `rootfs.target_path` is a PORTABLE, host-independent /// path relative to `snap_dir` (review #295 r6 blocker 3 — e.g. /// `"rootfs.ext4"`); it is resolved against `snap_dir` to get the -/// absolute path Firecracker reopens. If `target_path` is absolute -/// (legacy packs written before portability), it is used as-is. +/// absolute path Firecracker reopens. A target_path that is absolute +/// or contains `..` is REJECTED (fail-closed) — see the safety +/// validation in the body (review #295 blocker 4 / 2026-08-22). fn satisfy_rootfs( rootfs: &hub::RootfsRef, source: SidecarSource, snap_dir: &std::path::Path, ) -> Result<()> { - // Resolve the portable (relative) target_path against the - // destination snapshot dir. Absolute paths (legacy packs) are - // used as-is so old packs still restore. + // Review #295 blocker 4 / 2026-08-22: validate `target_path` BEFORE + // resolving it against snap_dir. A malicious/buggy manifest can + // supply `../../...` (or an absolute path) and sidecar placement + // would otherwise write outside the snapshot directory — dangerous + // because these commands are commonly run via sudo. + // + // Current forkd packs emit a PORTABLE, single-component relative + // filename (e.g. "rootfs.ext4" — see emit_rootfs_sidecar). We + // therefore require target_path to be a safe, bare filename: + // - not absolute, + // - exactly one path component (no `/`, no `\`, no leading `./`), + // - no `.` or `..` components, + // - non-empty. + // A legacy absolute path is refused (fail-closed) rather than honored + // with an unconditional write outside the snapshot dir, per the + // explicit-safe-migration requirement. let target = std::path::Path::new(&rootfs.target_path); - let dst = if target.is_absolute() { - target.to_path_buf() - } else { - snap_dir.join(target) - }; + let is_safe_filename = !target.is_absolute() + && target.components().count() == 1 + && target.file_name().map(|n| !n.is_empty()).unwrap_or(false); + if !is_safe_filename { + return Err(anyhow::anyhow!( + "unsafe rootfs target_path {:?} in manifest (must be a single safe relative \ + filename like \"rootfs.ext4\", no path separators / absolute / \"..\"). \ + Re-pack with a current forkd; legacy absolute target_paths are not honored \ + because they allow writing outside the snapshot directory.", + rootfs.target_path + )); + } + let dst = snap_dir.join(target); if dst.exists() { if let Ok(existing) = hub::sha256_file(&dst) { if existing.eq_ignore_ascii_case(&rootfs.sha256) { @@ -2700,20 +2745,15 @@ fn snapshot_cmd( .with_context(|| format!("remove stale staging dir {}", staging_dir.display()))?; } - // src == dst guard: cloning the baseline into itself would both - // produce a corrupt "clone" and, after publish, let a re-run - // unlink its own source. Compare canonical paths (resolves - // symlinks, `..`, relative-vs-absolute). - let rootfs_canon = rootfs.canonicalize().unwrap_or_else(|_| rootfs.clone()); - let staging_rootfs_canon = staging_dir.join("rootfs.ext4"); - // (staging_dir doesn't exist yet, so canonicalize the parent + - // filename instead of the full path.) - let staging_rootfs_canon = staging_dir - .parent() - .map(|p| p.canonicalize().unwrap_or_else(|_| p.to_path_buf())) - .map(|p| p.join("rootfs.ext4")) - .unwrap_or(staging_rootfs_canon); - if rw && rootfs_canon == staging_rootfs_canon { + // src == dst guard (review #295 blocker 3 / 2026-08-22): cloning the + // src == dst guard (review #295 blocker 3 / 2026-08-22): reject + // cloning the baseline into the snapshot's own final rootfs path. See + // `rootfs_clone_into_self` — canonical compare against the FINAL + // published path, not a transient staging path. + if rw && rootfs_clone_into_self(&rootfs, &snap_dir) { + let rootfs_canon = rootfs + .canonicalize() + .unwrap_or_else(|_| rootfs.to_path_buf()); bail!( "refusing to clone rootfs into itself: source {} resolves to {}, \ which is the snapshot's own rootfs.ext4 path. Use a different \ @@ -2724,9 +2764,32 @@ fn snapshot_cmd( } let boot_rootfs = if rw { - std::fs::create_dir_all(&staging_dir) - .with_context(|| format!("create staging dir {}", staging_dir.display()))?; - let clone_path = staging_dir.join("rootfs.ext4"); + // Blocker 1 (review r8/2026-08-22): the FC-visible rootfs drive + // path is serialized into the binary vmstate at snapshot time and + // is REOPENED by that exact path on restore (Firecracker does not + // accept a PUT /drives override before /snapshot/load). If we + // booted from a transient `staging_dir/rootfs.ext4` and then + // renamed the whole directory, the recorded path would vanish and + // the first restore after publish would fail. Therefore the RW + // clone must live at its FINAL, stable path (`snap_dir/rootfs.ext4`) + // from the moment the VM boots. Only the volatile artifacts + // (vmstate, memory.bin, snapshot.json) are staged and swapped + // atomically at publish. + std::fs::create_dir_all(&snap_dir) + .with_context(|| format!("create snapshot dir {}", snap_dir.display()))?; + let clone_path = snap_dir.join("rootfs.ext4"); + // `reflink_copy` opens the destination with `create_new(true)` + // (EEXIST on an existing file). On a re-snapshot/re-bake of an + // existing tag, snap_dir/rootfs.ext4 already exists from the prior + // bake. The self-clone guard above already ruled out + // `src == snap_dir/rootfs.ext4`, so removing a stale clone here is + // safe (we are replacing this tag's rootfs with a fresh clone of a + // DISTINCT baseline). + if clone_path.exists() { + std::fs::remove_file(&clone_path).with_context(|| { + format!("remove stale clone {} before re-bake", clone_path.display()) + })?; + } eprintln!(" rootfs mode: read-write (ext4, immutable baseline clone)"); eprintln!( " cloning rootfs {} → {} (reflink preferred)...", @@ -2815,11 +2878,13 @@ fn snapshot_cmd( .snapshot_to(vmstate, memory, volumes) .context("snapshot create")?; // Record the rootfs path Firecracker froze into the vmstate so - // `pack` / `pull` can ship + relocate it (issue #242). For the RW - // clone this was the STAGING rootfs.ext4 path; after publish the - // clone lives at snap_dir/rootfs.ext4, so re-point the recorded - // path at the final location (canonicalized) — pull placement - // must not depend on the transient staging path. + // `pack` / `pull` can ship + relocate it (issue #242). With the + // blocker-1 fix the RW clone boots directly from the STABLE final + // path `snap_dir/rootfs.ext4` (see boot_rootfs above) — so the + // vmstate already records the path that persists after publish, and + // `Snap.rootfs` points at the same canonical final location. Nothing + // transient is recorded. (Previously the VM booted from a + // `staging_dir/rootfs.ext4` that vanished on publish.) if rw { snap.rootfs = Some( snap_dir @@ -2843,24 +2908,30 @@ fn snapshot_cmd( std::fs::write(staging_dir.join("snapshot.json"), meta).context("write snapshot.json")?; // Kill the parent BEFORE publishing so the rootfs clone is no - // longer being held open by a live Firecracker when we rename it. + // longer being held open by a live Firecracker when we move the + // metadata files into place. vm.kill().context("kill parent")?; - // Atomically publish: replace the published snap_dir with the - // staging dir. On Linux, rename over an existing directory is NOT - // atomic, so we do the two-step shuffle: move the old snap_dir - // aside, rename staging into place, then drop the old one. If the - // rename-into-place succeeds we're committed; if it fails we restore - // the old snap_dir from the aside copy. The aside dir is cleaned up - // last, so a crash at any point leaves either the new or the old - // snapshot intact, never neither. - publish_snapshot(&staging_dir, &snap_dir).with_context(|| { + // Publish the snapshot metadata. The RW rootfs already lives at the + // stable final path `snap_dir/rootfs.ext4` (blocker-1 fix) so it is + // NOT moved here — only vmstate, memory.bin, and snapshot.json are + // staged and then renamed into snap_dir. + // + // snapshot.json is the restore entry point (`load_snapshot_meta`): + // we install vmstate + memory.bin first and snapshot.json LAST, so a + // crash mid-publish leaves either the old snapshot.json (pointing at + // the old vmstate/memory, still present because we rename over) or + // the new one. `fs::rename` over an existing path is atomic on Linux. + publish_snapshot_metadata(&staging_dir, &snap_dir).with_context(|| { format!( - "publish staging {} → snap_dir {}", + "publish snapshot metadata staging {} → snap_dir {}", staging_dir.display(), snap_dir.display() ) })?; + // Drop the staged-only dir (now empty of the files we renamed, or + // holding only a leftover on a partial failure). + let _ = std::fs::remove_dir_all(&staging_dir); eprintln!(" published snapshot → {}", snap_dir.display()); // Parent VM is dead and the snapshot lives under data_dir; work_dir @@ -2923,74 +2994,53 @@ fn cleanup_workdir(work_dir: &std::path::Path) { /// `.old-` dir that a later run can clean up (it is never mistaken /// for a snapshot because `list_local` only enumerates valid snapshot /// dirs, and the `.old-` prefix keeps it out of tag-based lookups). -fn publish_snapshot(staging: &std::path::Path, snap_dir: &std::path::Path) -> Result<()> { - let parent = snap_dir - .parent() - .with_context(|| format!("snap_dir {} has no parent", snap_dir.display()))?; - // Ensure the parent exists so the aside + rename have a home. - std::fs::create_dir_all(parent) - .with_context(|| format!("create snapshot parent {}", parent.display()))?; - - let aside = parent.join(format!( - "{}.old-{}", - snap_dir - .file_name() - .and_then(|s| s.to_str()) - .unwrap_or("snapshot"), - std::process::id() - )); - // A stale aside from a previous crashed publish should not exist - // (the pid is unique per run), but if it does, drop it — it was - // never the committed snapshot. - if aside.exists() { - let _ = std::fs::remove_dir_all(&aside); - } - - let had_old = snap_dir.exists(); - if had_old { - std::fs::rename(snap_dir, &aside).with_context(|| { - format!( - "move old snap_dir {} aside → {}", - snap_dir.display(), - aside.display() - ) - })?; - } - - // Commit point: rename staging → snap_dir. - if let Err(e) = std::fs::rename(staging, snap_dir) { - // Restore the old snapshot from the aside so the tag isn't left - // with nothing. If the restore also fails, surface both errors. - if had_old { - if let Err(restore_err) = std::fs::rename(&aside, snap_dir) { - return Err(anyhow::anyhow!( - "publish failed ({e}) AND restoring the old snapshot \ - from {} failed ({restore_err}); the old snapshot is \ - still at {}", - aside.display(), - aside.display() - )); - } +/// Publish a snapshot's VOLATILE metadata (vmstate, memory.bin, +/// snapshot.json) from a temporary `staging` dir into the target +/// `snap_dir`, leaving the rootfs file (which lives at the stable +/// `snap_dir/rootfs.ext4` from boot time — blocker-1 fix) untouched. +/// +/// Unlike the previous whole-directory rename, this must NOT move the +/// rootfs: Firecracker serializes the drive `path_on_host` into the +/// binary vmstate and reopens it on restore, so the rootfs clone must +/// stay at the path the snapshot recorded. +/// +/// Ordering: rename `vmstate` and `memory.bin` first, then +/// `snapshot.json` LAST. `snapshot.json` is the restore entry point +/// (`load_snapshot_meta`); a crash mid-publish therefore leaves either +/// the OLD snapshot.json (restore still points at the old vmstate/memory, +/// both still present because `fs::rename` atomically replaces files) or +/// the NEW one — never a snapshot.json pointing at a half-installed set. +/// `fs::rename` over an existing path is atomic on Linux. +fn publish_snapshot_metadata(staging: &std::path::Path, snap_dir: &std::path::Path) -> Result<()> { + // Ensure the target snapshot dir exists so the metadata files have a + // home. The rootfs.ext4 may already be there (RW clone) or absent (RO + // squashfs, never staged/cloned into it). create_dir_all is a no-op + // when it already exists. + std::fs::create_dir_all(snap_dir) + .with_context(|| format!("create snapshot dir {}", snap_dir.display()))?; + + // Each metadata file is atomic-moved into place; snapshot.json last + // is the commit marker. Verify EVERY staged file exists BEFORE the + // first rename so a torn staging dir (missing memory.bin, etc.) is + // rejected without installing a half-written vmstate/memory pair + // (review-correctness follow-up: pre-check all, then atomically move). + const META: [&str; 3] = ["vmstate", "memory.bin", "snapshot.json"]; + for name in META { + let staged = staging.join(name); + if !staged.exists() { + return Err(anyhow::anyhow!( + "publish: staged file {} missing; cannot publish incomplete snapshot", + staged.display() + )); } - return Err(e).with_context(|| { - format!( - "rename staging {} → snap_dir {}", - staging.display(), - snap_dir.display() - ) - }); } - - // Committed. Drop the old snapshot (best-effort). - if had_old { - if let Err(e) = std::fs::remove_dir_all(&aside) { - eprintln!( - " note: published {} but could not remove old snapshot {}: {e}\n \ - it is harmless and can be removed manually", - snap_dir.display(), - aside.display() - ); - } + // All present — now move them in snapshot.json-last order. + for name in META { + let staged = staging.join(name); + let dest = snap_dir.join(name); + std::fs::rename(&staged, &dest).with_context(|| { + format!("publish metadata {} → {}", staged.display(), dest.display()) + })?; } Ok(()) } @@ -4016,89 +4066,121 @@ mod tests { ); } - /// Review #295 r6 blocker 2 + src==dst: `publish_snapshot` atomically - /// publishes a staging dir into the target snap_dir, replacing an - /// existing snapshot. The OLD snapshot must be dropped only after - /// the new one is committed, so a crash at any point leaves either - /// the new OR the old — never neither. + /// Review #295 blocker 1 / 2026-08-22: `publish_snapshot_metadata` + /// installs only the VOLATILE metadata (vmstate, memory.bin, + /// snapshot.json) into snap_dir, leaving the rootfs file at its + /// stable final path untouched. The rootfs must NOT be moved because + /// Firecracker serializes its drive path into the vmstate and reopens + /// it on restore. A re-snapshot replaces metadata while the existing + /// rootfs stays put until the new clone is written at the same path. #[test] - fn publish_snapshot_atomically_replaces_existing() { + fn publish_snapshot_metadata_replaces_metadata_keeps_rootfs() { let dir = tempfile::tempdir().unwrap(); let snap_dir = dir.path().join("py"); - // Existing (old) snapshot. + // Existing (old) snapshot, incl. a stable rootfs at the final path. std::fs::create_dir_all(&snap_dir).unwrap(); std::fs::write(snap_dir.join("snapshot.json"), b"OLD").unwrap(); - std::fs::write(snap_dir.join("rootfs.ext4"), b"OLD-ROOTFS").unwrap(); - // Staging (new) snapshot. + std::fs::write(snap_dir.join("vmstate"), b"OLD-VMSTATE").unwrap(); + std::fs::write(snap_dir.join("memory.bin"), b"OLD-MEM").unwrap(); + std::fs::write(snap_dir.join("rootfs.ext4"), b"STABLE-ROOTFS").unwrap(); + // Staging (new) metadata — note: NO rootfs.ext4 staged. let staging = dir.path().join("py.staging-123"); std::fs::create_dir_all(&staging).unwrap(); std::fs::write(staging.join("snapshot.json"), b"NEW").unwrap(); - std::fs::write(staging.join("rootfs.ext4"), b"NEW-ROOTFS").unwrap(); + std::fs::write(staging.join("vmstate"), b"NEW-VMSTATE").unwrap(); + std::fs::write(staging.join("memory.bin"), b"NEW-MEM").unwrap(); - publish_snapshot(&staging, &snap_dir).expect("publish should succeed"); + publish_snapshot_metadata(&staging, &snap_dir).expect("publish should succeed"); - // New snapshot is committed at snap_dir. + // Metadata replaced. assert_eq!( std::fs::read(snap_dir.join("snapshot.json")).unwrap(), b"NEW" ); + assert_eq!( + std::fs::read(snap_dir.join("vmstate")).unwrap(), + b"NEW-VMSTATE" + ); + assert_eq!( + std::fs::read(snap_dir.join("memory.bin")).unwrap(), + b"NEW-MEM" + ); + // Rootfs untouched (it stays at the stable final path). assert_eq!( std::fs::read(snap_dir.join("rootfs.ext4")).unwrap(), - b"NEW-ROOTFS" + b"STABLE-ROOTFS", + "rootfs must NOT be moved/overwritten by metadata publish" ); - // Staging dir is gone (renamed away). + // The staged metadata files were moved OUT of staging (the + // staging dir itself is removed later by snapshot_cmd). assert!( - !staging.exists(), - "staging dir should be gone after publish" + !staging.join("snapshot.json").exists(), + "snapshot.json should be moved out of staging after publish" ); - // No stray aside dir left behind. - assert_eq!( - std::fs::read_dir(dir.path()).unwrap().count(), - 1, - "only snap_dir should remain; no stray .old- dir" + assert!( + !staging.join("vmstate").exists(), + "vmstate should be moved out of staging after publish" + ); + assert!( + !staging.join("memory.bin").exists(), + "memory.bin should be moved out of staging after publish" ); } - /// Review #295 r6 blocker 2: `publish_snapshot` into a NON-existent - /// snap_dir (first snapshot for this tag) works without the - /// aside-shuffle — just a rename into place. + /// Review #295 blocker 1 / 2026-08-22: `publish_snapshot_metadata` + /// into a NON-existent snap_dir (first snapshot for this tag) + /// creates the dir and installs the metadata. #[test] - fn publish_snapshot_into_nonexistent_snap_dir() { + fn publish_snapshot_metadata_into_nonexistent_snap_dir() { let dir = tempfile::tempdir().unwrap(); let snap_dir = dir.path().join("fresh-tag"); let staging = dir.path().join("fresh-tag.staging-1"); std::fs::create_dir_all(&staging).unwrap(); std::fs::write(staging.join("snapshot.json"), b"NEW").unwrap(); + std::fs::write(staging.join("vmstate"), b"NEW-VMSTATE").unwrap(); + std::fs::write(staging.join("memory.bin"), b"NEW-MEM").unwrap(); - publish_snapshot(&staging, &snap_dir).expect("publish to fresh dir should succeed"); + publish_snapshot_metadata(&staging, &snap_dir) + .expect("publish to fresh dir should succeed"); assert!(snap_dir.exists()); assert_eq!( std::fs::read(snap_dir.join("snapshot.json")).unwrap(), b"NEW" ); - assert!(!staging.exists()); + assert_eq!( + std::fs::read(snap_dir.join("vmstate")).unwrap(), + b"NEW-VMSTATE" + ); } - /// Review #295 r6 blocker 2 (same-tag failure recovery): if - /// `publish_snapshot` is given a staging dir that doesn't exist (a - /// failed build), it errors WITHOUT touching the existing snapshot — - /// the last usable snapshot survives the failed re-run. This is the - /// same-tag failure regression: re-running a tag can no longer - /// destroy the last usable snapshot. + /// Review #295 blocker 1 + same-tag failure recovery: if staging is + /// missing a required metadata file (a failed/incomplete build), the + /// publish errors WITHOUT touching the existing snapshot — the last + /// usable snapshot survives the failed re-run. #[test] - fn publish_snapshot_preserves_existing_when_staging_missing() { + fn publish_snapshot_metadata_preserves_existing_when_metadata_missing() { let dir = tempfile::tempdir().unwrap(); let snap_dir = dir.path().join("py"); std::fs::create_dir_all(&snap_dir).unwrap(); std::fs::write(snap_dir.join("snapshot.json"), b"OLD").unwrap(); + std::fs::write(snap_dir.join("vmstate"), b"OLD-VMSTATE").unwrap(); + std::fs::write(snap_dir.join("memory.bin"), b"OLD-MEM").unwrap(); std::fs::write(snap_dir.join("rootfs.ext4"), b"OLD-ROOTFS").unwrap(); - // Staging dir does NOT exist (build failed before staging). + // Staging exists but is missing memory.bin (incomplete build). + // vmstate + snapshot.json are present so the pre-check must reject + // BEFORE any rename — a torn vmstate/memory pair must never be + // installed. let staging = dir.path().join("py.staging-999"); + std::fs::create_dir_all(&staging).unwrap(); + std::fs::write(staging.join("snapshot.json"), b"NEW").unwrap(); + std::fs::write(staging.join("vmstate"), b"NEW-VMSTATE").unwrap(); - let err = publish_snapshot(&staging, &snap_dir).unwrap_err(); + let err = publish_snapshot_metadata(&staging, &snap_dir).unwrap_err(); let msg = format!("{err:#}"); - // The existing snapshot MUST be intact. + // The existing snapshot MUST be fully intact — including vmstate + // and memory.bin — because the pre-check rejected before any + // rename (no torn pair). assert_eq!( std::fs::read(snap_dir.join("snapshot.json")).unwrap(), b"OLD" @@ -4107,9 +4189,222 @@ mod tests { std::fs::read(snap_dir.join("rootfs.ext4")).unwrap(), b"OLD-ROOTFS" ); + assert_eq!( + std::fs::read(snap_dir.join("vmstate")).unwrap(), + b"OLD-VMSTATE", + "missing metadata must not install a torn vmstate" + ); + assert_eq!( + std::fs::read(snap_dir.join("memory.bin")).unwrap(), + b"OLD-MEM", + "missing metadata must not install a torn memory.bin" + ); assert!( - msg.contains("staging") || msg.contains("rename"), - "should error on missing staging, got: {msg}" + msg.contains("missing") || msg.contains("memory.bin"), + "should error on missing staged metadata, got: {msg}" ); } + + /// Review #295 blocker 4 / 2026-08-22: `satisfy_rootfs` must reject a + /// malicious `RootfsRef.target_path` that would escape the snapshot + /// dir (path traversal / absolute legacy path) BEFORE placing the + /// sidecar — otherwise the write lands outside `snap_dir`, which is + /// dangerous under sudo. Each unsafe shape must error and write + /// nothing. + #[test] + fn satisfy_rootfs_rejects_unsafe_target_paths() { + let tmp = tempfile::tempdir().unwrap(); + let snap_dir = tmp.path().join("snap"); + std::fs::create_dir_all(&snap_dir).unwrap(); + + let unsafe_paths = [ + "../../etc/passwd", // traversal + "../../rootfs.ext4", // escape one level + "/etc/cron.d/evil", // absolute legacy path + "foo/../bar", // embedded .. + "foo\\bar/rootfs.ext4", // separator via backslash + "", // empty + ".", // dot + "..", // parent + ]; + for bad in unsafe_paths { + let rootfs_ref = hub::RootfsRef { + target_path: bad.to_string(), + sha256: "deadbeef".to_string(), + size: 0, + }; + let res = satisfy_rootfs( + &rootfs_ref, + SidecarSource::LocalSibling(&tmp.path().join("pack.tar.zst")), + &snap_dir, + ); + assert!(res.is_err(), "unsafe target_path {bad:?} must be rejected"); + // Nothing written outside the snapshot dir. + assert!( + !tmp.path().join("etc/passwd").exists(), + "traversal must not write to {bad:?}" + ); + } + + // The safe portable filename still works: it resolves inside snap_dir. + std::fs::write(snap_dir.join("rootfs.ext4"), b"rootfs-content").unwrap(); + let good_sha = hub::sha256_file(&snap_dir.join("rootfs.ext4")).unwrap(); + let good = hub::RootfsRef { + target_path: "rootfs.ext4".to_string(), + sha256: good_sha, + size: 0, + }; + let good_res = satisfy_rootfs( + &good, + SidecarSource::LocalSibling(&tmp.path().join("pack.tar.zst")), + &snap_dir, + ); + assert!( + good_res.is_ok(), + "safe single-component target_path must be accepted" + ); + } + + /// Review #295 blocker 3 / 2026-08-22: the exact same-tag regression. + /// Re-snapshotting a tag whose RW baseline IS the tag's own + /// `snap_dir/rootfs.ext4` must be flagged as a self-clone (so a re-run + /// can't delete its own source during publish). `rootfs_clone_into_self` + /// compares against the FINAL `snap_dir/rootfs.ext4` path, not a + /// transient staging path. + #[test] + fn rootfs_clone_into_self_rejects_same_tag_rootfs() { + let tmp = tempfile::tempdir().unwrap(); + let snap_dir = tmp.path().join("py"); + std::fs::create_dir_all(&snap_dir).unwrap(); + // The tag's own rootfs.ext4 already exists (a prior snapshot). + let existing = snap_dir.join("rootfs.ext4"); + std::fs::write(&existing, b"existing-rootfs").unwrap(); + + // Passing the existing tag's own rootfs.ext4 as the baseline = self-clone. + assert!( + rootfs_clone_into_self(&existing, &snap_dir), + "baseline == snap_dir/rootfs.ext4 must be flagged as self-clone" + ); + + // A genuinely distinct baseline is NOT a self-clone. + let other = tmp.path().join("baseline.ext4"); + std::fs::write(&other, b"different-rootfs").unwrap(); + assert!( + !rootfs_clone_into_self(&other, &snap_dir), + "a distinct baseline must not be flagged as self-clone" + ); + + // Relative path resolving to the same inode is still flagged. + assert!( + rootfs_clone_into_self(&existing, &snap_dir), + "same file via same path must be flagged" + ); + } + + /// Review #295 blocker 1 / 2026-08-22 Linux+KVM regression (the + /// reviewer's explicit ask): a snapshot whose rootfs lives at the + /// STABLE `snap_dir/rootfs.ext4` — and whose staging dir is gone after + /// publish — must restore from the published tag. The blocker-1 fix + /// matters because Firecracker serializes the drive `path_on_host` + /// into the binary vmstate and reopens THAT path on `/snapshot/load` + /// (no PUT /drives override is accepted). Previously the clone booted + /// from a transient `staging_dir/rootfs.ext4` that vanish on publish, + /// so the first restore failed. This test boots, snapshots to a + /// staging dir, publishes metadata (rootfs already at the stable + /// path), confirms staging is cleared, then restores and pings the + /// guest. + /// + /// Needs Linux + KVM + firecracker + an ext4 rootfs image (see + /// FORKD_TEST_KERNEL / FORKD_TEST_ROOTFS). Mirrors the forkd-vmm + /// `kvm_clock_survives_snapshot_restore` harness. + #[test] + #[ignore = "requires Linux + KVM + firecracker + rootfs image"] + #[cfg(target_os = "linux")] + fn snapshot_stable_rootfs_publishes_and_restores() { + let kernel = std::env::var("FORKD_TEST_KERNEL") + .unwrap_or_else(|_| "/var/lib/forkd/kernels/vmlinux".to_string()); + let baseline = std::env::var("FORKD_TEST_ROOTFS") + .expect("FORKD_TEST_ROOTFS must point to an ext4 rootfs image"); + assert!( + std::path::Path::new(&kernel).exists(), + "FORKD_TEST_KERNEL not found" + ); + assert!( + std::path::Path::new(&baseline).exists(), + "FORKD_TEST_ROOTFS not found" + ); + + let root = std::env::temp_dir().join(format!( + "forkd-snapshot-stable-rootfs-{}", + std::process::id() + )); + let snap_dir = root.join("snap"); + let work_dir = root.join("work"); + let staging = root.join("snap.staging"); + let restore_dir = root.join("restore"); + for p in [&root, &snap_dir, &work_dir, &restore_dir] { + let _ = std::fs::remove_dir_all(p); + } + + // Clone the baseline to the STABLE final rootfs path (blocker-1 + // fix: the VM boots from where the clone will persist after + // publish). + std::fs::create_dir_all(&snap_dir).unwrap(); + let final_rootfs = snap_dir.join("rootfs.ext4"); + forkd_vmm::chain::reflink_copy(std::path::Path::new(&baseline), &final_rootfs) + .expect("clone baseline to stable rootfs path"); + + // Boot from the stable path; snapshot vmstate+memory into staging. + let cfg = BootConfig::ext4_rw( + std::path::PathBuf::from(kernel), + final_rootfs.clone(), + work_dir.clone(), + ); + let mut vm = Vm::boot(&cfg).expect("boot parent VM from stable rootfs"); + let _ = ping_at("10.42.0.2:8888"); // best-effort warmup + vm.pause().expect("pause parent"); + let snap = vm + .snapshot_to( + staging.join("vmstate"), + staging.join("memory.bin"), + Vec::new(), + ) + .expect("snapshot to staging"); + vm.kill().expect("kill parent"); + + // Mirror snapshot_cmd: write the Snapshot metadata (with its rootfs + // path) into staging so publish_snapshot_metadata's all-three-files + // pre-check passes and the published snapshot is restorable. + let meta = serde_json::to_vec_pretty(&snap).expect("serialize snapshot meta"); + std::fs::write(staging.join("snapshot.json"), meta).expect("write staging snapshot.json"); + + // Publish metadata only — the rootfs stays at the stable path. + publish_snapshot_metadata(&staging, &snap_dir).expect("publish metadata"); + // Confirm the transient staging artifacts are gone (reviewer ask). + assert!( + !staging.join("vmstate").exists() + && !staging.join("memory.bin").exists() + && !staging.join("snapshot.json").exists(), + "staging must be cleared after publish" + ); + // Rootfs still at the stable final path (not moved/renamed). + assert!( + final_rootfs.exists(), + "rootfs must remain at the stable final path" + ); + + // Restore from the published tag: the vmstate reopens + // snap_dir/rootfs.ext4 (the recorded path), which still exists. + let opts = ForkOpts { + n: 1, + memory_backend: forkd_vmm::MemoryBackend::File, + ..ForkOpts::default() + }; + let res = snap + .restore_many_with(opts, &restore_dir) + .expect("restore from published tag must succeed (blocker-1)"); + assert_eq!(res.children.len(), 1, "one child expected after restore"); + + let _ = std::fs::remove_dir_all(&root); + } }