diff --git a/lib/fs.js b/lib/fs.js index 4fbdaf813018..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 { @@ -3254,6 +3253,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 +3301,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 +3339,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); @@ -3418,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; @@ -3466,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); @@ -3497,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); +}); 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..2a5dfdad7a45 --- /dev/null +++ b/test/parallel/test-fs-realpath-stale-stat-values.js @@ -0,0 +1,73 @@ +// Flags: --expose-internals +'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. + +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 { realpathCacheKey } = require('internal/fs/utils'); +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'); + +// 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); + +assert.strictEqual(require.resolve(throughLink), throughReal); +assert.strictEqual(require(throughLink), require(throughReal));