Skip to content

glob: fix abort when joined absolute paths exceed the join buffer - #33145

Merged
Jarred-Sumner merged 3 commits into
mainfrom
farm/04c78893/glob-absolute-join-overflow
Jun 30, 2026
Merged

Jarred-Sumner merged 3 commits into
mainfrom
farm/04c78893/glob-absolute-join-overflow

Conversation

@robobun

@robobun robobun commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

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
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.

The absolute-path branch of GlobWalker's join wrote the normalized result
into ResolvePath's fixed 4096-byte thread-local buffer with no bounds
check, so scanning a tree whose joined absolute paths exceed it aborted
the process with an index-out-of-range panic. The relative branch already
uses a growable join and reports ENAMETOOLONG via the work-item guards.

Add join_z_spill (the missing sibling of join_spill) and use the spill
variants in bun_join so the join can produce paths of any length. Over-long
directory work items then surface ENAMETOOLONG like the relative walk, and
matched entries whose joined path merely exceeds the buffer are returned.
@robobun

robobun commented Jun 30, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:34 PM PT - Jun 30th, 2026

❌ @robobun, your commit 3f8d925 has 1 failures in Build #67288 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33145

That installs a local version of the PR into your bun-33145 executable, so you can run:

bun-33145 --bun

@coderabbitai

coderabbitai Bot commented Jun 30, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d418ad6d-52e2-4f2f-bc58-be5164b798c7

📥 Commits

Reviewing files that changed from the base of the PR and between 9d58e73 and 3f8d925.

📒 Files selected for processing (1)
  • test/js/bun/glob/path-length.test.ts

Walkthrough

resolve_path.rs adds join_z_spill for long ZStr joins. GlobWalker switches bun_join to spill-backed joins. The glob path-length tests are refactored and extended with absolute-path overflow cases.

Changes

GlobWalker long-path spill fix

Layer / File(s) Summary
join_z_spill helper in resolve_path
src/paths/resolve_path.rs
Adds join_z_spill with a thread-local fast path and a spill-buffer fallback for joined ZStr output.
bun_join switched to spill variants
src/glob/GlobWalker.rs
bun_join now uses join_z_spill/join_spill with a heap-backed spill buffer and preserves the sentinel NUL behavior.
Absolute-path overflow tests
test/js/bun/glob/path-length.test.ts
Adds helpers for deep-tree setup and sorted path output, refactors the existing overflow test, and adds absolute-path overflow assertions.

Possibly related PRs

  • oven-sh/bun#32368: Introduces and uses spill-based path join helpers in resolve_path.rs, closely matching the join-path changes here.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main fix: preventing aborts when absolute joined paths exceed the join buffer.
