readlink -f and realpath fail on every operand of a rooted mount - #429
Closed
tobert wants to merge 2 commits into
Closed
readlink -f and realpath fail on every operand of a rooted mount#429tobert wants to merge 2 commits into
tobert wants to merge 2 commits into
Conversation
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 <noreply@anthropic.com>
…e 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: <operand>: <message>` (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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUPERSEDED — do not merge. Kept open only so the trail is readable; close it whenever you like.
This branch fixed
readlink -fon a rooted mount by patching readlink's own path walk. Both halves of that fix turned out to belong a layer down, and both now live elsewhere:The ancestor half is #435.
ls,stat,cd, and the file tests failed identically on the same paths, so it was never readlink's bug — the router's ancestor synthesis was unreachable whenever/was mounted.The containment half is #434. A builtin walking symlinks itself never reaches the containment its backend would apply, so canonicalization moved into
Filesystemwhereresolve_beneathalready answers the question.The eight tests survive, on #434. They run the exact reported shape end to end through a real kernel, and they pass there with none of this branch's implementation present — which is the claim worth pinning.
Read #435 then #434 instead.