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..673eb3be 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,76 @@ 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`. +/// +/// 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 { + descendant != path && descendant.starts_with(path) +} + /// 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 +289,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..f59f79a7 --- /dev/null +++ b/crates/kaish-kernel/tests/readlink_rooted_mount_tests.rs @@ -0,0 +1,218 @@ +//! `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(); + // 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 outward.txt").await; + assert_ne!( + code, 0, + "readlink -f through a link escaping the mount root must be refused, got out={out:?}" + ); + 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] +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); +}