glob: bound-check PathBuffer copies in GlobWalker - #28836
Conversation
GlobWalker.next() builds up arena-allocated work-item paths via join() and later copies them back into fixed-size bun.PathBuffer scratch buffers. For deeply nested trees or self-referential symlinks (with followSymlinks) the accumulated path can exceed MAX_PATH_BYTES, and the unchecked memcpy writes past the buffer. Add length checks at the four copy sites in GlobWalker.zig (Iterator.init, transitionToDirIterState, the symlink branch in next(), and handleSysErrWithPath) so the walker surfaces ENAMETOOLONG instead of scribbling past its PathBuffer. This also terminates symlink loops that would otherwise grow the work-item path unboundedly.
|
Updated 7:04 PM PT - Apr 6th, 2026
❌ @dylan-conway, your commit 1baf6c9 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 28836That installs a local version of the PR into your bun-28836 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughAdded explicit buffer-length guards in Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/glob/path-length.test.ts`:
- Line 2: Replace usage of tmpdirSync with the tempDir helper from the harness:
update the import to pull tempDir instead of tmpdirSync (the import line that
currently references tmpdirSync) and change each test that calls tmpdirSync to
call tempDir() to obtain a disposable directory object, then use its path and
rely on its automatic cleanup; update references at the locations that use
tmpdirSync (around the tests identified at lines ~24 and ~61) to use the tempDir
instance's directory path API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c64cd230-80a0-4713-a8f2-857187d69466
📒 Files selected for processing (2)
src/glob/GlobWalker.zigtest/js/bun/glob/path-length.test.ts
| @@ -0,0 +1,91 @@ | |||
| import { describe, expect, test } from "bun:test"; | |||
| import { bunEnv, bunExe, isWindows, tmpdirSync } from "harness"; | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use tempDir instead of tmpdirSync.
The coding guidelines specify to use tempDir from harness rather than tmpdirSync. The tempDir helper returns a disposable that handles cleanup automatically.
♻️ Proposed refactor to use tempDir
-import { bunEnv, bunExe, isWindows, tmpdirSync } from "harness";
+import { bunEnv, bunExe, isWindows, tempDir } from "harness";Then in the tests:
test("deep directory tree does not overflow path buffer", async () => {
- const root = tmpdirSync("bun-glob-overflow-deep-");
+ using root = tempDir("bun-glob-overflow-deep-"); test("self-referential symlink does not overflow path buffer", async () => {
- const root = tmpdirSync("bun-glob-overflow-symlink-");
+ using root = tempDir("bun-glob-overflow-symlink-");As per coding guidelines: "Use tempDir from harness to create temporary directories - do not use tmpdirSync or fs.mkdtempSync".
Also applies to: 24-24, 61-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/glob/path-length.test.ts` at line 2, Replace usage of tmpdirSync
with the tempDir helper from the harness: update the import to pull tempDir
instead of tmpdirSync (the import line that currently references tmpdirSync) and
change each test that calls tmpdirSync to call tempDir() to obtain a disposable
directory object, then use its path and rely on its automatic cleanup; update
references at the locations that use tmpdirSync (around the tests identified at
lines ~24 and ~61) to use the tempDir instance's directory path API.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the four bound-check guards are correct and the clamp in closes the last unsafe copy path.
Extended reasoning...
Overview
This PR adds four length guards in src/glob/GlobWalker.zig to prevent @memcpy / bun.copy writes past the end of fixed-size bun.PathBuffer scratch buffers when a work-item path exceeds MAX_PATH_BYTES. It also adds a new test file exercising both the deep-directory-tree and self-referential-symlink crash paths.
Security Risks
The underlying issue was a stack/heap buffer overflow reachable via adversarially crafted directory trees or symlink loops. The fix is the correct minimal intervention: guard each copy site with if (len >= buf.len) and propagate ENAMETOOLONG. No new attack surface is introduced.
Level of Scrutiny
The Zig changes are small and mechanical — four near-identical one-liners plus a @min clamp. The logic at each site is easy to verify: the null terminator is written one byte past the copied data, so the >= (rather than >) bound is correct. The error code and propagation path match the existing pattern in the file. Low risk of introducing a regression.
Other Factors
One nit was flagged as an inline comment: the test file contains expect(scanStderr).not.toContain("panic") assertions that are explicitly forbidden by CLAUDE.md (they can never fail in CI release builds). These are harmless redundancies — expect(scanCode).toBe(0) already catches crashes — and don't affect correctness. The inline comment covers this.
| expect(scanStdout.trim()).toBe("ERR:ENAMETOOLONG"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🟡 The two new test cases both call expect(scanStderr).not.toContain("panic") and expect(scanStderr).not.toContain("Segmentation fault"), which is explicitly forbidden by CLAUDE.md line 116: 'NEVER write tests that check for no panic or uncaught exception or similar in the test output. These tests will never fail in CI.' These assertions should be removed; expect(scanCode).toBe(0) already catches any crash.
Extended reasoning...
What the bug is and how it manifests
The root CLAUDE.md at line 116 explicitly states: 'NEVER write tests that check for no "panic" or "uncaught exception" or similar in the test output. These tests will never fail in CI.' The new test file test/js/bun/glob/path-length.test.ts adds exactly these forbidden assertions in both test cases: expect(scanStderr).not.toContain("panic") and expect(scanStderr).not.toContain("Segmentation fault") (lines 57–58 and 80–81).
The specific code path that triggers it
Both test cases spawn a subprocess running Bun with an eval script, capture its stderr, and then assert that stderr does not contain the strings "panic" or "Segmentation fault". These are .not.toContain() assertions — they pass when the string is absent and fail when present.
Why existing code doesn't prevent it
In a release build, if the subprocess crashes due to a buffer overrun or other fatal error, the OS delivers a signal (e.g. SIGSEGV). A release binary does not print the string "panic" or "Segmentation fault" to stderr before dying; it simply exits with a non-zero status code. Therefore the .not.toContain() assertions always pass, even in the presence of a crash. They are structurally incapable of detecting the failure they appear to guard against.
What the impact would be
These assertions give a false sense of safety. A developer reading the test might believe they provide an additional safety net against crashes, but they do not. The assertions are dead code: they can never transition from passing to failing regardless of what the subprocess does.
How to fix it
Simply remove the four forbidden assertions (two per test case). The meaningful regression protection is already provided by:
expect(scanCode).toBe(0)— catches any non-zero exit code, including crashes from signalsexpect(scanStdout.trim()).toBe("ERR:ENAMETOOLONG")— verifies the correct output
Step-by-step proof
- The subprocess crashes due to a buffer overrun (SIGSEGV).
- The OS delivers SIGSEGV; the process exits with a non-zero code (e.g. 139 on Linux, or the signal is recorded in the exit code).
- No "panic" or "Segmentation fault" string is written to stderr by the release binary.
expect(scanStderr).not.toContain("panic")evaluates:scanStderris empty or contains only other output → assertion passes even though a crash occurred.expect(scanCode).toBe(0)evaluates: exit code is 139 → assertion fails, correctly catching the crash.
This demonstrates the panic/segfault string checks are redundant and explicitly forbidden by the project's own guidelines.
- Filter musl getcwd warning from the deep-tree fixture builder's stderr (bash's cd warns once the cumulative path exceeds PATH_MAX on musl, but mkdir/cd still succeed). - Update scan.test.ts ./* snapshot to include the new test file.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/js/bun/glob/path-length.test.ts (1)
2-2: 🛠️ Refactor suggestion | 🟠 MajorReplace
tmpdirSyncwith disposabletempDir.This is still using the non-disposable temp dir helper and should be migrated to
tempDir+using.♻️ Suggested patch
-import { bunEnv, bunExe, isMusl, isWindows, tmpdirSync } from "harness"; +import { bunEnv, bunExe, isMusl, isWindows, tempDir } from "harness"; ... - const root = tmpdirSync("bun-glob-overflow-deep-"); + using root = tempDir("bun-glob-overflow-deep-"); ... - const root = tmpdirSync("bun-glob-overflow-symlink-"); + using root = tempDir("bun-glob-overflow-symlink-");As per coding guidelines: "Use
tempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync".Also applies to: 24-24, 68-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/bun/glob/path-length.test.ts` at line 2, Replace uses of tmpdirSync with the disposable tempDir helper and ensure resources are disposed via using: locate where tmpdirSync is imported/used (symbol tmpdirSync) and change to import tempDir from "harness", create the temp directory with tempDir() and wrap its result with using(...) so the directory is automatically cleaned up; update all occurrences (including lines referencing tmpdirSync at start and the other occurrences noted) to follow this pattern and remove any direct fs.mkdtempSync usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/glob/path-length.test.ts`:
- Line 30: Replace uses of String.prototype.repeat for generating long segment
names with Buffer.alloc(...).toString(); specifically change the declaration of
segName in the test (currently const segName = "D".repeat(255);) to use
Buffer.alloc(255, "D").toString(), and make the same replacement for the other
similar occurrence around line 69 (any other test variables creating long
repeated strings). This ensures test-generated long strings follow the guideline
without changing semantics.
- Around line 59-64: Remove the brittle stderr panic checks and reorder
assertions: delete the expect(scanStderr).not.toContain("panic") and
expect(scanStderr).not.toContain("Segmentation fault") lines, assert the
expected stdout via expect(scanStdout.trim()).toBe("ERR:ENAMETOOLONG") before
asserting the subprocess exit code with expect(scanCode).toBe(0), and apply the
same changes to the second similar block around the other test (the block
referenced by the comment "Also applies to: 89-96"), using the existing
variables scanStdout, scanStderr, and scanCode to locate the assertions.
---
Duplicate comments:
In `@test/js/bun/glob/path-length.test.ts`:
- Line 2: Replace uses of tmpdirSync with the disposable tempDir helper and
ensure resources are disposed via using: locate where tmpdirSync is
imported/used (symbol tmpdirSync) and change to import tempDir from "harness",
create the temp directory with tempDir() and wrap its result with using(...) so
the directory is automatically cleaned up; update all occurrences (including
lines referencing tmpdirSync at start and the other occurrences noted) to follow
this pattern and remove any direct fs.mkdtempSync usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e398331a-14ae-480b-9c3a-ee7ebdf2d7f2
⛔ Files ignored due to path filters (1)
test/js/bun/glob/__snapshots__/scan.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
test/js/bun/glob/path-length.test.ts
| // path length grows past MAX_PATH_BYTES (1024 on macOS, 4096 on | ||
| // Linux) even though the tree is legal on the filesystem. | ||
| const depth = 18; | ||
| const segName = "D".repeat(255); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use Buffer.alloc(...).toString() for repeated segment names.
Please avoid .repeat() for generated long strings in tests.
♻️ Suggested patch
- const segName = "D".repeat(255);
+ const segName = Buffer.alloc(255, "D").toString();
...
- const segName = "S".repeat(255);
+ const segName = Buffer.alloc(255, "S").toString();As per coding guidelines: "Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests."
Also applies to: 69-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/glob/path-length.test.ts` at line 30, Replace uses of
String.prototype.repeat for generating long segment names with
Buffer.alloc(...).toString(); specifically change the declaration of segName in
the test (currently const segName = "D".repeat(255);) to use Buffer.alloc(255,
"D").toString(), and make the same replacement for the other similar occurrence
around line 69 (any other test variables creating long repeated strings). This
ensures test-generated long strings follow the guideline without changing
semantics.
| expect(scanStderr).not.toContain("panic"); | ||
| expect(scanStderr).not.toContain("Segmentation fault"); | ||
| expect(scanCode).toBe(0); | ||
| // Walker must surface ENAMETOOLONG rather than keep walking past the | ||
| // fixed-size PathBuffer it copies each work item into. | ||
| expect(scanStdout.trim()).toBe("ERR:ENAMETOOLONG"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Drop panic-string stderr checks and assert stdout before exit code.
The panic/Segmentation fault string checks should be removed, and expected stdout should be asserted before exitCode for subprocess diagnostics.
♻️ Suggested patch
- expect(scanStderr).not.toContain("panic");
- expect(scanStderr).not.toContain("Segmentation fault");
- expect(scanCode).toBe(0);
// Walker must surface ENAMETOOLONG rather than keep walking past the
// fixed-size PathBuffer it copies each work item into.
expect(scanStdout.trim()).toBe("ERR:ENAMETOOLONG");
+ expect(scanCode).toBe(0);
...
- expect(scanStderr).not.toContain("panic");
- expect(scanStderr).not.toContain("Segmentation fault");
- expect(scanCode).toBe(0);
// Each hop through the self-loop appends a 256-byte segment, so after a
// few iterations work_item.path exceeds MAX_PATH_BYTES. The walker must
// terminate the loop with ENAMETOOLONG instead of copying the oversized
// path into its fixed-size PathBuffer.
expect(scanStdout.trim()).toBe("ERR:ENAMETOOLONG");
+ expect(scanCode).toBe(0);As per coding guidelines: "Never write tests that check for 'panic', 'uncaught exception', or similar in test output - these tests will never fail in CI" and "When spawning processes in tests, expect stdout before expecting exit code to get more useful error messages on test failure".
Also applies to: 89-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/glob/path-length.test.ts` around lines 59 - 64, Remove the
brittle stderr panic checks and reorder assertions: delete the
expect(scanStderr).not.toContain("panic") and
expect(scanStderr).not.toContain("Segmentation fault") lines, assert the
expected stdout via expect(scanStdout.trim()).toBe("ERR:ENAMETOOLONG") before
asserting the subprocess exit code with expect(scanCode).toBe(0), and apply the
same changes to the second similar block around the other test (the block
referenced by the comment "Also applies to: 89-96"), using the existing
variables scanStdout, scanStderr, and scanCode to locate the assertions.
| import { bunEnv, bunExe, isMusl, isWindows, tmpdirSync } from "harness"; | ||
| import * as fs from "node:fs"; | ||
| import * as path from "node:path"; | ||
|
|
||
| const runScanFixture = (pattern: string, opts: Record<string, unknown>) => ` | ||
| const { Glob } = require("bun"); | ||
| const g = new Glob(${JSON.stringify(pattern)}); | ||
| const opts = ${JSON.stringify(opts)}; | ||
| let count = 0; | ||
| try { | ||
| for (const p of g.scanSync(opts)) { | ||
| count++; | ||
| if (count > 100000) break; | ||
| } | ||
| console.log("OK:" + count); | ||
| } catch (err) { | ||
| console.log("ERR:" + (err && err.code ? err.code : String(err))); | ||
| } | ||
| `; | ||
|
|
||
| describe.skipIf(isWindows)("Glob path length", () => { | ||
| test("deep directory tree does not overflow path buffer", async () => { | ||
| const root = tmpdirSync("bun-glob-overflow-deep-"); | ||
| // Build a deep directory tree using bash cd+mkdir loops so each | ||
| // individual syscall uses a short relative path. The cumulative | ||
| // path length grows past MAX_PATH_BYTES (1024 on macOS, 4096 on | ||
| // Linux) even though the tree is legal on the filesystem. | ||
| const depth = 18; | ||
| const segName = "D".repeat(255); |
There was a problem hiding this comment.
🟡 Three CLAUDE.md guideline violations in test/js/bun/glob/path-length.test.ts: (1) tmpdirSync is imported and used (lines 2, 24, 61) instead of tempDir from harness, which provides automatic cleanup via the using keyword; (2) "D".repeat(255) (line 30) and "S".repeat(255) (line 69) should use Buffer.alloc(255, "D").toString() / Buffer.alloc(255, "S").toString() — String.prototype.repeat is very slow in debug JavaScriptCore builds (test/CLAUDE.md line 147); (3) in both test cases expect(scanCode).toBe(0) is checked before expect(scanStdout.trim()).toBe("ERR:ENAMETOOLONG") — stdout should be asserted first to give a more useful failure message (CLAUDE.md line 118).
Extended reasoning...
The new test file test/js/bun/glob/path-length.test.ts contains three distinct violations of project coding guidelines documented in CLAUDE.md and test/CLAUDE.md.
Bug 1 — tmpdirSync instead of tempDir (CLAUDE.md line 117)
Line 2 imports tmpdirSync from harness, and it is called at lines 24 and 61. CLAUDE.md line 117 explicitly states: "Use tempDir from harness to create a temporary directory. Do not use tmpdirSync or fs.mkdtempSync to create temporary directories." The tempDir helper returns a DisposableString that handles cleanup automatically via the using keyword (Symbol.dispose). The current code uses tmpdirSync and never explicitly cleans up the temporary directories, leaving them behind after the test run. The fix is to change the import to tempDir and use using root = tempDir("bun-glob-overflow-deep-") / using root = tempDir("bun-glob-overflow-symlink-").
Bug 2 — String.repeat() instead of Buffer.alloc() (test/CLAUDE.md line 147)
Line 30 uses "D".repeat(255) and line 69 uses "S".repeat(255) to build 255-character repetitive strings. test/CLAUDE.md line 147 explicitly states: "To create a repetitive string, use Buffer.alloc(count, fill).toString() instead of "A".repeat(count). "".repeat is very slow in debug JavaScriptCore builds." These strings are evaluated in the test runner JavaScript context, so in debug builds this incurs unnecessary overhead. The fix is Buffer.alloc(255, "D").toString() and Buffer.alloc(255, "S").toString().
Bug 3 — Assertion order: exit code before stdout (CLAUDE.md line 118)
Both test cases place expect(scanCode).toBe(0) before expect(scanStdout.trim()).toBe("ERR:ENAMETOOLONG"). CLAUDE.md line 118 states: "When spawning processes, tests should expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0). This gives you a more useful error message on test failure."
Why existing code does not prevent it
These are new tests introduced in this PR. There are no lint rules or type checks that catch CLAUDE.md guideline violations automatically; they rely on code review.
Step-by-step proof for Bug 3
Consider a regression where the walker crashes (SIGSEGV) instead of returning ENAMETOOLONG:
- The subprocess exits with code 139 (SIGSEGV on Linux) and produces empty stdout.
- With the current order:
expect(scanCode).toBe(0)fails first, error message is "expected 0, got 139" — gives no insight into what the program actually printed. - With the correct order:
expect(scanStdout.trim()).toBe("ERR:ENAMETOOLONG")fails first, error message is "expected ERR:ENAMETOOLONG, got (empty)" — immediately tells the developer the walker produced no output, pointing to a crash rather than a wrong error code.
How to fix all three
- Change
import { ..., tmpdirSync }toimport { ..., tempDir }and replaceconst root = tmpdirSync(...)withusing root = tempDir(...)in both tests. - Replace
"D".repeat(255)withBuffer.alloc(255, "D").toString()and"S".repeat(255)withBuffer.alloc(255, "S").toString(). - Move
expect(scanStdout.trim()).toBe("ERR:ENAMETOOLONG")beforeexpect(scanCode).toBe(0)in both test cases.
collapseDots() appends "/." or "/.." per leading Dot/DotBack pattern component into a fixed-size PathBuffer. The .DotBack bounds check compared the stale dir_path.len instead of the running len, so a run of multiple ".." components could grow len past MAX_PATH_BYTES and write out of the buffer. The .Dot check used the correct variable but still @Panic()ed on overflow. Change collapseDots()/skipSpecialComponents() to return Maybe(u32) and surface ENAMETOOLONG (routed through handleSysErrWithPath so the error path slice lives in walker.pathBuf, not the stack-resident iterator), fix the .DotBack check to use len, and propagate the error at both call sites. Adds two regression cases to path-length.test.ts that feed 2200 leading "./" and "../" components through scanSync().
GlobWalker builds up work-item paths via arena-allocated `join()` calls
and later copies them back into fixed-size `bun.PathBuffer` scratch
buffers (`[MAX_PATH_BYTES]u8` — 1024 on macOS, 4096 on Linux). For
deeply nested directory trees or self-referential symlinks (under
`followSymlinks: true`) the accumulated path can outgrow
`MAX_PATH_BYTES`, and the unchecked `@memcpy` in each copy site writes
past the buffer.
This adds length guards at the four affected sites in
`src/glob/GlobWalker.zig`:
- `Iterator.init` (root path copy into `walker.pathBuf`)
- `transitionToDirIterState` (work-item path into
`iter_state.directory.path`)
- the symlink branch in `next()` (work-item path into `walker.pathBuf`)
- `handleSysErrWithPath` (clamp to destination length)
When the incoming path would exceed the destination buffer, the walker
now returns a `bun.sys.Error` with `ENAMETOOLONG` (via `Maybe.err`)
instead of performing the copy. This also gives symlink loops a clean
termination: once the joined path exceeds `MAX_PATH_BYTES`, the walker
reports `ENAMETOOLONG` rather than looping unboundedly.
### Tests
`test/js/bun/glob/path-length.test.ts` (new) adds two regression tests:
- **deep directory tree**: creates 18 nested directories with 255-byte
names using bash `cd`+`mkdir` (so each syscall uses a short relative
path), then runs `new Glob('**/*').scanSync(...)` on the tree. The
accumulated relative path exceeds `MAX_PATH_BYTES`, and the scanner must
return `ENAMETOOLONG` instead of crashing.
- **self-referential symlink**: creates a 255-byte `S...S -> .` symlink
and scans `**/*` with `followSymlinks: true`. Each hop adds 256 bytes to
the work-item path; once it crosses `MAX_PATH_BYTES` the walker must
report `ENAMETOOLONG` rather than continuing past the PathBuffer.
Both tests fail on current main (SIGSEGV on the deep tree, silent
truncation for the symlink loop) and pass with the fix.
GlobWalker builds up work-item paths via arena-allocated `join()` calls
and later copies them back into fixed-size `bun.PathBuffer` scratch
buffers (`[MAX_PATH_BYTES]u8` — 1024 on macOS, 4096 on Linux). For
deeply nested directory trees or self-referential symlinks (under
`followSymlinks: true`) the accumulated path can outgrow
`MAX_PATH_BYTES`, and the unchecked `@memcpy` in each copy site writes
past the buffer.
This adds length guards at the four affected sites in
`src/glob/GlobWalker.zig`:
- `Iterator.init` (root path copy into `walker.pathBuf`)
- `transitionToDirIterState` (work-item path into
`iter_state.directory.path`)
- the symlink branch in `next()` (work-item path into `walker.pathBuf`)
- `handleSysErrWithPath` (clamp to destination length)
When the incoming path would exceed the destination buffer, the walker
now returns a `bun.sys.Error` with `ENAMETOOLONG` (via `Maybe.err`)
instead of performing the copy. This also gives symlink loops a clean
termination: once the joined path exceeds `MAX_PATH_BYTES`, the walker
reports `ENAMETOOLONG` rather than looping unboundedly.
### Tests
`test/js/bun/glob/path-length.test.ts` (new) adds two regression tests:
- **deep directory tree**: creates 18 nested directories with 255-byte
names using bash `cd`+`mkdir` (so each syscall uses a short relative
path), then runs `new Glob('**/*').scanSync(...)` on the tree. The
accumulated relative path exceeds `MAX_PATH_BYTES`, and the scanner must
return `ENAMETOOLONG` instead of crashing.
- **self-referential symlink**: creates a 255-byte `S...S -> .` symlink
and scans `**/*` with `followSymlinks: true`. Each hop adds 256 bytes to
the work-item path; once it crosses `MAX_PATH_BYTES` the walker must
report `ENAMETOOLONG` rather than continuing past the PathBuffer.
Both tests fail on current main (SIGSEGV on the deep tree, silent
truncation for the symlink loop) and pass with the fix.
…3145) ### Problem Scanning a directory tree whose joined absolute paths exceed 4096 bytes with `Bun.Glob` and `absolute: true` aborts the process: ``` panic: range end index 4209 out of range for slice of length 4095 ``` ```sh d=$(mktemp -d); cd "$d"; p="." for i in $(seq 1 30); do n=$(printf 'd%0148d' $i); p="$p/$n"; mkdir -p "$p"; done bun -e 'const {Glob} = require("bun"); console.log([...new Glob("**/*.ts").scanSync({ cwd: ".", absolute: true, onlyFiles: false })].length)' # bun: panic: range end index 4209 out of range for slice of length 4095 (SIGABRT, exit 134) ``` Such trees are legal on Linux (PATH_MAX limits a single syscall argument, not the depth of a tree), and they show up in practice (nested `node_modules`, generated content stores). `bun run --filter` uses the same walker with `absolute: true`, so a deep workspace tree can abort it too. ### Cause When `absolute` is set, `GlobWalker::join` goes through `bun_join`, which calls `resolve_path::join` / `join_z`. Those normalize into a fixed 4096-byte thread-local (`JOIN_BUF`) with no bounds check, so `normalize_string_generic_t`'s `buf[buf_i..buf_i + count]` indexes past the end as soon as dir + entry no longer fits (the `4095` in the message is the buffer minus the POSIX leading-separator slot). The relative branch of the same function uses a growable `Vec` join, and oversized work items are already converted to `ENAMETOOLONG` by the guards added in #28836. The absolute branch panics before those guards can run. ### Fix - `src/paths/resolve_path.rs`: add `join_z_spill`, the missing sibling of the existing `join_spill` (uses the thread-local buffer when the result fits, otherwise a caller-provided `Vec`). - `src/glob/GlobWalker.rs`: `bun_join` uses the spill variants, so joined paths of any length are produced without touching memory past the buffer. With that, the existing work-item guards take over: a directory whose joined path exceeds `MAX_PATH_BYTES` surfaces `ENAMETOOLONG` exactly like the relative walk does today, and a matched entry whose joined path merely exceeds the old buffer is returned instead of crashing. No behavior change for paths that fit. ### Tests Two new cases in `test/js/bun/glob/path-length.test.ts`, next to the existing deep-tree coverage: - deep tree scanned with `absolute: true` reports `ENAMETOOLONG` (was: SIGABRT) - a matched file whose absolute path exceeds the join buffer, inside a directory that is still walkable, is returned (was: SIGABRT) Both fail on the unfixed build and pass with the fix; the four pre-existing tests in the file are unchanged and still pass.
2 participants
GlobWalker builds up work-item paths via arena-allocated
join()calls and later copies them back into fixed-sizebun.PathBufferscratch buffers ([MAX_PATH_BYTES]u8— 1024 on macOS, 4096 on Linux). For deeply nested directory trees or self-referential symlinks (underfollowSymlinks: true) the accumulated path can outgrowMAX_PATH_BYTES, and the unchecked@memcpyin each copy site writes past the buffer.This adds length guards at the four affected sites in
src/glob/GlobWalker.zig:Iterator.init(root path copy intowalker.pathBuf)transitionToDirIterState(work-item path intoiter_state.directory.path)next()(work-item path intowalker.pathBuf)handleSysErrWithPath(clamp to destination length)When the incoming path would exceed the destination buffer, the walker now returns a
bun.sys.ErrorwithENAMETOOLONG(viaMaybe.err) instead of performing the copy. This also gives symlink loops a clean termination: once the joined path exceedsMAX_PATH_BYTES, the walker reportsENAMETOOLONGrather than looping unboundedly.Tests
test/js/bun/glob/path-length.test.ts(new) adds two regression tests:cd+mkdir(so each syscall uses a short relative path), then runsnew Glob('**/*').scanSync(...)on the tree. The accumulated relative path exceedsMAX_PATH_BYTES, and the scanner must returnENAMETOOLONGinstead of crashing.S...S -> .symlink and scans**/*withfollowSymlinks: true. Each hop adds 256 bytes to the work-item path; once it crossesMAX_PATH_BYTESthe walker must reportENAMETOOLONGrather than continuing past the PathBuffer.Both tests fail on current main (SIGSEGV on the deep tree, silent truncation for the symlink loop) and pass with the fix.