Description check ✅ Passed The description explains the problem, cause, fix, and verification, which covers the template’s intent despite using different section headings.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/bun/glob/path-length.test.ts`:
- Around line 113-118: The matched-path overflow test in path-length.test.ts is
using the filesystem MAX_PATH_BYTES limit instead of the actual 4096-byte join
buffer target, so on macOS it never exercises the spill path. Update the test
setup in the buildDeepTree/depth calculation flow to either skip this case on
darwin or derive the depth from the real join-buffer constant used by the glob
walker, so the fixture actually crosses the intended overflow threshold.
🪄 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: 89f0881d-4a9a-477f-87b2-9c096663ef5b

📥 Commits

Reviewing files that changed from the base of the PR and between 3e08719 and 64e45e6.

📒 Files selected for processing (3)
  • src/glob/GlobWalker.rs
  • src/paths/resolve_path.rs
  • test/js/bun/glob/path-length.test.ts

Comment thread test/js/bun/glob/path-length.test.ts Outdated
On macOS MAX_PATH_BYTES is 1024, so a directory that is still walkable can
never produce a joined path that reaches the 4096-byte join buffer; the
case only exercises the spill path on Linux. Size it by the join buffer
constant and skip it on macOS.
Comment thread test/js/bun/glob/path-length.test.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — the spill fix mirrors the existing join_spill pattern and the macOS test concern from the last round is addressed; the FreeBSD gate nit below is non-blocking.

Extended reasoning...

Overview

This PR fixes a SIGABRT in Bun.Glob when scanning with absolute: true over a directory tree whose joined paths exceed 4096 bytes. Three files touched:

  • src/paths/resolve_path.rs: adds join_z_spill (13 lines), a direct sibling of the pre-existing join_spill immediately above it — same fast-path check against JOIN_BUF_LEN, same spill-resize, calls join_z_buf instead of join_string_buf.
  • src/glob/GlobWalker.rs: bun_join swaps join/join_z for join_spill/join_z_spill with a local Vec<u8> spill. The fast path (needed <= JOIN_BUF_LEN) returns exactly what the old code returned, so no behavior change for any path that fit before.
  • test/js/bun/glob/path-length.test.ts: refactors the existing deep-tree builder into a helper and adds two regression tests.

Security risks

None. The change replaces an unchecked write into a fixed thread-local buffer (which Rust's bounds check turned into a panic/abort) with a heap-backed spill. Strictly safer; no new untrusted-input surface.

Level of scrutiny

Moderate — resolve_path.rs is core infrastructure, but the addition is a near-verbatim copy of join_spill (in-tree since #32368) and the only caller is the glob walker. The fast path is byte-identical to the old behavior. The bun_join change preserves the SENTINEL NUL handling exactly as before. No CODEOWNERS cover these paths.

Other factors

  • The CodeRabbit / my own prior-round concern about the third test not exercising the spill on macOS was addressed in 9d58e73: the test now has a "Linux-only" comment, derives depth from the 4096-byte join-buffer constant, and is gated with skipIf(isMacOS).
  • The one remaining finding from this round (skipIf(isMacOS) → skipIf(!isLinux) for FreeBSD) is explicitly a nit with no current CI impact — FreeBSD only runs build-rust, not the test suite. Worth tightening but not a merge blocker.
  • The two CI failures reported on 64e45e6 were a Windows-aarch64 agent-creation infra failure and a FreeBSD build-rust job; neither is plausibly caused by this platform-agnostic 13-line addition that mirrors already-building code.

Comment thread test/js/bun/glob/path-length.test.ts Outdated
The comment says Linux-only, but skipIf(isMacOS) still ran it on the BSDs,
where MAX_PATH_BYTES is also 1024 and the case cannot be constructed.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — the spill-variant fix mirrors the existing join_spill pattern, and both earlier review nits (test gating) have been addressed.

Extended reasoning...

Overview

This PR fixes a process abort (panic: range end index ... out of range) when Bun.Glob with absolute: true walks a directory tree whose joined paths exceed the 4096-byte thread-local JOIN_BUF. Three files touched:

  • src/paths/resolve_path.rs (+13): adds join_z_spill, a line-for-line sibling of the pre-existing join_spill — checks join_needed(parts) <= JOIN_BUF_LEN, falls through to join_z when it fits (zero behavior change for the common case), otherwise resizes the caller's Vec and calls join_z_buf.
  • src/glob/GlobWalker.rs (+8/-2): bun_join allocates an empty Vec<u8> spill and calls join_z_spill / join_spill instead of join_z / join. The result is immediately copied into an owned Box<[u8]>, so the borrow into spill does not escape and Vec::new() allocates nothing on the fast path.
  • test/js/bun/glob/path-length.test.ts: extracts a buildDeepTree helper from the existing test (no behavior change to that test) and adds two regression cases — one for the ENAMETOOLONG path and one Linux-only case that exercises the heap spill for a returned match.

Security risks

None. This is defensive path-buffer handling that converts an abort into either a heap spill (returned match) or the existing ENAMETOOLONG error (oversized work item). No auth, permissions, parsing of untrusted input formats, or external I/O is involved; the only new allocation is a bounded Vec sized by join_needed.

Level of scrutiny

Low-to-medium. The Rust change is ~20 lines that exactly follow the established join_spill / join_z_buf_spill pattern already in the same file, and the fast path is byte-identical to the pre-fix code. The glob walker is production code, but the change is purely additive (heap fallback) with no control-flow or semantics changes when paths fit.

Other factors

  • I left two inline comments on earlier revisions (test sizing on macOS; skipIf(isMacOS) vs skipIf(!isLinux) for BSDs). Both were addressed in 9d58e73 and 3f8d925 respectively, and the current diff reflects those fixes (isLinux imported, test.skipIf(!isLinux) used, comment updated).
  • CodeRabbit raised the same macOS sizing point; resolved.
  • The bug-hunting system found nothing.
  • The single CI failure (test-net-connect-memleak.js) is an unrelated Node parallel test.
  • No CODEOWNERS entry covers these paths.

@robobun

robobun commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: the only failing test on this PR's last two builds (67278, 67288) is test/js/node/test/parallel/test-net-connect-memleak.js on the linux-x64-musl (alpine) lanes, which is the pre-existing flake tracked in #33044 and pending quarantine in #33045. It is a GC timing assertion in node:net and is not reachable from this change (glob walker path joins).

Everything else is green: the new test/js/bun/glob/path-length.test.ts cases pass on every lane that runs them, and both review passes have no open findings. No further pushes from me to avoid re-rolling a known-flaky lane; this is ready for a maintainer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants