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

Infinite event loop after calling fs.realpath on 16-inch Macbook #33936

Closed
MMhunter opened this issue Jun 18, 2020 · 1 comment
Closed

Infinite event loop after calling fs.realpath on 16-inch Macbook #33936

MMhunter opened this issue Jun 18, 2020 · 1 comment
Labels
fs Issues and PRs related to the fs subsystem / file system.

Comments

@MMhunter
Copy link

  • Version:
    10, 12 and Latest
  • Platform:
    MacOS 10.15.5 (16-inch, 2019)

What steps will reproduce the bug?

just call fs.realpath with /usr/bin/tclsh as first parameter.

require('fs').realpath('/usr/bin/tclsh', console.log)

image

Callback will not be executed, and the node process will be in an infinite event loop. (Not dead, but having high cpu usage).

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

Every time.

What is the expected behavior?

Callback should be executed as expected.

Additional information

Little inspecting into this issue:

The link path for /usr/bin/tclsh:

/usr/bin/tclsh -> usr/bin/tclsh8.5 -> /System/Library/Frameworks/Tcl.framework/Versions/8.5/tclsh8.5

In the fs.js, the realpath function attempts to use seenLinks to store symbolicLinks that have been already visited.
image

However, on MacOS 10.15.5, 16-inch MacOS, calling node's fs.lstat on /usr/bin/tclsh and usr/bin/tclsh8.5 share the same dev and ino number. This means that the seenLinks for id of usr/bin/tclsh8.5 will target to itself, then the infinite loop happens.

image

Another interesting thing, it looks like the fs.lstat won't get the same number with terminal command stat on this system. (But they are the same on older MacBook)

image

Btw, fs.realpath.native works well.

image

@bnoordhuis bnoordhuis added the fs Issues and PRs related to the fs subsystem / file system. label Jun 18, 2020
@bnoordhuis
Copy link
Member

Oh dear, that looks like another instance of node using doubles to represent inode numbers, and losing precision in the process. It should be easy to fix by switching over to bigints:

diff --git a/lib/fs.js b/lib/fs.js
index 8d2a1e044e..5319b2d375 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -1608,7 +1608,7 @@ function realpathSync(p, options) {
   // On windows, check that the root exists. On unix there is no need.
   if (isWindows && !knownHard[base]) {
     const ctx = { path: base };
-    binding.lstat(pathModule.toNamespacedPath(base), false, undefined, ctx);
+    binding.lstat(pathModule.toNamespacedPath(base), true, undefined, ctx);
     handleErrorFromBinding(ctx);
     knownHard[base] = true;
   }
@@ -1650,7 +1650,7 @@ function realpathSync(p, options) {
 
       const baseLong = pathModule.toNamespacedPath(base);
       const ctx = { path: base };
-      const stats = binding.lstat(baseLong, false, undefined, ctx);
+      const stats = binding.lstat(baseLong, true, undefined, ctx);
       handleErrorFromBinding(ctx);
 
       if (!isFileType(stats, S_IFLNK)) {
@@ -1673,7 +1673,7 @@ function realpathSync(p, options) {
       }
       if (linkTarget === null) {
         const ctx = { path: base };
-        binding.stat(baseLong, false, undefined, ctx);
+        binding.stat(baseLong, true, undefined, ctx);
         handleErrorFromBinding(ctx);
         linkTarget = binding.readlink(baseLong, undefined, undefined, ctx);
         handleErrorFromBinding(ctx);
@@ -1694,7 +1694,7 @@ function realpathSync(p, options) {
     // On windows, check that the root exists. On unix there is no need.
     if (isWindows && !knownHard[base]) {
       const ctx = { path: base };
-      binding.lstat(pathModule.toNamespacedPath(base), false, undefined, ctx);
+      binding.lstat(pathModule.toNamespacedPath(base), true, undefined, ctx);
       handleErrorFromBinding(ctx);
       knownHard[base] = true;
     }

(note: that's for fs.realpathSync(). fs.realpath() needs updating too.)

That inode number, 1152921500312396900, is well beyond the range of numbers that a 64 bits floating point number can accurately represent (which ends at 2**53.)

Am I right that stat -x /usr/bin/tclsh and stat -x /usr/bin/tclsh8.5 print different inos?

lundibundi added a commit to lundibundi/node that referenced this issue Jun 18, 2020
The `fs.realpath` / `fs.realpathSync` cache already seen symbolic links
using the inode number which may be longer that max supported
JS number (2**53) and will therefore be incorrectly handled by possibly
entering infinite loop of calling stat on the same node.

This PR changes those functions (where appropriate) to use
bigint for inode numbers.

Fixes: nodejs#33936
codebytere pushed a commit that referenced this issue Jun 27, 2020
The `fs.realpath` / `fs.realpathSync` cache already seen symbolic links
using the inode number which may be longer that max supported
JS number (2**53) and will therefore be incorrectly handled by possibly
entering infinite loop of calling stat on the same node.

This PR changes those functions (where appropriate) to use
bigint for inode numbers.

Fixes: #33936

PR-URL: #33945
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this issue Jun 30, 2020
The `fs.realpath` / `fs.realpathSync` cache already seen symbolic links
using the inode number which may be longer that max supported
JS number (2**53) and will therefore be incorrectly handled by possibly
entering infinite loop of calling stat on the same node.

This PR changes those functions (where appropriate) to use
bigint for inode numbers.

Fixes: #33936

PR-URL: #33945
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this issue Jul 10, 2020
The `fs.realpath` / `fs.realpathSync` cache already seen symbolic links
using the inode number which may be longer that max supported
JS number (2**53) and will therefore be incorrectly handled by possibly
entering infinite loop of calling stat on the same node.

This PR changes those functions (where appropriate) to use
bigint for inode numbers.

Fixes: #33936

PR-URL: #33945
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this issue Jul 12, 2020
The `fs.realpath` / `fs.realpathSync` cache already seen symbolic links
using the inode number which may be longer that max supported
JS number (2**53) and will therefore be incorrectly handled by possibly
entering infinite loop of calling stat on the same node.

This PR changes those functions (where appropriate) to use
bigint for inode numbers.

Fixes: #33936

PR-URL: #33945
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this issue Jul 14, 2020
The `fs.realpath` / `fs.realpathSync` cache already seen symbolic links
using the inode number which may be longer that max supported
JS number (2**53) and will therefore be incorrectly handled by possibly
entering infinite loop of calling stat on the same node.

This PR changes those functions (where appropriate) to use
bigint for inode numbers.

Fixes: #33936

PR-URL: #33945
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants