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 <mail@hendrik-liebau.de>
PR-URL: #65113
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Author: unstubbable

lib/fs.js

@@ -84,7 +84,6 @@ const {
 
 const {
   FSReqCallback,
-  statValues,
 } = binding;
 const { toPathIfFileURL } = require('internal/url');
 const {
@@ -2787,6 +2786,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;
@@ -2830,8 +2834,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;
@@ -2869,7 +2872,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);
@@ -2941,6 +2946,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;
@@ -2989,8 +2999,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);
@@ -3020,9 +3029,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);

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);
+});

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));