Repository navigation
glob: pass pattern component as NtQueryDirectoryFile FileName filter on Windows - #28489
Conversation
When iterating a directory during glob walking, pass the current pattern
component as a kernel-side filter to NtQueryDirectoryFile. The filesystem
driver evaluates this via FsRtlIsNameInExpression, so only matching entries
are serialized back to userspace.
The filter is strictly a pre-filter: matchPatternImpl still runs on every
returned entry to handle case sensitivity and 8.3 short-name aliases. We
only emit a filter when the NT match is a guaranteed superset of the glob
match, which means:
- Skip for `*` and `**` (no benefit / would hide subdirectories)
- Skip if the component contains `?` (NT matches one UTF-16 code unit,
glob matches one codepoint, so NT would under-match on surrogate pairs)
- Skip if the component contains `[` `{` `\` `!` (not expressible)
- Skip if it contains `<` `>` `"` (NT wildcards, but invalid in
filenames anyway)
This covers the common cases: `*.ts`, `package.json`, `foo*bar`.
node_modules/*/lib/*.js exercises per-component filtering without `**`, which better demonstrates kernel-side filtering on Windows where each literal and wildcard-literal component can be passed to NtQueryDirectoryFile.
When a FileName filter is passed and matches no entries, NtQueryDirectoryFile returns STATUS_NO_SUCH_FILE on the first call (RestartScan=TRUE) rather than STATUS_NO_MORE_FILES. Both mean "empty result set", not an error.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughWindows-specific directory iteration filtering is added to optimize glob pattern matching. The changes include benchmarks for testing the new pattern, iterator updates supporting name filtering, and logic to compute NT-level filters from glob patterns. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No correctness bugs found, but this touches Windows NT kernel API internals (NtQueryDirectoryFile FileName filter, UNICODE_STRING, @constcast on the filter buffer) and is complex enough to warrant a human reviewer familiar with that layer.
Extended reasoning...
Overview
This PR adds a Windows-specific performance optimization to glob walking: passing the current pattern component as a FileName filter to NtQueryDirectoryFile, letting the filesystem driver pre-filter entries via FsRtlIsNameInExpression before they are serialized to userspace. Three files are touched: a benchmark addition in bench/glob/scan.mjs, iterator plumbing in src/bun.js/node/dir_iterator.zig (new name_filter field, UNICODE_STRING setup, NO_SUCH_FILE handling, setNameFilter), and walker logic in src/glob/GlobWalker.zig (nt_filter_buf, computeNtFilter, filter dispatch in transitionToDirIterState).
Security Risks
No security concerns. The filter is an input-only parameter to a read-only directory enumeration syscall. The @constCast on the filter buffer pointer is benign since NtQueryDirectoryFile does not write to it. No path injection vectors are introduced.
Level of Scrutiny
High. The change involves Windows NT internals: NtQueryDirectoryFile with a FileName filter, UNICODE_STRING lifetime management (stack-allocated filter_us referencing a buffer in the Iterator struct), RestartScan semantics, and the interaction between NT wildcard semantics and glob semantics. The two bugs flagged by the bug hunter are both nits (a conservative UTF-8/UTF-16 length comparison that can never cause overflow, and a dead-code NO_SUCH_FILE branch whose effect is already covered by the io.Information == 0 guard). Neither affects correctness. However, the overall change is non-trivial and deserves review from someone familiar with this subsystem.
Other Factors
The PR description is thorough and the benchmark numbers are credible. The correctness guarantee (pre-filter only; matchPatternImpl always re-runs on returned entries) is sound. The filter lifetime (slice into Iterator.nt_filter_buf, stored in iter_state.directory.iter.name_filter) appears safe given that Iterators are always used via pointers. No existing tests are visible in the diff; Windows-specific glob behavior with NT filters would benefit from a test covering the NO_SUCH_FILE path and non-ASCII patterns.
| .Single, .Double, .Dot, .DotBack => return null, | ||
| else => {}, | ||
| } | ||
|
|
||
| const slice = comp.patternSlice(this.walker.pattern); | ||
| if (slice.len == 0 or slice.len > this.nt_filter_buf.len) return null; | ||
|
|
||
| // Only `*` and literals are safe to lower. Reject anything NT cannot | ||
| // express (`[` `{` `\` `!`) or where NT semantics under-match glob | ||
| // (`?` matches one UTF-16 code unit, glob matches one codepoint). | ||
| // `<` `>` `"` are NT wildcards; treating them as literals would over-match, | ||
| // but they are invalid in Windows filenames so such a pattern never matches | ||
| // anyway. |
There was a problem hiding this comment.
🟡 The size guard in computeNtFilter compares the UTF-8 byte length of the pattern slice against nt_filter_buf.len (256 u16 elements), causing the NT filter optimization to be silently skipped for non-ASCII patterns whose UTF-8 byte count exceeds 256 even though their UTF-16 representation would fit in the buffer. The fix is to compare against the actual UTF-16 code unit count (e.g., via bun.strings.utf16Len) rather than the raw byte length.
Extended reasoning...
The Bug
In computeNtFilter (src/glob/GlobWalker.zig, around line 745), the guard reads:
if (slice.len == 0 or slice.len > this.nt_filter_buf.len) return null;slice is a UTF-8 byte slice returned by comp.patternSlice(this.walker.pattern), so slice.len is a count of UTF-8 bytes. this.nt_filter_buf is [256]u16, so .len is 256 — a count of UTF-16 code units. The two units are not the same for non-ASCII patterns.
Code Path
computeNtFilter is called from transitionToDirIterState on every directory transition on Windows. For each pattern component that passes the syntax check, it hits this guard before attempting the UTF-8→UTF-16 conversion via bun.strings.convertUTF8toUTF16InBuffer.
Why the existing guard is incorrect (but safe)
For all valid Unicode, the number of UTF-16 code units is always ≤ the number of UTF-8 bytes:
- 1-byte UTF-8 → 1 UTF-16 code unit (ASCII)
- 2-byte UTF-8 → 1 UTF-16 code unit (e.g., Cyrillic U+0400–U+04FF, Latin Extended)
- 3-byte UTF-8 → 1 UTF-16 code unit (e.g., most CJK)
- 4-byte UTF-8 → 2 UTF-16 code units (surrogate pair)
This means the guard can never cause a buffer overflow — if slice.len ≤ 256, the UTF-16 output is also ≤ 256 code units. However, it is overly conservative: it can reject patterns that would actually fit in the buffer.
Step-by-step Proof
Consider a glob pattern component made entirely of Cyrillic characters, each encoded as a 2-byte UTF-8 sequence (e.g., U+0410 = \xD0\x90):
- 128 Cyrillic characters:
slice.len = 256, UTF-16 length = 128. The guard evaluates256 > 256→false→ passes (correctly allows the filter). - 129 Cyrillic characters:
slice.len = 258, UTF-16 length = 129. The guard evaluates258 > 256→true→ returns null, skipping the NT filter optimization even though the UTF-16 output (129 code units) fits comfortably in the 256-element buffer.
For a 200-character Cyrillic pattern: slice.len = 400 > 256, so the guard skips the filter. The UTF-16 output would be 200 code units — well within the buffer.
Impact
Purely a missed optimization. matchPatternImpl still runs on every entry returned by NtQueryDirectoryFile, so glob results are always correct. The NT filter is a pre-filter that reduces the number of entries the kernel serializes to userspace. For non-ASCII patterns on Windows that have multi-byte UTF-8 encodings and are longer than 128 characters (for 2-byte sequences) but shorter than 257 UTF-16 code units, the optimization is silently skipped, costing some performance.
Fix
Compare against the actual UTF-16 length before conversion. One approach:
const utf16_len = bun.strings.utf16Len(slice);
if (utf16_len == 0 or utf16_len > this.nt_filter_buf.len) return null;Alternatively, run the conversion first and check the result length, discarding it if it exceeds the buffer.
| }; | ||
| } | ||
|
|
||
| if (rc == .NO_MORE_FILES) { | ||
| // NO_SUCH_FILE is returned on the first call when a FileName filter | ||
| // matches nothing; NO_MORE_FILES on subsequent calls. Both mean "done". | ||
| if (rc == .NO_MORE_FILES or rc == .NO_SUCH_FILE) { | ||
| bun.sys.syslog("NtQueryDirectoryFile({f}) = {s}", .{ self.dir, @tagName(rc) }); | ||
| self.end_index = self.index; | ||
| return .{ .result = null }; |
There was a problem hiding this comment.
🟡 The new rc == .NO_SUCH_FILE branch (line 271) is dead code in its primary intended scenario because the if (io.Information == 0) early-return at line 252 fires first. When NtQueryDirectoryFile returns STATUS_NO_SUCH_FILE (filter matched nothing on first call), the kernel writes 0 bytes to the buffer so io.Information == 0, causing the earlier guard to return null with a misleading syslog message of = 0 rather than = NO_SUCH_FILE. The fix would be to reorder the checks so status codes are evaluated before the io.Information == 0 guard.
Extended reasoning...
When NtQueryDirectoryFile returns STATUS_NO_SUCH_FILE on the first call (because a FileName filter matched no entries), the NT kernel writes zero bytes to the output buffer, setting io.Information to 0. This is standard NT behavior for synchronous file handles as Bun uses.
The check order in the Windows next() function is: (1) if (io.Information == 0) fires at line 252 returning null with syslog = 0, then (2) self.index/end_index are set, (3) INVALID_PARAMETER check, and finally (4) the new NO_MORE_FILES or NO_SUCH_FILE check at line 271. When STATUS_NO_SUCH_FILE is returned, step 1 fires immediately because io.Information == 0, so the new explicit NO_SUCH_FILE check at step 4 is never reached.
The io.Information == 0 guard is a reasonable early-exit optimization, but it was placed before the status-code checks. Since STATUS_NO_SUCH_FILE always (in the primary case) results in io.Information == 0, the io.Information path absorbs the return before the newly added explicit status check can run.
Functional correctness is unaffected — both paths return null. However, the debug syslog emits NtQueryDirectoryFile(fd) = 0 instead of = NO_SUCH_FILE, which is misleading when diagnosing why a filtered glob found no results. The PR description specifically claims this PR handles STATUS_NO_SUCH_FILE, but the actual handling was already provided by the pre-existing io.Information == 0 path.
The refutation argues this is intentional defensive programming for the hypothetical case where a filesystem driver returns STATUS_NO_SUCH_FILE with io.Information > 0. While theoretically possible with non-standard drivers, it is undocumented and not observed in practice. By contrast, the analogous NO_MORE_FILES check IS reachable in normal operation when the last batch of entries partially fills the buffer. The NO_SUCH_FILE case lacks this justification since it only occurs on the very first call. The defensive-programming argument is weak here because the code already correctly handles NO_SUCH_FILE via the io.Information == 0 path.
Step-by-step proof: (1) Glob pattern *.target used; computeNtFilter returns .target as NT filter. (2) NtQueryDirectoryFile called with RestartScan=TRUE and FileName=.target. (3) No files match; kernel writes 0 bytes to buffer, sets io.Information = 0, returns STATUS_NO_SUCH_FILE. (4) self.first = false is set. (5) if (io.Information == 0) is true, so syslog prints NtQueryDirectoryFile(fd) = 0 and return .{ .result = null } is executed. (6) Lines 268-274 (the NO_SUCH_FILE check) are never reached. (7) Syslog shows = 0 rather than = NO_SUCH_FILE.
The fix is to move the status-code checks (NO_MORE_FILES, NO_SUCH_FILE, INVALID_PARAMETER) before the io.Information == 0 guard, or merge the NO_SUCH_FILE branch into the io.Information == 0 path so both the correct syslog message and the early return are emitted together.
| var filter_us: w.UNICODE_STRING = undefined; | ||
| const filter_ptr: ?*w.UNICODE_STRING = if (self.name_filter) |f| blk: { | ||
| filter_us = .{ | ||
| .Length = @intCast(f.len * 2), |
There was a problem hiding this comment.
This is byte length?
PR #28496 replaced the single component_idx with an active BitSet, but the Windows NtQueryDirectoryFile filter call from PR #28489 still referenced the old variable name. computeNtFilter operates on a single pattern component, so apply the kernel filter only when exactly one index is active. For multi-index states (e.g. after **), skip the filter to avoid hiding entries needed by other indices. The filter is a pre-filter optimization; matchPatternImpl still runs for correctness.
PR #28496 replaced the single component_idx with an active BitSet, but the Windows NtQueryDirectoryFile filter call from PR #28489 still referenced the old variable name. computeNtFilter operates on a single pattern component, so apply the kernel filter only when exactly one index is active. For multi-index states (e.g. after **), skip the filter to avoid hiding entries needed by other indices. The filter is a pre-filter optimization; matchPatternImpl still runs for correctness.
## What Fixes a build error on `main` introduced by the interaction of #28496 and #28489: ``` src/glob/GlobWalker.zig:715:69: error: use of undeclared identifier 'component_idx' iterator.setNameFilter(this.computeNtFilter(component_idx)); ^~~~~~~~~~~~~ ``` ## Why \#28496 replaced the single `component_idx` variable with an `active` BitSet to track multiple active pattern component indices. \#28489 (merged just before) added a Windows `NtQueryDirectoryFile` filter call that references `component_idx`. After both landed, the variable no longer exists in scope. ## Fix `computeNtFilter` operates on a single pattern component, so it can only be applied when exactly one index is active. For multi-index states (e.g. after `**`), a single-component kernel filter could hide entries that other active indices need to match, so we skip it. This is safe because the NT filter is purely a pre-filter optimization — `matchPatternImpl` still runs on every returned entry for correctness (per the existing doc comment on `computeNtFilter`). ## Verification `bun run zig:check-all` passes on all platforms (macOS/Linux/Windows × x86_64/aarch64 × Debug/Release).
…on Windows (oven-sh#28489) When iterating a directory during glob walking on Windows, pass the current pattern component as a kernel-side `FileName` filter to `NtQueryDirectoryFile`. The filesystem driver evaluates it via `FsRtlIsNameInExpression`, so non-matching entries are never serialized to userspace. The filter is purely a pre-filter — `matchPatternImpl` still runs on every returned entry to handle case sensitivity and 8.3 short-name aliases. A filter is only emitted when the NT match is guaranteed to be a superset of the glob match: - skipped for `*` and `**` (no benefit / would hide subdirectories needed for recursion) - skipped for components containing `?` (NT matches one UTF-16 code unit, glob matches one codepoint — would under-match on surrogate pairs) - skipped for components containing `[` `{` `\` `!` `<` `>` `"` (not expressible / NT wildcards) Also handles `STATUS_NO_SUCH_FILE`, which `NtQueryDirectoryFile` returns on the first call when a filter matches nothing (previously only `STATUS_NO_MORE_FILES` was handled). ### Benchmark Synthetic test: 5,000 files per directory, 5 matching (0.1% match ratio). Windows 11, AMD Ryzen AI 9 HX 370. | Pattern | Before (min) | After (min) | Speedup | |---|---|---|---| | `*.target` (1 dir × 5000 files) | 1.22 ms | 0.51 ms | **2.39×** | | `pkg-*/*.target` (20 dirs × 5000) | 36.95 ms | 19.84 ms | **1.86×** | | `**/*.target` (control — filter skipped) | 36.54 ms | 36.52 ms | unchanged | Real-world `bench/glob/scan.mjs` over `bench/node_modules`: ~10-12% faster on non-`**` patterns, unchanged on `**` patterns.
…#28543) ## What Fixes a build error on `main` introduced by the interaction of oven-sh#28496 and oven-sh#28489: ``` src/glob/GlobWalker.zig:715:69: error: use of undeclared identifier 'component_idx' iterator.setNameFilter(this.computeNtFilter(component_idx)); ^~~~~~~~~~~~~ ``` ## Why \oven-sh#28496 replaced the single `component_idx` variable with an `active` BitSet to track multiple active pattern component indices. \oven-sh#28489 (merged just before) added a Windows `NtQueryDirectoryFile` filter call that references `component_idx`. After both landed, the variable no longer exists in scope. ## Fix `computeNtFilter` operates on a single pattern component, so it can only be applied when exactly one index is active. For multi-index states (e.g. after `**`), a single-component kernel filter could hide entries that other active indices need to match, so we skip it. This is safe because the NT filter is purely a pre-filter optimization — `matchPatternImpl` still runs on every returned entry for correctness (per the existing doc comment on `computeNtFilter`). ## Verification `bun run zig:check-all` passes on all platforms (macOS/Linux/Windows × x86_64/aarch64 × Debug/Release).
…on Windows (oven-sh#28489) When iterating a directory during glob walking on Windows, pass the current pattern component as a kernel-side `FileName` filter to `NtQueryDirectoryFile`. The filesystem driver evaluates it via `FsRtlIsNameInExpression`, so non-matching entries are never serialized to userspace. The filter is purely a pre-filter — `matchPatternImpl` still runs on every returned entry to handle case sensitivity and 8.3 short-name aliases. A filter is only emitted when the NT match is guaranteed to be a superset of the glob match: - skipped for `*` and `**` (no benefit / would hide subdirectories needed for recursion) - skipped for components containing `?` (NT matches one UTF-16 code unit, glob matches one codepoint — would under-match on surrogate pairs) - skipped for components containing `[` `{` `\` `!` `<` `>` `"` (not expressible / NT wildcards) Also handles `STATUS_NO_SUCH_FILE`, which `NtQueryDirectoryFile` returns on the first call when a filter matches nothing (previously only `STATUS_NO_MORE_FILES` was handled). ### Benchmark Synthetic test: 5,000 files per directory, 5 matching (0.1% match ratio). Windows 11, AMD Ryzen AI 9 HX 370. | Pattern | Before (min) | After (min) | Speedup | |---|---|---|---| | `*.target` (1 dir × 5000 files) | 1.22 ms | 0.51 ms | **2.39×** | | `pkg-*/*.target` (20 dirs × 5000) | 36.95 ms | 19.84 ms | **1.86×** | | `**/*.target` (control — filter skipped) | 36.54 ms | 36.52 ms | unchanged | Real-world `bench/glob/scan.mjs` over `bench/node_modules`: ~10-12% faster on non-`**` patterns, unchanged on `**` patterns.
…#28543) ## What Fixes a build error on `main` introduced by the interaction of oven-sh#28496 and oven-sh#28489: ``` src/glob/GlobWalker.zig:715:69: error: use of undeclared identifier 'component_idx' iterator.setNameFilter(this.computeNtFilter(component_idx)); ^~~~~~~~~~~~~ ``` ## Why \oven-sh#28496 replaced the single `component_idx` variable with an `active` BitSet to track multiple active pattern component indices. \oven-sh#28489 (merged just before) added a Windows `NtQueryDirectoryFile` filter call that references `component_idx`. After both landed, the variable no longer exists in scope. ## Fix `computeNtFilter` operates on a single pattern component, so it can only be applied when exactly one index is active. For multi-index states (e.g. after `**`), a single-component kernel filter could hide entries that other active indices need to match, so we skip it. This is safe because the NT filter is purely a pre-filter optimization — `matchPatternImpl` still runs on every returned entry for correctness (per the existing doc comment on `computeNtFilter`). ## Verification `bun run zig:check-all` passes on all platforms (macOS/Linux/Windows × x86_64/aarch64 × Debug/Release).
3 participants
When iterating a directory during glob walking on Windows, pass the current
pattern component as a kernel-side
FileNamefilter toNtQueryDirectoryFile.The filesystem driver evaluates it via
FsRtlIsNameInExpression, so non-matchingentries are never serialized to userspace.
The filter is purely a pre-filter —
matchPatternImplstill runs on everyreturned entry to handle case sensitivity and 8.3 short-name aliases. A filter
is only emitted when the NT match is guaranteed to be a superset of the glob
match:
*and**(no benefit / would hide subdirectories needed for recursion)?(NT matches one UTF-16 code unit, glob matches one codepoint — would under-match on surrogate pairs)[{\!<>"(not expressible / NT wildcards)Also handles
STATUS_NO_SUCH_FILE, whichNtQueryDirectoryFilereturns on thefirst call when a filter matches nothing (previously only
STATUS_NO_MORE_FILESwas handled).
Benchmark
Synthetic test: 5,000 files per directory, 5 matching (0.1% match ratio).
Windows 11, AMD Ryzen AI 9 HX 370.
*.target(1 dir × 5000 files)pkg-*/*.target(20 dirs × 5000)**/*.target(control — filter skipped)Real-world
bench/glob/scan.mjsoverbench/node_modules: ~10-12% faster onnon-
**patterns, unchanged on**patterns.