From 630702808c595f9ef4dd874c2423c0550db2e480 Mon Sep 17 00:00:00 2001 From: Hendrik Liebau Date: Fri, 7 Aug 2026 15:27:08 +0200 Subject: [PATCH 1/4] test: cover `realpathSync` resolving symlinks after a FIFO stat 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 --- .../test-fs-realpath-stale-stat-values.js | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 test/parallel/test-fs-realpath-stale-stat-values.js diff --git a/test/parallel/test-fs-realpath-stale-stat-values.js b/test/parallel/test-fs-realpath-stale-stat-values.js new file mode 100644 index 000000000000..5017e6b23467 --- /dev/null +++ b/test/parallel/test-fs-realpath-stale-stat-values.js @@ -0,0 +1,48 @@ +'use strict'; + +// Resolving a path must not depend on what was stat'ed before it. +// +// While walking a path, realpath skips the components it already knows are +// real, and in that branch it consulted the shared stat buffer to decide +// whether the walk had reached a pipe or a socket. That buffer holds the result +// of the last stat made anywhere in the process, so an unrelated stat of a FIFO +// made the walk stop early and hand back the path with its symlinks unresolved. +// The unresolved path is then cached, so every later resolution repeats it. +// +// The walk only takes that branch once something has established the ancestors +// as real, which is the state the module loader's realpath cache is in after it +// has resolved anything else under the same directory. So this goes through +// require() to reach it, and the second copy of the module is what the stale +// read costs. + +const common = require('../common'); + +if (common.isWindows) + common.skip('no mkfifo on Windows'); + +const assert = require('assert'); +const fs = require('fs'); +const path = require('path'); +const { execFileSync } = require('child_process'); +const tmpdir = require('../common/tmpdir'); + +tmpdir.refresh(); + +const pkg = tmpdir.resolve('pkg'); +const link = tmpdir.resolve('pkg-link'); +const fifo = tmpdir.resolve('fifo'); + +fs.mkdirSync(pkg); +fs.writeFileSync(path.join(pkg, 'index.js'), 'module.exports = {};\n'); +fs.writeFileSync(tmpdir.resolve('warm.js'), 'module.exports = {};\n'); +fs.symlinkSync('pkg', link); +execFileSync('mkfifo', [fifo]); + +const throughLink = path.join(link, 'index.js'); +const throughReal = path.join(pkg, 'index.js'); + +require(tmpdir.resolve('warm.js')); +fs.statSync(fifo); + +assert.strictEqual(require.resolve(throughLink), throughReal); +assert.strictEqual(require(throughLink), require(throughReal)); From fc87eed6f22075a901c90d6335ed3cc7bf45cd3f Mon Sep 17 00:00:00 2001 From: Hendrik Liebau Date: Fri, 7 Aug 2026 15:27:09 +0200 Subject: [PATCH 2/4] fs: stop reading the shared stat buffer in `realpathSync` `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 --- lib/fs.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 4fbdaf813018..af6172f70f88 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -3254,6 +3254,11 @@ function realpathSync(p, options) { const seenLinks = new SafeMap(); const knownHard = new SafeSet(); const original = p; + // Whether the symlink this walk resolved last pointed at a pipe or a + // socket, which is where the walk stops. It cannot be read back from the + // shared stat buffer, which holds the last stat made anywhere in the + // process rather than the last one made here. + let reachedPipeOrSocket = false; // Current character position in p let pos; @@ -3297,8 +3302,7 @@ function realpathSync(p, options) { // Continue if not a symlink, break if a pipe/socket if (knownHard.has(base) || cache?.get(base) === base) { - if (isFileType(statValues, S_IFIFO) || - isFileType(statValues, S_IFSOCK)) { + if (reachedPipeOrSocket) { break; } continue; @@ -3336,7 +3340,9 @@ function realpathSync(p, options) { } } if (linkTarget === null) { - binding.stat(base, false, undefined, true); + const targetStats = binding.stat(base, false, undefined, true); + reachedPipeOrSocket = isFileType(targetStats, S_IFIFO) || + isFileType(targetStats, S_IFSOCK); linkTarget = binding.readlink(base, undefined); } resolvedLink = pathModule.resolve(previous, linkTarget); From 95f567456696c60f8d3b9130b0436c494d8378cc Mon Sep 17 00:00:00 2001 From: Hendrik Liebau Date: Sat, 8 Aug 2026 16:15:51 +0200 Subject: [PATCH 3/4] test: assert `realpathSync` directly, not only through `require()` 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 --- .../test-fs-realpath-stale-stat-values.js | 37 ++++++++++++++++--- 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/test/parallel/test-fs-realpath-stale-stat-values.js b/test/parallel/test-fs-realpath-stale-stat-values.js index 5017e6b23467..2a5dfdad7a45 100644 --- a/test/parallel/test-fs-realpath-stale-stat-values.js +++ b/test/parallel/test-fs-realpath-stale-stat-values.js @@ -1,3 +1,4 @@ +// Flags: --expose-internals 'use strict'; // Resolving a path must not depend on what was stat'ed before it. @@ -8,12 +9,6 @@ // of the last stat made anywhere in the process, so an unrelated stat of a FIFO // made the walk stop early and hand back the path with its symlinks unresolved. // The unresolved path is then cached, so every later resolution repeats it. -// -// The walk only takes that branch once something has established the ancestors -// as real, which is the state the module loader's realpath cache is in after it -// has resolved anything else under the same directory. So this goes through -// require() to reach it, and the second copy of the module is what the stale -// read costs. const common = require('../common'); @@ -24,6 +19,7 @@ const assert = require('assert'); const fs = require('fs'); const path = require('path'); const { execFileSync } = require('child_process'); +const { realpathCacheKey } = require('internal/fs/utils'); const tmpdir = require('../common/tmpdir'); tmpdir.refresh(); @@ -41,6 +37,35 @@ execFileSync('mkfifo', [fifo]); const throughLink = path.join(link, 'index.js'); const throughReal = path.join(pkg, 'index.js'); +// The walk only skips a component once something has established it as real. A +// cache carrying the ancestors is that state, and it is the state the module +// loader's own cache is in after it has resolved anything else under the +// directory. +function ancestorCache() { + const cache = new Map(); + let dir = ''; + for (const part of tmpdir.path.split(path.sep).slice(1)) { + dir += path.sep + part; + cache.set(dir, dir); + } + return cache; +} + +fs.statSync(path.join(pkg, 'index.js')); +assert.strictEqual( + fs.realpathSync(throughLink, { [realpathCacheKey]: ancestorCache() }), + throughReal, +); + +fs.statSync(fifo); +assert.strictEqual( + fs.realpathSync(throughLink, { [realpathCacheKey]: ancestorCache() }), + throughReal, +); + +// What the stale read costs through the module loader, whose cache puts the +// walk in that same state: the symlink stays unresolved, so the file is loaded +// a second time under a second name. require(tmpdir.resolve('warm.js')); fs.statSync(fifo); From 1deb9b8c1a876a7a036361f1f8d262eb7c1313b2 Mon Sep 17 00:00:00 2001 From: Hendrik Liebau Date: Sat, 8 Aug 2026 16:49:31 +0200 Subject: [PATCH 4/4] fs: stop reading the shared stat buffer in async `realpath` `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 --- lib/fs.js | 13 ++-- ...est-fs-realpath-async-stale-stat-values.js | 62 +++++++++++++++++++ 2 files changed, 71 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-fs-realpath-async-stale-stat-values.js diff --git a/lib/fs.js b/lib/fs.js index af6172f70f88..81c4d2b9c884 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -81,7 +81,6 @@ const { const { FSReqCallback, - statValues, } = binding; const { toPathIfFileURL } = require('internal/url'); const { @@ -3424,6 +3423,11 @@ function realpath(p, options, callback) { const seenLinks = new SafeMap(); const knownHard = new SafeSet(); + // Whether the symlink this walk resolved last pointed at a pipe or a + // socket, which is where the walk stops. It cannot be read back from the + // shared stat buffer, which holds the last stat made anywhere in the + // process rather than the last one made here. + let reachedPipeOrSocket = false; // Current character position in p let pos; @@ -3472,8 +3476,7 @@ function realpath(p, options, callback) { // Continue if not a symlink, break if a pipe/socket if (knownHard.has(base)) { - if (isFileType(statValues, S_IFIFO) || - isFileType(statValues, S_IFSOCK)) { + if (reachedPipeOrSocket) { return callback(null, encodeRealpathResult(p, options)); } return process.nextTick(LOOP); @@ -3503,9 +3506,11 @@ function realpath(p, options, callback) { return gotTarget(null, seenLinks.get(id)); } } - fs.stat(base, (err) => { + fs.stat(base, (err, targetStats) => { if (err) return callback(err); + reachedPipeOrSocket = targetStats.isFIFO() || targetStats.isSocket(); + fs.readlink(base, (err, target) => { if (!isWindows) seenLinks.set(id, target); gotTarget(err, target); diff --git a/test/parallel/test-fs-realpath-async-stale-stat-values.js b/test/parallel/test-fs-realpath-async-stale-stat-values.js new file mode 100644 index 000000000000..83e8af7b3eaf --- /dev/null +++ b/test/parallel/test-fs-realpath-async-stale-stat-values.js @@ -0,0 +1,62 @@ +'use strict'; + +// The async realpath() reads the shared stat buffer the same way realpathSync() +// did, to decide whether the walk has reached a pipe or a socket. The walk's +// own fs.stat() does leave the right value there, but it 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 path used here has two. + +const common = require('../common'); + +if (common.isWindows) + common.skip('no mkfifo on Windows'); + +const assert = require('assert'); +const fs = require('fs'); +const path = require('path'); +const { execFileSync } = require('child_process'); +const tmpdir = require('../common/tmpdir'); + +tmpdir.refresh(); + +const real = tmpdir.resolve('real'); +const pkg = tmpdir.resolve('pkg'); +const fifo = tmpdir.resolve('fifo'); + +fs.mkdirSync(real); +fs.mkdirSync(pkg); +fs.writeFileSync(path.join(real, 'index.js'), ''); +fs.symlinkSync(path.join('..', 'real'), path.join(pkg, 'sub')); +fs.symlinkSync('pkg', tmpdir.resolve('link')); +execFileSync('mkfifo', [fifo]); + +const throughLinks = tmpdir.resolve('link', 'sub', 'index.js'); +const expected = path.join(real, 'index.js'); + +// Keep stats of the FIFO completing for as long as the walk runs, so that one +// of them lands in the buffer during the window. +let settled = false; +(function statFifo() { + if (settled) return; + fs.stat(fifo, statFifo); +})(); + +let error; +let resolvedPath; + +fs.realpath(throughLinks, common.mustCall((err, resolved) => { + settled = true; + error = err; + resolvedPath = resolved; +})); + +// Asserted on exit rather than in the callback. An assertion that fails inside +// this callback is lost: it does not reach an `uncaughtException` handler and +// the process still exits 0, so the test would pass over the bug it covers. +process.on('exit', () => { + assert.ifError(error); + assert.strictEqual(resolvedPath, expected); +});