Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

require() no longer follows symlinks after calling statSync() on a FIFO or socket file #51142

Open
oliverfrye opened this issue Dec 13, 2023 · 5 comments
Labels
fs Issues and PRs related to the fs subsystem / file system. loaders Issues and PRs related to ES module loaders

Comments

@oliverfrye
Copy link

Version

v20.10.0

Platform

Linux debian 6.1.0-13-arm64 #1 SMP Debian 6.1.55-1 (2023-09-29) aarch64 GNU/Linux

Subsystem

fs

What steps will reproduce the bug?

This issue was discovered when using pnpm and dependencies downloaded from NPM, but it can be reproduced with some simple shell commands instead.

  1. In lieu of PNPM, create a node_modules structure that emulates pnpm's virtual store technique where dependencies are symlinked from the root of the node_modules into a virtual store directory called .pnpm:
mkdir -p node_modules/.pnpm/top-level-dependency/node_modules/top-level-dependency
echo 'require("transitive-dependency");' > node_modules/.pnpm/top-level-dependency/node_modules/top-level-dependency/index.js
mkdir -p node_modules/.pnpm/top-level-dependency/node_modules/transitive-dependency
touch node_modules/.pnpm/top-level-dependency/node_modules/transitive-dependency/index.js
cd node_modules
ln -s .pnpm/top-level-dependency/node_modules/top-level-dependency
cd ..
  1. Create a FIFO or socket file. Here, it is simple to use the mkfifo utility:
mkfifo fifo
  1. Create two scripts, one of which will demonstrate the bug, and the other which will serve as control:
cat <<EOF > fail.js
require("fs").statSync("fifo");
require("top-level-dependency");
console.log("Success!");
EOF

cat <<EOF > success.js
require("fs");
require("top-level-dependency");
console.log("Success!");
EOF

Note that these files differ only by the removal of the statSync call.

How often does it reproduce? Is there a required condition?

Every time. The only requirement I have found is that the argument to statSync() be either a socket or FIFO file (i.e. a directory or regular file would not cause the issue).

What is the expected behavior? Why is that the expected behavior?

$ node success.js
Success!

This behaviour is expected because Node is supposed to follow symlinks when resolving modules and use the real path when resolving the transitive dependencies of those modules.

What do you see instead?

$ node fail.js
node:internal/modules/cjs/loader:1147
  throw err;
  ^

Error: Cannot find module 'transitive-dependency'
Require stack:
- /home/oliver/node-require-symlink-repro/node_modules/top-level-dependency/index.js
- /home/oliver/node-require-symlink-repro/fail.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1144:15)
    at Module._load (node:internal/modules/cjs/loader:985:27)
    at Module.require (node:internal/modules/cjs/loader:1235:19)
    at require (node:internal/modules/helpers:176:18)
    at Object.<anonymous> (/home/oliver/node-require-symlink-repro/node_modules/top-level-dependency/index.js:1:1)
    at Module._compile (node:internal/modules/cjs/loader:1376:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
    at Module.load (node:internal/modules/cjs/loader:1207:32)
    at Module._load (node:internal/modules/cjs/loader:1023:12)
    at Module.require (node:internal/modules/cjs/loader:1235:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/oliver/node-require-symlink-repro/node_modules/top-level-dependency/index.js',
    '/home/oliver/node-require-symlink-repro/fail.js'
  ]
}

Node.js v20.10.0

Additional information

I have debugging output with NODE_DEBUG that shows that the symlink from the root of the node_modules is not followed into the .pnpm subdirectory as it should in the case that the statSync() call is made.

I have also got strace output that I can share, if that would be of assistance.

My colleagues have been able to reproduce this issue, both on the same Debian OS as me, and on MacOS (i.e. Darwin).

@oliverfrye
Copy link
Author

oliverfrye commented Dec 13, 2023

Some additional information: if you call statSync() against a FIFO, then a second time against a regular file, and then do the require() then the bug is not apparent. For example:

const fs = require("fs");

fs.statSync("fifo");
fs.statSync("/home");
require("top-level-dependency");
console.log("Success!");

Our working theory is that there is some shared or global state that is corrupted when statSync() is invoked against a FIFO or socket file.

@marco-ippolito marco-ippolito added the fs Issues and PRs related to the fs subsystem / file system. label Dec 13, 2023
@anonrig
Copy link
Member

anonrig commented Dec 19, 2023

cc @GeoffreyBooth

@GeoffreyBooth
Copy link
Member

@nodejs/loaders @nodejs/fs

I think first we should narrow down which release of Node (or ideally which commit) introduced the bug. There have been a handful of changes to CommonJS in the last year and some refactors to fs, so it could be either.

@mgabeler-lee-6rs
Copy link

mgabeler-lee-6rs commented Jul 22, 2024

I'm getting killed by this bug on NodeJS 20.x I think. I've stepped through the module require system into the implementation of fs.realpathSync, and this code is firing somehow:

node/lib/fs.js

Lines 2711 to 2718 in 19b6fc9

// 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)) {
break;
}
continue;
}

Specifically it is going into the break when looking at /home, which is very certainly a directory, not a fifo or socket!

I'm not very familiar with the nodejs internals, but I'm inferring from some reading that statValues has info about the last call to (l)stat. But for the first run through this while loop, there is no call to binding.lstat for non-windows platforms. This seems to have been the case since this fifo/socket check was added in [b3d1e3d] in #13028

I think this usually doesn't blow up because other parts of the module loading system tend to have run stat on various files & directories, just before this, but sometimes things go weird.

Attaching strace to my process, it seems like the socket stat triggering this and leading to my particular misery today is from VSCode vue.js tooling, but that just happens to be one case. The code reading the statValues before populating them seems like a general issue, as demonstrated by the OP's original test case.

@RedYetiDev RedYetiDev added loaders Issues and PRs related to ES module loaders repro-exists labels Jul 23, 2024
@RedYetiDev
Copy link
Member

RedYetiDev commented Jul 23, 2024

+ repro-exists (see the linked issue)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. loaders Issues and PRs related to ES module loaders
Projects
None yet
Development

No branches or pull requests

7 participants