Skip to content

vfs: prevent stack overflow in recursive readdir on circular symlinks - #64149

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
AkshatOP:fix-vfs-readdir-circular-symlink
Jul 21, 2026
Merged

vfs: prevent stack overflow in recursive readdir on circular symlinks#64149
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
AkshatOP:fix-vfs-readdir-circular-symlink

Conversation

@AkshatOP

Copy link
Copy Markdown
Contributor

MemoryProvider#readdirSync with recursive: true follows symlinks to directories during traversal but did not bound the number of symlinks followed along a branch. A circular symlink therefore caused unbounded recursion in the internal walk() helper until the call stack was exhausted, crashing the process with RangeError: Maximum call stack size exceeded. Both the synchronous and promise-based variants were affected. The existing kMaxSymlinkDepth guard in #lookupEntry did not help, because walk() resolved each symlink target with a fresh depth of zero.

Track the number of symlink hops along the current branch and stop recursing once it would exceed kMaxSymlinkDepth, mirroring the ELOOP guard in #lookupEntry and the behavior of the real filesystem, which follows directory symlinks until the OS symlink limit is reached. The entries themselves are still listed, so non-circular symlinks continue to be followed as before.

Fixes: #64148

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. vfs Issues and PRs related to the virtual filesystem subsystem. labels Jun 26, 2026
Comment threadlib/internal/vfs/providers/memory.js Outdated
@@ -612,7 +612,7 @@ class MemoryProvider extends VirtualProvider {
#readdirRecursive(dirEntry, dirPath, withFileTypes) {
const results = [];

const walk = (entry, currentPath, relativePath) => {
const walk = (entry, currentPath, relativePath, symlinkDepth) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of keeping this recursive, can it be refactored to be iterative to avoid the risk entirely?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alrightt... I switched #readdirRecursive to an iterative traversal with an explicit queue (same shape as fs.readdirSyncRecursive), so neither circular symlinks nor deeply nested trees can blow the call stack now.

I kept a per-entry symlink-hop counter bounded by kMaxSymlinkDepth, since the walk still follows directory symlinks and a cycle would otherwise grow the queue without end. It stops at the same point the real FS does (ELOOP) — the self-referential PoC returns the same 42 entries as fs.readdirSync(path, { recursive: true }). I went with the depth bound over a visited set to keep the output in parity with real fs, but happy to switch if you'd rather.

@AkshatOP
AkshatOPforce-pushed the fix-vfs-readdir-circular-symlink branch from 62229c0 to 2e4e1beCompareJune 27, 2026 03:56
@jojin1709

Copy link
Copy Markdown

Thanks for the fix! I can confirm the iterative approach is the right call —
avoids both the stack overflow and keeps output parity with real fs.
The queue-based traversal with symlink depth bound is clean.

@AkshatOP

Copy link
Copy Markdown
ContributorAuthor

friendly ping! anything else you'd like changed here?
@jasnell

@AkshatOP
AkshatOP requested a review from jasnellJune 29, 2026 18:35
MemoryProvider recursive readdir walked the directory tree with a
recursive helper. Rewrite it to traverse iteratively with an explicit
stack so a deeply nested tree can no longer exhaust the call stack.
The set of directories on the active traversal path is still tracked, so
a circular symlink stops descending while its entry remains listed; the
output and observable behavior are unchanged.
Refs: nodejs#64168
Signed-off-by: AkshatOP <hunterdevil0987@gmail.com>
@AkshatOP
AkshatOPforce-pushed the fix-vfs-readdir-circular-symlink branch from 2e4e1be to 56ef7a4CompareJuly 5, 2026 06:46
@AkshatOP

Copy link
Copy Markdown
ContributorAuthor

Updated this PR per @jasnell's preference for an iterative walk (raised here and on #64168: "I'd much prefer to see if there's a way to make the walk iterative rather than recursive").

It's now a pure recursive→iterative refactor on top of the landed #64168 , #readdirRecursive uses an explicit stack, so deeply nested trees can't exhaust the call stack. The active-path cycle handling is unchanged, so all the tests from #64168 pass as-is and the output is identical (the self-referential symlink still returns ['dir','dir/loop','dir/nested.txt']). No behavior change, just no recursion. Happy to adjust anything.

@jasnell
jasnell requested a review from mcollinaJuly 7, 2026 03:27
@jasnell

Copy link
Copy Markdown
Member

@mcollina ... can you take a look?

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test?

@AkshatOP

Copy link
Copy Markdown
ContributorAuthor

@mcollina Added the test

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollinamcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 9, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 9, 2026
@nodejs-github-bot

This comment was marked as outdated.

@codecov

codecovBot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.23%. Comparing base (b59def5) to head (b0e0986).
⚠️ Report is 152 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #64149 +/- ##
==========================================
- Coverage 92.04% 90.23% -1.82% 
==========================================
Files 381 741 +360 Lines 169293 240991 +71698 Branches 25948 45401 +19453 ==========================================
+ Hits 155826 217451 +61625 - Misses 13177 15111 +1934 - Partials 290 8429 +8139 
Files with missing linesCoverage Δ
lib/internal/vfs/providers/memory.js95.06% <100.00%> (+0.48%)⬆️

... and 495 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@AkshatOP
AkshatOP requested a review from mcollinaJuly 14, 2026 20:22
@AkshatOP

Copy link
Copy Markdown
ContributorAuthor

didn't mean to ping but i don't know the process after this , is this approved for a merge or do it need changes

@trivikrtrivikr added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 21, 2026
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 21, 2026
@nodejs-github-bot
nodejs-github-bot merged commit 4bec191 into nodejs:mainJul 21, 2026
79 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 4bec191

aduh95 pushed a commit that referenced this pull request Jul 21, 2026
MemoryProvider recursive readdir walked the directory tree with a
recursive helper. Rewrite it to traverse iteratively with an explicit
stack so a deeply nested tree can no longer exhaust the call stack.
The set of directories on the active traversal path is still tracked, so
a circular symlink stops descending while its entry remains listed; the
output and observable behavior are unchanged.
Refs: #64168
Signed-off-by: AkshatOP <hunterdevil0987@gmail.com>
PR-URL: #64149Fixes: #64148
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
aduh95 pushed a commit that referenced this pull request Jul 21, 2026
MemoryProvider recursive readdir walked the directory tree with a
recursive helper. Rewrite it to traverse iteratively with an explicit
stack so a deeply nested tree can no longer exhaust the call stack.
The set of directories on the active traversal path is still tracked, so
a circular symlink stops descending while its entry remains listed; the
output and observable behavior are unchanged.
Refs: #64168
Signed-off-by: AkshatOP <hunterdevil0987@gmail.com>
PR-URL: #64149Fixes: #64148
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
aduh95 pushed a commit that referenced this pull request Jul 21, 2026
MemoryProvider recursive readdir walked the directory tree with a
recursive helper. Rewrite it to traverse iteratively with an explicit
stack so a deeply nested tree can no longer exhaust the call stack.
The set of directories on the active traversal path is still tracked, so
a circular symlink stops descending while its entry remains listed; the
output and observable behavior are unchanged.
Refs: #64168
Signed-off-by: AkshatOP <hunterdevil0987@gmail.com>
PR-URL: #64149Fixes: #64148
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jul 29, 2026
MemoryProvider recursive readdir walked the directory tree with a
recursive helper. Rewrite it to traverse iteratively with an explicit
stack so a deeply nested tree can no longer exhaust the call stack.
The set of directories on the active traversal path is still tracked, so
a circular symlink stops descending while its entry remains listed; the
output and observable behavior are unchanged.
Refs: #64168
Signed-off-by: AkshatOP <hunterdevil0987@gmail.com>
PR-URL: #64149Fixes: #64148
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.vfsIssues and PRs related to the virtual filesystem subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

node:vfs MemoryProvider: readdirSync({recursive:true}) crashes via circular symlinks (stack overflow)

6 participants

@AkshatOP@jojin1709@jasnell@nodejs-github-bot@mcollina@trivikr