Fix crash in Bun.build() with many conditions - #30557
Conversation
… non-empty The expression `defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len` parses as `defaults.len + 2 + (if (allow_addons) 1 else (0 + conditions.len))`, so when allow_addons is true (the default), conditions.len was dropped from the capacity calculation, causing putAssumeCapacity to trip an assertion when enough user-specified conditions were passed to Bun.build.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR refactors capacity calculations in the ESM conditions bundler logic to avoid repeating conditional expressions, then adds a test case that validates builds succeed when many conditions are supplied to ChangesBuild Options Capacity Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #30466, which already fixes this plus a second instance of the same operator-precedence bug in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean fix for the Zig if-else precedence trap, with a regression test. The two inline notes are pre-existing/adjacent and non-blocking.
Extended reasoning...
Overview
This PR fixes a crash in ESMConditions.init (src/bundler/options.zig) caused by a Zig operator-precedence trap: a + if (cond) 1 else 0 + b parses as a + (if (cond) 1 else (0 + b)), so when allow_addons was true the conditions.len term was dropped from the ensureTotalCapacity calculation, and the subsequent putAssumeCapacity calls overflowed the map and tripped an assertion. The fix hoists if (allow_addons) 1 else 0 into a const addon_count: usize and uses it in the three capacity expressions — a minimal, obviously-correct transformation. A 12-line regression test in bun-build-api.test.ts passes 20 conditions to Bun.build() and asserts success.
Security risks
None. This is purely a capacity-arithmetic fix in the bundler's ESM conditions map initialization. No auth, crypto, permissions, or untrusted-input parsing is touched; the only behavioral change is that the map is now sized correctly instead of under-sized.
Level of scrutiny
Low. The diff is 4 lines of Zig (3 modified + 1 added const) plus a small test. The bug, its root cause (Zig IfExpr else-branch consuming the trailing expression), and the fix are all clearly explained in the PR description and trivially verifiable by reading the diff. The style_condition_map line immediately below already had the correct form (no if), so the fix brings the other three lines in line with it. No CODEOWNER covers these files.
Other factors
The two inline comments from the bug-hunting system are both explicitly flagged as pre-existing and not introduced by this PR: (1) style_condition_map reserves capacity for user conditions but the init loop never inserts into it — a real but orthogonal inconsistency worth a follow-up; (2) the same precedence pattern exists in loadersFromTransformOptions, but it's harmless there because subsequent inserts use getOrPutValue which grows. Neither blocks this crash fix. The full test file passes per the PR description, and the change was found via Fuzzilli with a concrete fingerprint.
| try default_condition_amp.ensureTotalCapacity(defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len); | ||
| try import_condition_map.ensureTotalCapacity(defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len); | ||
| try require_condition_map.ensureTotalCapacity(defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len); | ||
| const addon_count: usize = if (allow_addons) 1 else 0; |
There was a problem hiding this comment.
🟡 nit (pre-existing): the same if (cond) a else 0 + ... precedence pattern this PR fixes also exists in loadersFromTransformOptions (~line 1575): input_loaders.extensions.len + if (target.isBun()) default_loader_ext_bun.len else 0 + if (target == .browser) ... else 0 + default_loader_ext.len. It doesn't crash there because the subsequent inserts use getOrPutValue (which grows) rather than putAssumeCapacity, so the only effect is an unnecessary reallocation — but since this PR is specifically about this precedence trap, it might be worth giving that callsite the same const x: usize = if (...) n else 0; treatment for consistency.
Extended reasoning...
What
loadersFromTransformOptions in the same file computes its hashmap capacity as:
input_loaders.extensions.len +
if (target.isBun()) default_loader_ext_bun.len else 0 +
if (target == .browser) default_loader_ext_browser.len else 0 +
default_loader_ext.len,Per Zig grammar, an IfExpr's else branch consumes a full expression, so this parses as:
input_loaders.extensions.len + (if (target.isBun()) 2 else (0 + (if (target == .browser) 1 else (0 + 18))))This is the exact precedence pitfall this PR fixes in ESMConditions.init.
Step-by-step
With target = .bun and a user-supplied loaders map of 1 entry:
target.isBun()→ true, so the wholeifevaluates todefault_loader_ext_bun.len= 2.- The trailing
+ if (target == .browser) ... + default_loader_ext.lenis part of the else branch and is never evaluated. - Reserved capacity =
1 + 2= 3. - Intended capacity =
1 + 2 + 0 + 18= 21. stringHashMapFromArrayscallsensureTotalCapacity(3)thenputAssumeCapacityexactlyinput_loaders.extensions.len= 1 time → fits.- The 18
default_loader_extentries and 2default_loader_ext_bunentries are inserted viagetOrPutValue, which grows the map → no crash, just a redundant rehash/realloc.
(Note also that stringHashMapFromArrays only calls ensureTotalCapacity at all when input_loaders.extensions.len > 0, i.e. only when the user passes custom loaders.)
Why it doesn't crash here but did in ESMConditions
ESMConditions.init follows the under-reserved ensureTotalCapacity with putAssumeCapacity for all inserts, so under-allocation overflows the map and trips the assertion. loadersFromTransformOptions only putAssumeCapacitys the first input_loaders.extensions.len items — and that term is the unconditional first addend, always counted regardless of how the if parses — so the assumed-capacity inserts always fit. Everything after that uses getOrPutValue.
Addressing the objection
One verifier argued this shouldn't be flagged because there's no incorrect behavior — just a sub-optimal capacity hint. That's accurate: this is not a correctness bug and would not justify a standalone report. It's flagged here only because the PR's stated purpose is fixing this specific precedence trap in this file, and an identical instance sits ~500 lines away. Filing it as a nit / pre-existing so the author can decide whether to roll it into the same change; it should not block the PR.
Suggested fix
Same shape as the PR's fix:
const bun_count: usize = if (target.isBun()) default_loader_ext_bun.len else 0;
const browser_count: usize = if (target == .browser) default_loader_ext_browser.len else 0;
... input_loaders.extensions.len + bun_count + browser_count + default_loader_ext.len ...| try default_condition_amp.ensureTotalCapacity(defaults.len + 2 + addon_count + conditions.len); | ||
| try import_condition_map.ensureTotalCapacity(defaults.len + 2 + addon_count + conditions.len); | ||
| try require_condition_map.ensureTotalCapacity(defaults.len + 2 + addon_count + conditions.len); | ||
| try style_condition_map.ensureTotalCapacity(defaults.len + 2 + conditions.len); |
There was a problem hiding this comment.
🟣 Pre-existing, not introduced here, but since you're touching this exact block: style_condition_map reserves capacity for conditions.len on the line right below, yet the for (conditions) loop a few lines down only inserts into import/require/default — never style. Meanwhile ESMConditions.appendSlice does add to self.style, so whether a user condition applies to CSS @import package-exports resolution depends on which codepath populated the struct. Probably worth a follow-up (either add style_condition_map.putAssumeCapacity(condition, {}) to the loop, or drop + conditions.len from style's reservation if the omission is intentional).
Extended reasoning...
What the inconsistency is
ESMConditions.init reserves capacity in style_condition_map for the user-supplied conditions slice (src/bundler/options.zig:1094):
try style_condition_map.ensureTotalCapacity(defaults.len + 2 + conditions.len);but the loop that actually inserts those conditions skips style entirely (lines 1100-1104):
for (conditions) |condition| {
import_condition_map.putAssumeCapacity(condition, {});
require_condition_map.putAssumeCapacity(condition, {});
default_condition_amp.putAssumeCapacity(condition, {});
}So style ends up with only defaults.len + 2 entries ("style", the target defaults, and "default"). The extra conditions.len of reserved capacity is never filled — harmless on its own, but it's a strong hint the loop was meant to insert into style too.
Why it's a behavioral inconsistency, not just over-reservation
ESMConditions.appendSlice (lines 1152-1163) and append (1166-1171) do insert into self.style:
pub fn appendSlice(self: *ESMConditions, conditions: []const string) bun.OOM!void {
try self.default.ensureUnusedCapacity(conditions.len);
try self.import.ensureUnusedCapacity(conditions.len);
try self.require.ensureUnusedCapacity(conditions.len);
try self.style.ensureUnusedCapacity(conditions.len);
for (conditions) |condition| {
...
self.style.putAssumeCapacity(condition, {});
}
}The resolver consults r.opts.conditions.style for ImportKind.at / .at_conditional (resolver.zig:1871), i.e. CSS @import "pkg" going through package.json "exports". So whether a given condition is honored for CSS resolution depends on which API added it:
| Path | Reaches style? |
|---|---|
Bun.build({ conditions: ["custom"] }) → transform.conditions → ESMConditions.init |
❌ |
conditions.appendSlice(&.{"development"}) (JSBundleCompletionTask.zig:217, build_command.zig:240, bake.zig:764) |
✅ |
conditions.appendSlice(&.{"react-server"}) (bake.zig:760, build_command.zig:269) |
✅ |
conditions.appendSlice(&.{"node"}) (bake.zig:769) |
✅ |
Step-by-step example
- User calls
Bun.build({ entrypoints: ["./app.css"], conditions: ["custom"] }). BundleOptions.fromApicallsESMConditions.init(allocator, target.defaultConditions(), true, &.{"custom"}).style_condition_mapreserves space fordefaults.len + 2 + 1, but thefor (conditions)loop never inserts"custom"into it.styleends up as{"style", "browser", "module", "default"}.app.csscontains@import "some-pkg";wheresome-pkg/package.jsonhas"exports": { "custom": "./custom.css", "default": "./index.css" }.- Resolver hits
ImportKind.at→ usesconditions.style→"custom"is absent → resolves to./index.css. - Yet the same
"custom"condition is inimport/require/default, so JS imports from the same build would see"custom". And if the condition had been added later viaappendSlice(as the framework codepaths do for"development"), CSS would see it.
Why nothing prevents it
There's no fallback — style is the only map consulted for CSS @import. The over-reservation is silently tolerated because ensureTotalCapacity just allocates more than needed; nothing asserts that the reserved slots get used. The asymmetry with appendSlice/append has no guard either.
Impact
User-supplied conditions passed to Bun.build() (or bun build --conditions) are silently ignored for CSS @import package-exports resolution, while framework-injected conditions like development/react-server are honored. Niche but surprising; the capacity-reservation line and the symmetry with appendSlice both suggest the omission in the init loop was accidental.
Fix
Add the missing line to the loop:
for (conditions) |condition| {
import_condition_map.putAssumeCapacity(condition, {});
require_condition_map.putAssumeCapacity(condition, {});
default_condition_amp.putAssumeCapacity(condition, {});
style_condition_map.putAssumeCapacity(condition, {});
}(Capacity is already reserved on line 1094, so putAssumeCapacity is safe.) Alternatively, if excluding user conditions from CSS resolution is intentional, drop + conditions.len from line 1094 and add a comment — but appendSlice's behavior argues against that being the intent.
Relationship to this PR
Not introduced here — the PR only touches lines 1090-1093. But line 1094 is literally the next line, in the same function, operating on the same conditions parameter that this PR is fixing the capacity math for. Flagging as adjacent context; should be its own PR rather than folded into this crash fix.
1 participant
What does this PR do?
Fixes an assertion failure in
ESMConditions.initwhenBun.build()is called with a non-trivial number ofconditions.The capacity reservation used:
which Zig parses as:
When
allow_addonsistrue(the default),conditions.lenwas dropped entirely from the capacity calculation. The subsequentputAssumeCapacitycalls would then overflow the map and tripassert(self.len < self.capacity)inMultiArrayList.addOneAssumeCapacity.Repro:
How did you verify your code works?
test/bundler/bun-build-api.test.tsthat passes 20 conditions; it crashes the bundle thread before this change and passes after.bun bd test test/bundler/bun-build-api.test.ts— all 40 tests pass.Found by Fuzzilli (fingerprint
a16293634a0a5123).