From 75fdeccfd1d327a03454e83b9c3bc9b8147a496a Mon Sep 17 00:00:00 2001 From: Fernando Dingler Date: Wed, 21 Oct 2026 05:00:04 +0000 Subject: [PATCH] perf: stop re-resolving every prefix of a missing path Finding the bind above a path walked up one component at a time and resolved the whole prefix at each step, so a long path of missing components cost time quadratic in its length while holding the VFS lock. Nothing resolves past a bind and a shorter prefix resolves whenever a longer one does, so a binary search for the longest resolving prefix finds the same ancestor, and the missing tail is appended once. Adds unit tests that pin the existing answers (missing tails under a bind, '..' across a missing component, symlinks before a missing tail, refused host symlinks, no bind ancestor) and a test that compares the cost of 2,000 and 16,000 missing components. --- src/vfs_kernel.rs | 191 ++++++++++++++++++++++++++++------ tests/path_resolution_cost.rs | 60 +++++++++++ 2 files changed, 218 insertions(+), 33 deletions(-) create mode 100644 tests/path_resolution_cost.rs diff --git a/src/vfs_kernel.rs b/src/vfs_kernel.rs index d9e6838..0cb5977 100644 --- a/src/vfs_kernel.rs +++ b/src/vfs_kernel.rs @@ -73,41 +73,54 @@ impl VfsKernel { _ => {} } } - // Walk components looking for a HostDir ancestor - let components: Vec<&str> = abs_path.split('/').filter(|c| !c.is_empty()).collect(); - for i in (0..components.len()).rev() { - let prefix = format!("/{}", components[..=i].join("/")); - if let Ok(ino) = vfs.resolve(&prefix, true) - && let Ok(inode) = vfs.get(ino) - && let InodeData::HostDir(host_base, ro) = &inode.data - { - let rest = &components[i + 1..]; - let mut host = PathBuf::from(host_base); - for c in rest { - host.push(c); - } - // Canonicalize and verify the path stays within the bind mount - let canon_base = - std::fs::canonicalize(host_base).unwrap_or_else(|_| PathBuf::from(host_base)); - if let Ok(canon_host) = std::fs::canonicalize(&host) { - if !canon_host.starts_with(&canon_base) { - return None; // symlink escape — block access - } - return Some((canon_host, *ro, canon_base)); - } - // canonicalize failed — check if path is a dangling symlink - if host.symlink_metadata().is_ok() { - return None; // dangling symlink pointing outside mount - } - // Path truly doesn't exist — verify parent is safe - if let Some(parent) = host.parent() - && let Ok(canon_parent) = std::fs::canonicalize(parent) - && !canon_parent.starts_with(&canon_base) - { - return None; + // Nothing resolves past a HostDir, and a prefix of a normalized path resolves whenever a + // longer one does, so only the longest prefix that resolves can be the HostDir ancestor. + let normalized = vfs::normalize(abs_path); + let components: Vec<&str> = normalized.split('/').filter(|c| !c.is_empty()).collect(); + let resolves = |length: usize| { + vfs.resolve(&format!("/{}", components[..length].join("/")), true) + .ok() + }; + let (mut low, mut high) = (0, components.len()); + while low < high { + let middle = low + (high - low).div_ceil(2); + if resolves(middle).is_some() { + low = middle; + } else { + high = middle - 1; + } + } + if low > 0 + && let Some(ino) = resolves(low) + && let Ok(inode) = vfs.get(ino) + && let InodeData::HostDir(host_base, ro) = &inode.data + { + let rest = &components[low..]; + let mut host = PathBuf::from(host_base); + for c in rest { + host.push(c); + } + // Canonicalize and verify the path stays within the bind mount + let canon_base = + std::fs::canonicalize(host_base).unwrap_or_else(|_| PathBuf::from(host_base)); + if let Ok(canon_host) = std::fs::canonicalize(&host) { + if !canon_host.starts_with(&canon_base) { + return None; // symlink escape — block access } - return Some((host, *ro, canon_base)); + return Some((canon_host, *ro, canon_base)); + } + // canonicalize failed — check if path is a dangling symlink + if host.symlink_metadata().is_ok() { + return None; // dangling symlink pointing outside mount + } + // Path truly doesn't exist — verify parent is safe + if let Some(parent) = host.parent() + && let Ok(canon_parent) = std::fs::canonicalize(parent) + && !canon_parent.starts_with(&canon_base) + { + return None; } + return Some((host, *ro, canon_base)); } None } @@ -1537,3 +1550,115 @@ mod url_safety_tests { assert!(!matches("http://evil.example.com/", "http://h.example.com")); } } + +#[cfg(all(test, unix))] +mod resolve_host_tests { + use std::path::PathBuf; + + use super::VfsKernel; + use crate::vfs::{InodeData, LASH_GID, LASH_UID, Vfs}; + + struct Fixture { + vfs: Vfs, + workspace: PathBuf, + canonical: PathBuf, + } + + // A bind at /mnt/deep/ws, reached directly, through /alias, and through /chain -> alias. + fn fixture(name: &str) -> Fixture { + let base = std::env::temp_dir().join(format!("resolve-host-{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + let workspace = base.join("ws"); + let outside = base.join("outside"); + std::fs::create_dir_all(workspace.join("sub")).unwrap(); + std::fs::create_dir_all(&outside).unwrap(); + std::fs::write(workspace.join("real.txt"), "in mount").unwrap(); + std::os::unix::fs::symlink(base.join("absent"), workspace.join("dangle")).unwrap(); + std::os::unix::fs::symlink(&outside, workspace.join("escape")).unwrap(); + + let mut vfs = Vfs::new(); + vfs.mkdir_p("/mnt/deep", 0o755, LASH_UID, LASH_GID).unwrap(); + let source = workspace.to_str().unwrap().to_string(); + vfs.mknod( + "/mnt/deep/ws", + InodeData::HostDir(source, false), + 0o100644, + LASH_UID, + LASH_GID, + ) + .unwrap(); + vfs.symlink("/alias", "/mnt/deep/ws", LASH_UID, LASH_GID) + .unwrap(); + vfs.symlink("/chain", "alias", LASH_UID, LASH_GID).unwrap(); + let canonical = std::fs::canonicalize(&workspace).unwrap(); + Fixture { + vfs, + workspace, + canonical, + } + } + + fn host(fixture: &Fixture, path: &str) -> Option { + VfsKernel::resolve_host(&fixture.vfs, path).map(|(host, _, _)| host) + } + + #[test] + fn existing_and_missing_paths_under_a_bind() { + let f = fixture("under"); + assert_eq!(host(&f, "/mnt/deep/ws"), Some(f.workspace.clone())); + assert_eq!( + host(&f, "/mnt/deep/ws/real.txt"), + Some(f.canonical.join("real.txt")) + ); + assert_eq!( + host(&f, "/mnt/deep/ws/a/b/c"), + Some(f.workspace.join("a/b/c")) + ); + assert_eq!( + host(&f, "/mnt/deep/ws/sub/missing"), + Some(f.workspace.join("sub/missing")) + ); + } + + #[test] + fn dot_dot_across_a_missing_component() { + let f = fixture("dotdot"); + assert_eq!( + host(&f, "/mnt/deep/ws/missing/../real.txt"), + Some(f.canonical.join("real.txt")) + ); + assert_eq!( + host(&f, "/mnt/deep/absent/../ws/real.txt"), + Some(f.canonical.join("real.txt")) + ); + assert_eq!(host(&f, "/mnt/deep/ws/missing/../../x"), None); + } + + #[test] + fn symlinks_before_a_missing_tail() { + let f = fixture("symlinked"); + assert_eq!( + host(&f, "/alias/real.txt"), + Some(f.canonical.join("real.txt")) + ); + assert_eq!(host(&f, "/alias/sub/x"), Some(f.workspace.join("sub/x"))); + assert_eq!(host(&f, "/chain/a/b"), Some(f.workspace.join("a/b"))); + } + + #[test] + fn host_symlinks_that_dangle_or_leave_the_bind_are_refused() { + let f = fixture("refused"); + assert_eq!(host(&f, "/mnt/deep/ws/dangle"), None); + assert_eq!(host(&f, "/mnt/deep/ws/escape"), None); + assert_eq!(host(&f, "/mnt/deep/ws/escape/f"), None); + } + + #[test] + fn paths_without_a_bind_ancestor_have_no_host_path() { + let f = fixture("virtual"); + assert_eq!(host(&f, "/"), None); + assert_eq!(host(&f, "/mnt/deep"), None); + assert_eq!(host(&f, "/mnt/deep/x/y"), None); + assert_eq!(host(&f, "/absent/ws/real.txt"), None); + } +} diff --git a/tests/path_resolution_cost.rs b/tests/path_resolution_cost.rs new file mode 100644 index 0000000..17b34e0 --- /dev/null +++ b/tests/path_resolution_cost.rs @@ -0,0 +1,60 @@ +//! Resolving a missing path should cost roughly linear time in its component count. + +use std::path::PathBuf; +use std::time::{Duration, Instant}; + +use strands_shell::Shell; + +const SHORT: usize = 2_000; +const LONG: usize = 8 * SHORT; + +fn shell(name: &str) -> Shell { + let base = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join(name); + let _ = std::fs::remove_dir_all(&base); + let workspace = base.join("ws"); + std::fs::create_dir_all(&workspace).unwrap(); + let mut shell = Shell::builder() + .bind_direct(workspace.to_str().unwrap(), "/workspace") + .max_input(1 << 20) + .build() + .unwrap(); + shell.proc.cwd = PathBuf::from("/workspace"); + shell +} + +async fn time_to_fail(name: &str, root: &str, components: usize) -> Duration { + let mut shell = shell(name); + let path = vec!["a"; components].join("/"); + let start = Instant::now(); + let output = shell.run(&format!("cat -- {root}/{path}")).await; + let elapsed = start.elapsed(); + assert_ne!( + output.status, 0, + "a missing path was read: {}", + output.stdout + ); + elapsed +} + +// Eight times the components should take about eight times as long; a quadratic walk takes +// about sixty-four. Comparing two sizes in one run keeps the bound independent of machine speed. +async fn assert_grows_linearly(root: &str) { + let short = time_to_fail("path-cost-short", root, SHORT).await; + let long = time_to_fail("path-cost-long", root, LONG).await; + let bound = short * 24 + Duration::from_secs(2); + assert!( + long < bound, + "{LONG} components under {root:?} took {long:?}, {SHORT} took {short:?}" + ); +} + +#[tokio::test] +async fn a_missing_path_of_many_components_resolves_in_linear_time() { + let local = tokio::task::LocalSet::new(); + local + .run_until(async { + assert_grows_linearly("/workspace").await; + assert_grows_linearly("").await; + }) + .await; +}