fs: fix realpath leaving symlinks unresolved after an unrelated stat - #65113
fs: fix realpath leaving symlinks unresolved after an unrelated stat#65113unstubbable wants to merge 4 commits into
realpath leaving symlinks unresolved after an unrelated stat#65113Conversation
While walking a path, `realpathSync` skips the components it already knows are real, and in that branch it reads the shared stat buffer to decide whether the walk has reached a pipe or a socket. That buffer holds the result of the last stat made anywhere in the process rather than the last one made by the walk, so an unrelated stat of a FIFO ends the walk early and the path comes back with its symlinks unresolved. The unresolved path is then written to the cache, so every later resolution repeats it. The walk only takes that branch once the ancestors are established as real, which is the state the module loader's cache is in. The test goes through `require()` to reach it, where the stale read costs a second copy of a module reached through a symlink. Signed-off-by: Hendrik Liebau <mail@hendrik-liebau.de>
`realpathSync` decided whether a walk had reached a pipe or a socket by reading `statValues`, which holds the result of the last stat made anywhere in the process rather than the last one made by the walk itself. Any unrelated stat of a FIFO or a socket therefore ended the walk early, returning the path with its symlinks unresolved and caching it in that form. It now tracks whether the symlink it resolved last pointed at a pipe or a socket, which is the value the check was always meant to read. The async `realpath()` carries the same check and the same latent problem; that is left for a separate change, since no test here reaches it. Signed-off-by: Hendrik Liebau <mail@hendrik-liebau.de>
nodejs-github-bot
added
fs
github-actions
Bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65113 +/- ##
=======================================
Coverage 90.31% 90.31%
=======================================
Files 759 759
Lines 248290 248336 +46
Branches 46859 46864 +5
=======================================
+ Hits 224241 224297 +56
+ Misses 15472 15469 -3
+ Partials 8577 8570 -7
π New features to boost your workflow:
|
|
Reproduced the original bug on v24.18.0 and on On the async Async stat callbacks fill the same global array ( For the early return to cost anything, the path needs a second symlink after the one being resolved, so that truncating the walk leaves real work undone: $ mkdir -p pkg real && echo x > real/index.js
$ ln -s ../real pkg/sub
$ ln -s pkg link
$ mkfifo fifoconst fs = require('fs');
const path = require('path');
const p = path.join(__dirname, 'link', 'sub', 'index.js');
const expected = path.join(__dirname, 'real', 'index.js');
console.log('sync :', fs.realpathSync(p) === expected ? 'OK' : `BROKEN ${fs.realpathSync(p)}`);
let done = false;
(function spam() { if (done) return; fs.stat(path.join(__dirname, 'fifo'), spam); })();
fs.realpath(p, (err, res) => {
done = true;
console.log('async:', res === expected ? 'OK' : `BROKEN ${res}`);
});5/5 runs, both on v24.18.0 and on this branch:
fs.stat(base, (err, targetStats) => {
if (err) return callback(err);
reachedPipeOrSocket = targetStats.isFIFO() || targetStats.isSocket();
fs.readlink(base, (err, target) => {with the flag declared next to One more thing, not a regression but worth noting: the flag isn't updated on the two paths that reuse a link target without stat'ing β the |
The test covered the bug through the module loader, which is where it costs something, but the assertion sat two layers away from the function being fixed. It now also calls `realpathSync` with a cache carrying the ancestors, which is the state that makes the walk skip a component and reach the stale read, and asserts the returned path directly. The loader case stays, because a second copy of a module under a second name is what the wrong path actually costs. Signed-off-by: Hendrik Liebau <mail@hendrik-liebau.de>
`realpath()` has the same stale read that `realpathSync()` had. Its own `fs.stat()` does leave the right value in `statValues`, but the value is not read until after `fs.readlink()` and a `process.nextTick()`, and any stat completing in that window replaces it. Truncating the walk only costs something when a second symlink follows the one being resolved, so the test uses a path with two. The flag it now reads is set from the `stat()` that follows the link, as in the synchronous walk, which leaves `statValues` unused in this file. The test asserts on exit rather than inside the `realpath()` callback. An assertion that fails there is lost: it does not reach an `uncaughtException` handler and the process still exits 0, so the test passed over the bug it covers. Signed-off-by: Hendrik Liebau <mail@hendrik-liebau.de>
unstubbable
changed the title
realpathSyncrealpath leaving symlinks unresolved after an unrelated stat
|
@mcollina Thanks, that's a much better repro than anything I got for the async path. I've folded the fix in with a test built on your shape: it fails on v24.18.0 and on Worth knowing while you are in this code: an assertion that fails inside an On |
github-actions
Bot
removed
the
request-ci
mcollina
added
the
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/65113 β Done loading data for nodejs/node/pull/65113 ----------------------------------- PR info ------------------------------------ Title fs: fix `realpath` leaving symlinks unresolved after an unrelated stat (#65113) β Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch unstubbable:fix-realpath-issue -> nodejs:main Labels fs, needs-ci, commit-queue Commits 4 - test: cover `realpathSync` resolving symlinks after a FIFO stat - fs: stop reading the shared stat buffer in `realpathSync` - test: assert `realpathSync` directly, not only through `require()` - fs: stop reading the shared stat buffer in async `realpath` Committers 1 - Hendrik Liebau <mail@hendrik-liebau.de> PR-URL: https://github.com/nodejs/node/pull/65113 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/65113 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> -------------------------------------------------------------------------------- βΉ This PR was created on Fri, 07 Aug 2026 17:32:51 GMT β Approvals: 2 β - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/65113#pullrequestreview-4912979381 β - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/65113#pullrequestreview-4889115375 β Last GitHub CI successful βΉ Last Full PR CI on 2026-08-12T13:37:55Z: https://ci.nodejs.org/job/node-test-pull-request/75796/ - Querying data for job/node-test-pull-request/75796/ β Build data downloaded β Last Jenkins CI successful -------------------------------------------------------------------------------- β No git cherry-pick in progress β No git am in progress β No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD β origin/main is now up-to-date - Downloading patch for 65113 From https://github.com/nodejs/node * branch refs/pull/65113/merge -> FETCH_HEAD β Fetched commits as 00f0f8cf8acc..1deb9b8c1a87 -------------------------------------------------------------------------------- [main 4c6edbd383] test: cover `realpathSync` resolving symlinks after a FIFO stat Author: Hendrik Liebau <mail@hendrik-liebau.de> Date: Fri Aug 7 15:27:08 2026 +0200 1 file changed, 48 insertions(+) create mode 100644 test/parallel/test-fs-realpath-stale-stat-values.js [main 1469562480] fs: stop reading the shared stat buffer in `realpathSync` Author: Hendrik Liebau <mail@hendrik-liebau.de> Date: Fri Aug 7 15:27:09 2026 +0200 1 file changed, 9 insertions(+), 3 deletions(-) [main 43be604990] test: assert `realpathSync` directly, not only through `require()` Author: Hendrik Liebau <mail@hendrik-liebau.de> Date: Sat Aug 8 16:15:51 2026 +0200 1 file changed, 31 insertions(+), 6 deletions(-) [main 1c91d3ea3d] fs: stop reading the shared stat buffer in async `realpath` Author: Hendrik Liebau <mail@hendrik-liebau.de> Date: Sat Aug 8 16:49:31 2026 +0200 2 files changed, 71 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-fs-realpath-async-stale-stat-values.js β Patches applied There are 4 commits in the PR. Attempting autorebase. (node:432) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated. (Use `node --trace-deprecation ...` to show where the warning was created) Rebasing (2/8) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- test: cover `realpathSync` resolving symlinks after a FIFO stathttps://github.com/nodejs/node/actions/runs/31638684564 |
4 participants
fs.realpathSync()can return a path with its symlinks unresolved, depending on what the process happened to stat beforehand.While walking a path, the components already known to be real are skipped:
statValuesis the shared stat buffer, holding the result of the last stat made anywhere in the process. In the intended flow that is thestat()the walk itself made when following a symlink, which is the signal the check wants (added in #13028 sorealpath('/dev/stdin')resolves). But nothing keeps it that way: any unrelated stat overwrites it. If that stat was of a FIFO or a socket, the walk breaks early and the remaining components, including any symlink, are never resolved. The unresolved path is then written to the cache, so every later resolution of it repeats the same answer.It reproduces through public API alone, because the module loader keeps a realpath cache in exactly the state that takes the skipped-component branch:
This is not theoretical. A process that stats a socket or a pipe can make
require()resolve a package through itsnode_modules/<pkg>symlink rather than its realpath, and load a second copy of every module underneath it.I ran into it in the Next.js dev server, which I work on: on a pnpm install it stats a pipe while talking to its worker threads, and from that point
next/dist/...resolves through thenode_modules/nextsymlink, so the process ends up with two copies of theAsyncLocalStorageinstances that hold per-request state, and a request handler reads a store that nothing ever entered.The fix has each walk track whether the symlink it resolved last pointed at a pipe or a socket, taking it from the
stat()that follows the link, which is the value the check was always reading out of the shared buffer. The asyncrealpath()has the same read and is fixed the same way; @mcollina found the shape that reproduces it, which needs a second symlink after the one being resolved so that truncating the walk leaves real work undone.The first commit adds the test and is expected to fail on its own, so you can check it out and see the bug before the second commit fixes it. The third extends that test to assert on
realpathSyncdirectly, and the fourth is the async fix and a test for it. Please squash when landing.Two things to flag:
/dev/stdin, and on Linux that resolves throughpipe:[N], a link target that is not a real path. That path needs CI, or someone on Linux.realpath()callback. An assertion that fails there disappears: no stderr, nouncaughtExceptioneven with a handler installed, and the process exits 0. The first version of that test passed against unfixed Node for that reason. That looks like a separate bug, which I have not looked into.Reproduces on v20.19.6, v22.13.1, v24.16.0 and v25.2.1.