From 7d9c4202548314bc9a2ee8c1b767030b8590d5e0 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Tue, 1 Sep 2026 09:36:07 -0400 Subject: [PATCH 1/2] fix: readlink -f / realpath fail on a rooted LocalFs mount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit kaibo reported readlink -f failing on every operand against v0.17.0, 100% failure rate, always naming the same wrong path. Their kernel mounts LocalFs read-only at a project root that mirrors its own host path several components deep, with MemoryFs at /. canonicalize_path_allow_missing_final (readlink.rs) walks a target path component by component from an empty PathBuf and lstats every Normal component through the router. Above the mount point, those components are structural — no single backend owns "/tmp" when the real mount sits at "/tmp/x/fixture". VfsRouter::mount_of always matches "/" (root matches everything), so the query routes to MemoryFs and comes back NotFound, and the walk reports that as an intermediate-component error naming the ancestor. realpath.rs calls the same helper and shared the bug. Bare readlink was unaffected — it does one lstat on the full mount-relative path, never walking ancestors. A reproduction test (readlink_rooted_mount_tests.rs) confirmed the mechanism exactly: all five reported cases (symlink, plain file, dangling link, missing file, realpath) failed with "No such file or directory: /home" — the first component of the test's own tempdir, not the operand. The unrooted control (LocalFs mounted at VFS /) kept passing, matching the report that unrooted kernels never saw this. The fix teaches the walk which VFS paths are structural: is_structural computes the longest-prefix owning mount for a path (mirroring VfsRouter::mount_of) via KernelBackend::mounts(), and treats an exact mount boundary or a strict ancestor of one as a synthesized directory, skipping the lstat. A path genuinely covered by a real, more specific mount still goes through that mount's own lstat unchanged. Fixing the ancestor walk uncovered a second bug the same evidence loop caught: once the walk could reach a symlink inside the mount, a target escaping the mount's own root (an absolute host path outside it, or enough ".." to climb out) resolved and printed successfully instead of being refused. The per-hop lstat calls route through whatever mount happens to claim the resulting path (root, in practice), with no containment check of their own. resolve_beneath already gives LocalFs this guarantee for a single mount; readlink.rs now enforces the same rule across the walk: once resolution starts under a non-root mount, a symlink hop landing outside that mount is refused with "path escapes root", not silently returned. Rule now in force: canonicalizing a VFS path must recognize the structural ancestors of a mount before asking any backend to lstat them, and must refuse a symlink chain that crosses out of the mount it started in. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 6 + .../src/tools/builtin/readlink.rs | 89 +++++++- .../tests/readlink_rooted_mount_tests.rs | 202 ++++++++++++++++++ 3 files changed, 295 insertions(+), 2 deletions(-) create mode 100644 crates/kaish-kernel/tests/readlink_rooted_mount_tests.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index fb1469a7..94aab20f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,12 @@ breaking entries are marked **BREAKING**. ## [Unreleased] +### Fixed +- **`readlink -f` / `realpath` failed every operand on a LocalFs mount rooted + below `/`** — canonicalization `lstat`'d structural VFS components above + the mount, naming the mount's first path segment as missing. Also closed: + a symlink target escaping the mount root was returned instead of refused. + ## [0.17.0] - 2026-08-31 ### Changed diff --git a/crates/kaish-kernel/src/tools/builtin/readlink.rs b/crates/kaish-kernel/src/tools/builtin/readlink.rs index 762b71f8..96611f0d 100644 --- a/crates/kaish-kernel/src/tools/builtin/readlink.rs +++ b/crates/kaish-kernel/src/tools/builtin/readlink.rs @@ -9,6 +9,7 @@ use std::path::{Path, PathBuf}; use crate::interpreter::{ExecResult, OutputData}; use crate::tools::{schema_from_clap, ExecContext, ToolCtx, GlobalFlags, Tool, ToolArgs, ToolSchema}; +use kaish_types::backend::MountInfo; /// Maximum symlink hops to prevent infinite loops (matches Linux MAXSYMLINKS). const MAX_SYMLINK_HOPS: usize = 40; @@ -150,6 +151,14 @@ impl Tool for Readlink { /// if the last component doesn't exist but its parent does, return the /// normalized parent + final component. /// +/// A mount can be rooted below `/` (an embedder mounting `LocalFs` at a deep +/// VFS path, with another filesystem covering `/`). The components ABOVE that +/// mount point are structural — the router synthesizes them as directories — +/// not entries any single backend can `lstat`; querying one against whatever +/// backend happens to own `/` produced a bogus "No such file or directory" +/// naming the mount's first path component. `is_structural` recognizes those +/// components (via `KernelBackend::mounts`) and skips the `lstat`. +/// /// Returns an error if any intermediate (non-final) component is missing or /// if symlink resolution loops. pub async fn canonicalize_path_allow_missing_final( @@ -163,6 +172,7 @@ pub async fn canonicalize_path_allow_missing_final( return Err("empty path".to_string()); } + let mounts = ctx.backend.mounts(); let mut current = PathBuf::new(); for (idx, component) in components.iter().enumerate() { @@ -178,8 +188,10 @@ pub async fn canonicalize_path_allow_missing_final( } std::path::Component::Normal(_) => { current.push(component); - // Resolve symlinks at this component. - current = resolve_symlink_component(ctx, current, is_last).await?; + if !is_structural(¤t, &mounts) { + // Resolve symlinks at this component. + current = resolve_symlink_component(ctx, &mounts, current, is_last).await?; + } } std::path::Component::Prefix(_) => { current.push(component); @@ -190,17 +202,79 @@ pub async fn canonicalize_path_allow_missing_final( Ok(current) } +/// True when `path` is a structural VFS path no single backend owns: a mount +/// point boundary itself, or a strict ancestor of one (`/tmp` above a mount +/// at `/tmp/x/fixture`). The router synthesizes both as directories rather +/// than asking a backend, so they can never be symlinks and never need an +/// `lstat` call. +/// +/// A path genuinely covered by a real, more specific (non-`/`) mount is not +/// structural — that mount's own `lstat` answers for it, containment and all. +/// Longest-prefix match mirrors `VfsRouter::mount_of`'s own selection so this +/// agrees with how the router will actually route the path. +fn is_structural(path: &Path, mounts: &[MountInfo]) -> bool { + match owning_mount(path, mounts) { + // The path IS a mount point: the router synthesizes it, never + // delegates to the mount's own root lstat. + Some(m) if m == path => true, + // A real, more specific mount covers this path — trust its lstat. + Some(m) if m != Path::new("/") => false, + // Only `/` (or nothing) covers this path directly. Structural only + // if it sits strictly above some other mount's own root. + _ => mounts.iter().any(|m| m.path != path && is_strict_prefix(path, &m.path)), + } +} + +/// The longest mount path that covers `path` — `path` equals it, or sits +/// beneath it — mirroring `VfsRouter::mount_of`'s own longest-prefix +/// selection. `None` when no mount covers `path` at all. +fn owning_mount(path: &Path, mounts: &[MountInfo]) -> Option { + mounts + .iter() + .map(|m| m.path.as_path()) + .filter(|m| *m == path || is_strict_prefix(m, path)) + .max_by_key(|m| m.as_os_str().len()) + .map(PathBuf::from) +} + +/// True when `path` is a strict ancestor of `descendant` (`descendant` +/// starts with `path` plus a separator). Handles `path == "/"` without +/// producing a doubled slash. +fn is_strict_prefix(path: &Path, descendant: &Path) -> bool { + let path_str = path.to_string_lossy(); + let descendant_str = descendant.to_string_lossy(); + if path_str == "/" { + descendant_str.len() > 1 && descendant_str.starts_with('/') + } else { + descendant_str.starts_with(&format!("{}/", path_str)) + } +} + /// Resolve potential symlinks at `path`, following the chain up to /// `MAX_SYMLINK_HOPS`. If `allow_missing` is true and the path doesn't exist, /// return `path` unchanged (the caller already knows it's the final component). async fn resolve_symlink_component( ctx: &ExecContext, + mounts: &[MountInfo], path: PathBuf, allow_missing: bool, ) -> Result { + // The mount that owns the starting path. A rooted (non-`/`) mount is a + // containment boundary: a symlink hop that lands the walk outside it — + // an absolute target elsewhere on the host, or enough `..` to climb out + // — must be refused, the same guarantee `resolve_beneath` gives a single + // `LocalFs`. Root (`/`) draws no such boundary: it's the unrooted case + // every existing symlink walks across freely. + let start_owner = owning_mount(&path, mounts); let mut current = path; for _ in 0..MAX_SYMLINK_HOPS { + // A symlink target can jump onto a structural path (e.g. an absolute + // target landing above another mount); re-check every hop, not just + // the outer walk's own components. + if is_structural(¤t, mounts) { + return Ok(current); + } match ctx.backend.lstat(Path::new(¤t)).await { Ok(entry) if entry.is_symlink() => { let target = ctx @@ -218,6 +292,17 @@ async fn resolve_symlink_component( } // Normalize out any . and .. introduced by the target. current = normalize_path_buf(current); + + if let Some(start) = &start_owner + && start.as_path() != Path::new("/") + && owning_mount(¤t, mounts).as_deref() != Some(start.as_path()) + { + return Err(format!( + "path escapes root: {} is not under {}", + current.display(), + start.display() + )); + } } Ok(_) => { // Not a symlink — resolved. diff --git a/crates/kaish-kernel/tests/readlink_rooted_mount_tests.rs b/crates/kaish-kernel/tests/readlink_rooted_mount_tests.rs new file mode 100644 index 00000000..def7e237 --- /dev/null +++ b/crates/kaish-kernel/tests/readlink_rooted_mount_tests.rs @@ -0,0 +1,202 @@ +//! `readlink -f` / `realpath` on a LocalFs mount rooted below `/`. +//! +//! Reproduces a bug reported against v0.17.0 by the kaibo project: a kernel +//! shape with `LocalFs` mounted read-only at a deep VFS path (mirroring its +//! own host directory, several path components below `/`) and `MemoryFs` at +//! `/` — the common embedder pattern (`kaijutsu`, `kaibo`). `readlink -f` +//! failed on every operand, reporting the FIRST PATH COMPONENT of the mount +//! root as "No such file or directory", because `canonicalize_path_allow_missing_final` +//! walks every component of the VFS path through `lstat`, including the +//! components ABOVE the mount point that no single backend owns. +//! +//! Bare `readlink` (no `-f`) was unaffected — it does one `lstat` on the +//! full, already-mount-relative path, never walking ancestors. + +// Test-fixture code: unwrap/expect on known-good setup is the idiom here. +#![allow(clippy::unwrap_used, clippy::expect_used)] +// Symlinks are unix-only; real FS via localfs feature. +#![cfg(all(feature = "localfs", unix))] + +use std::os::unix::fs::symlink; +use std::path::Path; +use std::sync::Arc; + +use kaish_kernel::vfs::{LocalFs, MemoryFs, VfsRouter}; +use kaish_kernel::{Kernel, KernelBackend, KernelConfig, LocalBackend}; + +fn tempdir() -> tempfile::TempDir { + // Several path components deep under CARGO_TARGET_TMPDIR (itself deep), + // so the mount root is not adjacent to `/` — the shape that reproduces + // the bug. `fixture_root` joins on two more components below this. + tempfile::Builder::new() + .prefix("readlink-rooted-") + .tempdir_in(env!("CARGO_TARGET_TMPDIR")) + .expect("tempdir under CARGO_TARGET_TMPDIR") +} + +/// The mount's VFS path AND host root: several components below `/`, and, +/// per the report, the mount path mirrors the host path (the same string +/// used as both the VFS prefix and the real directory) — the common +/// embedder pattern of projecting a project's own absolute host path +/// straight into the VFS namespace. +fn fixture_root(base: &tempfile::TempDir) -> std::path::PathBuf { + let root = base.path().join("project").join("fixture"); + std::fs::create_dir_all(&root).expect("mkdir project/fixture"); + root +} + +/// LocalFs read-only at `fixture_root`, mounted at that SAME path in VFS +/// space; MemoryFs at `/` — the kaibo-reported shape. +fn rooted_kernel(root: &Path) -> Kernel { + let mut vfs = VfsRouter::new(); + vfs.mount(root.to_path_buf(), LocalFs::read_only(root.to_path_buf())); + vfs.mount("/", MemoryFs::new()); + let backend: Arc = Arc::new(LocalBackend::new(Arc::new(vfs))); + let config = KernelConfig::isolated().with_cwd(root.to_path_buf()); + Kernel::with_backend(backend, config, |_| {}, |_| {}).expect("with_backend kernel") +} + +/// Control: the same fixture layout with LocalFs mounted at VFS root `/` +/// (the ordinary, unrooted shape existing tests already cover). Must keep +/// passing — proves the fix does not regress the common case. +fn unrooted_kernel(root: &Path) -> Kernel { + let config = KernelConfig::repl() + .with_cwd(root.to_path_buf()) + .with_trash(false); + Kernel::new(config).expect("kernel") +} + +async fn run(kernel: &Kernel, script: &str) -> (String, String, i64) { + let r = kernel.execute(script).await.expect("kernel execute"); + (r.text_out().trim().to_string(), r.err.clone(), r.code) +} + +fn seed(root: &Path) { + std::fs::create_dir_all(root.join("d")).unwrap(); + std::fs::write(root.join("d/a.txt"), "content").unwrap(); + std::fs::write(root.join("top.txt"), "top-level").unwrap(); + symlink("d/a.txt", root.join("link.txt")).unwrap(); + symlink("nosuchtarget", root.join("dangling")).unwrap(); +} + +// --------------------------------------------------------------------------- +// Rooted mount: all five reported cases +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn rooted_bare_readlink_on_symlink_works() { + let base = tempdir(); + let root = fixture_root(&base); + seed(&root); + let k = rooted_kernel(&root); + + let (out, err, code) = run(&k, "readlink link.txt").await; + assert_eq!(code, 0, "bare readlink should succeed: err={err}"); + assert_eq!(out, "d/a.txt"); +} + +#[tokio::test] +async fn rooted_readlink_f_on_symlink_resolves() { + let base = tempdir(); + let root = fixture_root(&base); + seed(&root); + let k = rooted_kernel(&root); + + let (out, err, code) = run(&k, "readlink -f link.txt").await; + assert_eq!(code, 0, "readlink -f on a symlink should succeed: err={err}"); + let expected = root.join("d/a.txt").to_string_lossy().into_owned(); + assert_eq!(out, expected); +} + +#[tokio::test] +async fn rooted_readlink_f_on_regular_file_resolves() { + let base = tempdir(); + let root = fixture_root(&base); + seed(&root); + let k = rooted_kernel(&root); + + let (out, err, code) = run(&k, "readlink -f top.txt").await; + assert_eq!(code, 0, "readlink -f on a plain regular file should succeed: err={err}"); + let expected = root.join("top.txt").to_string_lossy().into_owned(); + assert_eq!(out, expected); +} + +#[tokio::test] +async fn rooted_readlink_f_on_dangling_link_resolves() { + let base = tempdir(); + let root = fixture_root(&base); + seed(&root); + let k = rooted_kernel(&root); + + let (out, err, code) = run(&k, "readlink -f dangling").await; + assert_eq!(code, 0, "readlink -f on a dangling link should succeed (GNU allows a missing final target): err={err}"); + let expected = root.join("nosuchtarget").to_string_lossy().into_owned(); + assert_eq!(out, expected); +} + +#[tokio::test] +async fn rooted_readlink_f_on_missing_file_resolves() { + let base = tempdir(); + let root = fixture_root(&base); + seed(&root); + let k = rooted_kernel(&root); + + let (out, err, code) = run(&k, "readlink -f nosuchfile").await; + assert_eq!(code, 0, "readlink -f on a missing final component should succeed: err={err}"); + let expected = root.join("nosuchfile").to_string_lossy().into_owned(); + assert_eq!(out, expected); +} + +#[tokio::test] +async fn rooted_readlink_f_link_escaping_root_is_refused() { + // A symlink inside the root whose target is an absolute host path + // outside the mount's own root. Containment must still be refused — + // fixing the ancestor-walk bug must not open this hole. + let base = tempdir(); + let root = fixture_root(&base); + seed(&root); + let outside = tempfile::tempdir().expect("outside tempdir"); + std::fs::write(outside.path().join("secret.txt"), "s").unwrap(); + symlink(outside.path().join("secret.txt"), root.join("escape.txt")).unwrap(); + + let k = rooted_kernel(&root); + let (out, err, code) = run(&k, "readlink -f escape.txt").await; + assert_ne!( + code, 0, + "readlink -f through a link escaping the mount root must be refused, got out={out:?}" + ); + // Specifically "escape" — not the ancestor-walk bug's "No such file or + // directory" wearing a different path, which would pass here for the + // wrong reason. + assert!(err.contains("escape"), "error should name the escape, got: {err}"); +} + +#[tokio::test] +async fn rooted_realpath_on_symlink_resolves() { + let base = tempdir(); + let root = fixture_root(&base); + seed(&root); + let k = rooted_kernel(&root); + + let (out, err, code) = run(&k, "realpath link.txt").await; + assert_eq!(code, 0, "realpath on a symlink should succeed: err={err}"); + let expected = root.join("d/a.txt").to_string_lossy().into_owned(); + assert_eq!(out, expected); +} + +// --------------------------------------------------------------------------- +// Control: unrooted (LocalFs at VFS `/`) must keep working +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn unrooted_readlink_f_on_symlink_still_resolves() { + let base = tempdir(); + let root = fixture_root(&base); + seed(&root); + let k = unrooted_kernel(&root); + + let (out, err, code) = run(&k, "readlink -f link.txt").await; + assert_eq!(code, 0, "control (unrooted) readlink -f must still pass: err={err}"); + let expected = root.join("d/a.txt").to_string_lossy().into_owned(); + assert_eq!(out, expected); +} From 5425b6976de55921f86b443dbc4aa770080845f7 Mon Sep 17 00:00:00 2001 From: A Tobey Date: Tue, 1 Sep 2026 09:45:43 -0400 Subject: [PATCH 2/2] test(readlink): the containment test passed in both trees; the fixture name did it Review of the previous commit. The containment fix is right and the test for it did not test it. The link was named `escape.txt` and the assertion was `err.contains("escape")`. readlink formats a failure as `readlink: : ` (readlink.rs:94), so the operand puts "escape" in every failure message this test could produce -- including the ancestor-walk failure the same commit removes, which refused every operand with "No such file or directory". Both assertions passed before the fix and after it. The comment above them explained why the assertion was specific; the reasoning was sound and the fixture name defeated it. Proved rather than argued: disabling ONLY the containment branch, keeping the ancestor fix, makes the test fail with got out="/tmp/.tmphS8Cjv/secret.txt" so the hole is real and the test now sees it. It asserts the containment message, asserts the absence of "No such file or directory" so the old bug cannot satisfy it, and asserts the target never appears in stdout. The fixture is `outward.txt`: a name that cannot pass the test on the operand's behalf. The rule: a fixture named for the behavior under test can satisfy an assertion about that behavior without the behavior occurring. Name the fixture for what it IS, not for what it should do. `is_strict_prefix` also went component-wise. It compared `to_string_lossy` output and matched a textual `"{path}/"` prefix, which called `/tmpfoo` a child of `/tmp` and folded two distinct non-UTF-8 paths together. It decides whether a symlink may leave its mount, so `Path::starts_with` is the comparison it needed. --- .../src/tools/builtin/readlink.rs | 17 +++++------ .../tests/readlink_rooted_mount_tests.rs | 28 +++++++++++++++---- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/crates/kaish-kernel/src/tools/builtin/readlink.rs b/crates/kaish-kernel/src/tools/builtin/readlink.rs index 96611f0d..673eb3be 100644 --- a/crates/kaish-kernel/src/tools/builtin/readlink.rs +++ b/crates/kaish-kernel/src/tools/builtin/readlink.rs @@ -237,17 +237,14 @@ fn owning_mount(path: &Path, mounts: &[MountInfo]) -> Option { .map(PathBuf::from) } -/// True when `path` is a strict ancestor of `descendant` (`descendant` -/// starts with `path` plus a separator). Handles `path == "/"` without -/// producing a doubled slash. +/// True when `path` is a strict ancestor of `descendant`. +/// +/// Component-wise, not textual: `/tmpfoo` is not under `/tmp`, and a path +/// whose bytes are not UTF-8 compares as itself. A containment check decides +/// whether a symlink may leave its mount, so a lossy comparison here would be +/// a lossy comparison there. fn is_strict_prefix(path: &Path, descendant: &Path) -> bool { - let path_str = path.to_string_lossy(); - let descendant_str = descendant.to_string_lossy(); - if path_str == "/" { - descendant_str.len() > 1 && descendant_str.starts_with('/') - } else { - descendant_str.starts_with(&format!("{}/", path_str)) - } + descendant != path && descendant.starts_with(path) } /// Resolve potential symlinks at `path`, following the chain up to diff --git a/crates/kaish-kernel/tests/readlink_rooted_mount_tests.rs b/crates/kaish-kernel/tests/readlink_rooted_mount_tests.rs index def7e237..f59f79a7 100644 --- a/crates/kaish-kernel/tests/readlink_rooted_mount_tests.rs +++ b/crates/kaish-kernel/tests/readlink_rooted_mount_tests.rs @@ -157,18 +157,34 @@ async fn rooted_readlink_f_link_escaping_root_is_refused() { seed(&root); let outside = tempfile::tempdir().expect("outside tempdir"); std::fs::write(outside.path().join("secret.txt"), "s").unwrap(); - symlink(outside.path().join("secret.txt"), root.join("escape.txt")).unwrap(); + // The link is NOT named for what it does. The builtin formats a failure as + // `readlink: : `, so an operand containing "escape" makes + // every failure — including the ancestor-walk bug this fix removes — satisfy + // an assertion looking for that word. The name must not be able to pass the + // test on the operand's behalf. + symlink(outside.path().join("secret.txt"), root.join("outward.txt")).unwrap(); let k = rooted_kernel(&root); - let (out, err, code) = run(&k, "readlink -f escape.txt").await; + let (out, err, code) = run(&k, "readlink -f outward.txt").await; assert_ne!( code, 0, "readlink -f through a link escaping the mount root must be refused, got out={out:?}" ); - // Specifically "escape" — not the ancestor-walk bug's "No such file or - // directory" wearing a different path, which would pass here for the - // wrong reason. - assert!(err.contains("escape"), "error should name the escape, got: {err}"); + assert!( + err.contains("path escapes root"), + "containment must be what refused it, got: {err}" + ); + // The ancestor-walk bug refused everything with this message. If it is back, + // the refusal above is the old bug wearing the new test's clothes. + assert!( + !err.contains("No such file or directory"), + "refused for the wrong reason — this is the ancestor-walk bug, not containment: {err}" + ); + // The target must not leak, whatever the reason for refusal. + assert!( + !out.contains("secret.txt"), + "an escaping target must never be printed, got out={out:?}" + ); } #[tokio::test]