Skip to content

Fix crash in Bun.build with many conditions - #30466

Closed
robobun wants to merge 3 commits into
mainfrom
farm/19973416/fix-esm-conditions-capacity
Closed

Fix crash in Bun.build with many conditions#30466
robobun wants to merge 3 commits into
mainfrom
farm/19973416/fix-esm-conditions-capacity

Conversation

@robobun

@robobun robobun commented May 10, 2026

Copy link
Copy Markdown
Collaborator

ESMConditions.init computed hashmap capacity with:

defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len

which Zig parses as if (allow_addons) 1 else (0 + conditions.len). When allow_addons is true (the default for Bun.build), conditions.len was dropped from the capacity request, so passing several custom conditions would overflow the preallocated map and hit the putAssumeCapacity assertion on the bundle thread.

Replaced the if expression with @intFromBool(allow_addons) so the sum is unambiguous.

Repro

await Bun.build({
  entrypoints: ["./entry.js"],
  conditions: ["aa", "bb", "cc", "dd", "ee", "ff", "gg", "hh"],
});

Found by Fuzzilli (fingerprint f7b40b0e431d91d6).

The expression 'if (allow_addons) 1 else 0 + conditions.len' parses as
'if (allow_addons) 1 else (0 + conditions.len)' due to Zig's if/else
precedence, so when allow_addons is true (the default), conditions.len
was dropped from the ensureTotalCapacity computation. Passing enough
conditions to Bun.build() would then overflow the preallocated capacity
and trip an assertion in putAssumeCapacity.
@robobun

robobun commented May 10, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 9:41 PM PT - May 10th, 2026

@robobun, your commit cc22fb4 has 1 failures in Build #53197 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 30466

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

bun-30466 --bun

@coderabbitai

coderabbitai Bot commented May 10, 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: e5a4e85e-cd3f-446c-be51-594bee480d14

📥 Commits

Reviewing files that changed from the base of the PR and between ba9a2d0 and be7e33a.

📒 Files selected for processing (1)
  • src/bundler/options.zig

Walkthrough

This PR refactors capacity-sizing arithmetic in the ESMConditions initializer to use the built-in @intFromBool function and adds a concurrent regression test that runs Bun.build with many conditions entries to ensure the build does not crash.

Changes

ESMConditions Capacity Optimization

Layer / File(s) Summary
Capacity-sizing refactor
src/bundler/options.zig
ensureTotalCapacity calculations updated to use @intFromBool(allow_addons) and conditional additions for loader-map capacity are explicitly parenthesized.
Regression test
test/bundler/bun-build-api.test.ts
New concurrent test "conditions array with many entries does not crash" invokes Bun.build with many condition strings and asserts build.success is true.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main fix: resolving a crash in Bun.build when many conditions are provided.
Description check ✅ Passed The PR description provides comprehensive technical details including root cause analysis, the symptom, the fix, a reproduction case, and discovery attribution, exceeding the template's basic sections.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

@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 — straightforward operator-precedence fix with a regression test.

Extended reasoning...

Overview

Three identical lines in ESMConditions.init (src/bundler/options.zig) replace if (allow_addons) 1 else 0 with @intFromBool(allow_addons) so that + conditions.len is no longer swallowed into the else branch. A 10-line regression test in bun-build-api.test.ts exercises Bun.build with 8 custom conditions to cover the previously-crashing path.

Security risks

None. This is an arithmetic fix to a hashmap capacity hint; no auth, parsing, or external-input handling is touched beyond what already existed.

Level of scrutiny

Low. The diff is mechanical and self-evidently correct: @intFromBool(allow_addons) yields the intended 0/1 and the surrounding addition now matches the actual number of putAssumeCapacity calls below (verified against the insert loops for default/import/require maps). The style map line is untouched, which is correct since it never inserts node-addons.

Other factors

The one inline comment is a non-blocking nit about a sibling occurrence of the same pattern in loadersFromTransformOptions; that one only mis-sizes a capacity hint (subsequent inserts use getOrPutValue, which grows) and is pre-existing, so it doesn't affect approval. No prior reviewer comments to address, and the fix was fuzzer-discovered with a clear repro.

Comment thread src/bundler/options.zig
Same pattern; this one only mis-sized a capacity hint since subsequent
inserts use getOrPutValue which grows, but fix it for consistency.

@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 precedence fix is correct and the sibling case in loadersFromTransformOptions is now addressed too.

Extended reasoning...

Overview

