fs: fix realpath leaving symlinks unresolved after an unrelated stat

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

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