Conversation
…or walk walk_html_files previously recurse into directories without limit and followed symlinks unconditionally. On a filesystem with symlink cycles this would loop forever; on an unusually deep directory tree it risked a stack overflow. Changes: - Introduce MAX_DIR_DEPTH = 50 constant. The walk returns early (without error) once this depth is exceeded. - Split walk_html_files into a thin public wrapper and a private walk_html_files_inner that carries the depth counter. - Skip symlinks via path.is_symlink() before recursing, breaking any cycle at its first back-edge. Updated DESIGN.md (from_patterns section) and README.md (Locator setup section) to document the 50-level depth limit and symlink behaviour. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds safeguards to @microsoft/fast-build’s Locator filesystem scan to avoid infinite recursion on symlink cycles and cap recursion depth, with accompanying documentation updates.
Changes:
- Add
MAX_DIR_DEPTHand a depth-tracking inner walk to bound recursive directory traversal. - Skip symlink entries during the directory walk to avoid symlink-cycle loops.
- Document the depth limit and symlink behavior in README and DESIGN docs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/microsoft-fast-build/src/locator.rs | Introduces a max-depth guard and symlink skipping in the recursive HTML file walker. |
| crates/microsoft-fast-build/README.md | Documents the recursion depth limit and symlink-skipping behavior for Locator::from_patterns. |
| crates/microsoft-fast-build/DESIGN.md | Updates from_patterns design notes to reflect the bounded walk and symlink handling. |
- Use entry.file_type()? instead of path.is_symlink() + path.is_dir() to avoid extra syscalls and TOCTOU window - Add locator_walk test suite with 4 tests: - depth within limit is discovered (depth 10) - depth beyond limit is NOT discovered (depth 52) - symlinked directories are skipped - symlink cycles terminate without hanging Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
📖 Description
walk_html_filesinlocator.rspreviously recursed into directories without any depth limit and followed symlinks unconditionally. On a filesystem with symlink cycles this would loop forever; on an unusually deep directory tree it risked a stack overflow.MAX_DIR_DEPTH = 50. The walk returns silently (without error) once this depth is exceeded.walk_html_filesinto a thin public wrapper and a privatewalk_html_files_innerthat carries the current depth counter.path.is_symlink()before recursing, breaking any cycle at its first back-edge.DESIGN.md (
from_patternssection) and README.md (Locator setup section) updated to document the 50-level limit and symlink behaviour.📑 Test Plan
All existing tests pass. The locator tests exercise glob pattern scanning across the
tests/fixtures/directory.✅ Checklist
General
$ npm run change