Two surgical arithmetic fixes in src/bundler/options.zig plus a regression test:

  • ESMConditions.init: replaces if (allow_addons) 1 else 0 + conditions.len with @intFromBool(allow_addons) + conditions.len in three ensureTotalCapacity calls. The old form parsed as if (allow_addons) 1 else (0 + conditions.len), dropping conditions.len from the capacity request when allow_addons is true (the default), which tripped the putAssumeCapacity assertion with enough custom conditions.
  • loadersFromTransformOptions: parenthesizes the two if (...) N else 0 terms so default_loader_ext.len is always included in the capacity hint (follow-up to my earlier comment, fixed in be7e33a).
  • New test/bundler/bun-build-api.test.ts case builds with 8 custom conditions to cover the original crash.

Security risks

None. This only adjusts hashmap capacity arithmetic for bundler option setup; no user-input parsing, auth, or boundary changes.

Level of scrutiny

Low. The diff is ~6 lines of integer-expression rewrites whose correctness is verifiable by inspection: @intFromBool(allow_addons) yields 0/1 with no precedence ambiguity, and the parenthesized if expressions now sum exactly the intended terms. The behavior change is strictly "reserve enough capacity" — semantics of the maps are unchanged.

Other factors

  • My previous review comment (the sibling precedence bug in loadersFromTransformOptions) was addressed.
  • A regression test was added that exercises the crashing path.
  • CI on the first commit (ba9a2d0) showed build-zig failures across all platforms; the robobun status hasn't yet refreshed for be7e33a. CI remains the merge gate, so approving here doesn't bypass that — if the failure is real it'll still block, but the code change itself reads as correct Zig.

@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 sibling loadersFromTransformOptions case is now fixed too.

Extended reasoning...

Overview

Two-line arithmetic fix in src/bundler/options.zig plus a regression test:

  • ESMConditions.init (options.zig:1090-1092): replaces if (allow_addons) 1 else 0 with @intFromBool(allow_addons) in three ensureTotalCapacity calls. The old form parsed as if (allow_addons) 1 else (0 + conditions.len), so when allow_addons was true the user-supplied conditions.len term was dropped from the capacity request and the subsequent putAssumeCapacity loop asserted.
  • loadersFromTransformOptions (options.zig:1575-1577): parenthesizes the same if ... else 0 + ... pattern. This one was only a capacity hint (followed by growing getOrPutValue inserts), but it's the exact footgun I flagged in my earlier review and the author addressed it in be7e33a.
  • New test.concurrent in bun-build-api.test.ts that calls Bun.build with 8 conditions and asserts build.success.

Security risks

None. Pure integer-arithmetic correction in a capacity calculation; no parsing, auth, or untrusted-input handling changes.

Level of scrutiny

Low. The diff is six lines of mechanical precedence disambiguation with no behavioral change beyond requesting the intended capacity. @intFromBool returns a u1 so the sum is now unambiguous, and the parenthesized form in loadersFromTransformOptions is trivially equivalent to intent. The regression test follows existing patterns in the file (tempDirWithFiles + Bun.build + expect(build.success)).

Other factors

My earlier inline comment on this PR pointed out the second occurrence in loadersFromTransformOptions; it was fixed in a follow-up commit and the thread is resolved. The bug-hunting pass found nothing on the latest revision. No CODEOWNERS apply to these paths.

@robobun

robobun commented May 11, 2026

Copy link
Copy Markdown
Collaborator Author

CI failure is test/js/bun/test/parallel/test-http-should-emit-close-when-connection-is-aborted.ts timing out on windows-2019-x64 — a pre-existing issue also hitting #30453 and #30457, unrelated to this bundler capacity fix. All other 64+ checks pass including the new regression test on every platform.

@robobun

robobun commented May 13, 2026

Copy link
Copy Markdown
Collaborator Author

Dupe-checked from #30619 — this fixes the same capacity-precedence bug the issue reporter hit on the CLI side (bun --conditions a --conditions b --conditions c --conditions d … falls back to default, or panics under ASAN). Linking so the issue closes when this lands.

@robobun

robobun commented May 14, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #30692 which includes this fix plus a runtime --conditions regression test and links #30619.

@robobun robobun closed this May 14, 2026
ericsssan added a commit to ericsssan/zbc that referenced this pull request May 27, 2026
Fires on `else 0 + <addend>` / `else 1 + <addend>` — Zig's `if`
expression has lower precedence than `+`, so the addend is absorbed into
the else-branch and silently dropped when the condition is true.

Backed by oven-sh/bun#30466 and 20+ duplicate PRs where
`if (allow_addons) 1 else 0 + conditions.len` dropped `conditions.len`
whenever `allow_addons` was true, crashing Bun.build with several
conditions via putAssumeCapacity past the reserved slots.
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.

1 participant