Fix crash in Bun.build when passing many custom conditions - #30660
Fix crash in Bun.build when passing many custom conditions#30660robobun wants to merge 1 commit into
Conversation
The expression 'if (allow_addons) 1 else 0 + conditions.len' was parsed as 'if (allow_addons) 1 else (0 + conditions.len)', so when allow_addons was true (the default), conditions.len was never counted toward the map capacity. Passing more than a couple of custom conditions to Bun.build would trip the putAssumeCapacity assert in debug builds and write past the reserved capacity in release builds.
|
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 (2)
WalkthroughThis PR refactors capacity calculation in the bundler's ESM conditions initialization to use type-cast arithmetic instead of conditional expressions, then adds a regression test to verify the bundler handles many custom conditions without crashing. ChangesCustom ESM Conditions Capacity Fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #30466, which already has this fix plus an additional precedence fix in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — correct and minimal fix for the if/else precedence bug, with a regression test.
Extended reasoning...
Overview
This PR changes 3 lines in src/bundler/options.zig (ESMConditions.init), replacing if (allow_addons) 1 else 0 + conditions.len with @as(usize, @intFromBool(allow_addons)) + conditions.len to fix a Zig precedence bug where the else branch greedily consumed + conditions.len. When allow_addons was true (the default), the user-supplied conditions were not counted toward the reserved capacity, and the subsequent putAssumeCapacity loop over conditions overflowed the map. A regression test in test/bundler/bun-build-api.test.ts exercises 16 custom conditions.
Correctness
I verified the new capacity expression against the actual inserts: import/require maps insert exactly defaults.len + conditions.len + @intFromBool(allow_addons) + 2 entries (initial sentinel, conditions loop, defaults loop, optional node-addons, default), which matches the reservation. default_condition_amp inserts one fewer (no initial sentinel), so it's slightly over-reserved — harmless. The unchanged style line was already correct for its insert count.
Security risks
None. This is a capacity-hint arithmetic fix in bundler options initialization with no user-controlled data flowing into unsafe sinks.
Level of scrutiny
Low. The change is a 3-line arithmetic correction whose intent and effect are fully explained by the PR description and verifiable by inspection. No CODEOWNERS match for these paths. The added test follows the existing test.concurrent + tempDirWithFiles pattern in the same file.
Other factors
The one inline comment flags a pre-existing, harmless instance of the same precedence pattern in loadersFromTransformOptions (which uses getOrPutValue so cannot crash). It's explicitly non-blocking and doesn't affect approval.
| try default_condition_amp.ensureTotalCapacity(defaults.len + 2 + @as(usize, @intFromBool(allow_addons)) + conditions.len); | ||
| try import_condition_map.ensureTotalCapacity(defaults.len + 2 + @as(usize, @intFromBool(allow_addons)) + conditions.len); | ||
| try require_condition_map.ensureTotalCapacity(defaults.len + 2 + @as(usize, @intFromBool(allow_addons)) + conditions.len); |
There was a problem hiding this comment.
🟣 Pre-existing, non-blocking: the identical if (cond) X else 0 + Y precedence footgun also exists in loadersFromTransformOptions in this same file (lines 1574-1577). It's harmless there — the under-reserved capacity is followed by getOrPutValue calls that grow the map, so it only costs an extra rehash — but since this PR exists specifically to eliminate this pattern, you may want to apply the same @intFromBool fix there for consistency.
Extended reasoning...
What the issue is
This PR fixes a Zig precedence footgun in ESMConditions.init where if (allow_addons) 1 else 0 + conditions.len parses as if (allow_addons) 1 else (0 + conditions.len). The exact same construct appears ~480 lines later in the same file, in loadersFromTransformOptions (src/bundler/options.zig:1574-1577):
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,Zig's else branch is greedy, so this parses 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))))Step-by-step walkthrough
Take target = .bun (so target.isBun() == true):
- The outer
ifevaluates todefault_loader_ext_bun.len(= 2). - The entire
elsebranch — including the nested browser check anddefault_loader_ext.len(= 16) — is skipped. - Reserved capacity =
input_loaders.extensions.len + 2. - The intended capacity was
input_loaders.extensions.len + 2 + 0 + 16 = input_loaders.extensions.len + 18.
Similarly for target = .browser: the inner if returns default_loader_ext_browser.len (= 1) and again drops default_loader_ext.len, giving extensions.len + 0 + 1 instead of extensions.len + 0 + 1 + 16. Only target = .node accidentally gets the right answer (both conditions false → 0 + 0 + 16).
Why this is not a crash (unlike the ESMConditions case)
The fixed bug in ESMConditions.init was dangerous because the under-reserved map was followed by putAssumeCapacity calls for the user-supplied conditions. Here the safety story is different:
stringHashMapFromArraysonly callsputAssumeCapacityforinput_loaders.extensions, andinput_loaders.extensions.lenis always the first addend, so it is always covered regardless of how the rest mis-parses.- The 16 default extensions, the 2 bun extensions, and the 1 browser extension are all inserted afterward via
try loaders.getOrPutValue(...), which grows the map as needed. - Additionally, when no user loaders are passed (
keys.len == 0, the common case),stringHashMapFromArraysskipsensureTotalCapacityentirely, so the hint is unused.
So the only effect is a wrong capacity hint causing one or two extra rehashes on a ~20-entry map during one-time options initialization — no observable behavior change, no crash, not measurable.
Addressing the "out of scope" objection
It's fair to note that this PR doesn't touch loadersFromTransformOptions, doesn't call into it, and the under-reservation there has zero functional impact — on its own it would not warrant a comment. The reason it's worth a non-blocking mention here is narrower than "same file": it is literally the same syntactic trap (else 0 + ...) that this PR's title and description identify as the root cause of a crash. Leaving an identical instance of the pattern a few hundred lines down means the next person to change those getOrPutValue calls to putAssumeCapacity (a plausible micro-optimization) reintroduces the exact crash this PR fixes. Applying the same @intFromBool rewrite there is a one-line consistency change, and the author already has the pattern in their editor.
Suggested fix
input_loaders.extensions.len +
@as(usize, @intFromBool(target.isBun())) * default_loader_ext_bun.len +
@as(usize, @intFromBool(target == .browser)) * default_loader_ext_browser.len +
default_loader_ext.len,(or equivalently, parenthesize each ternary). This is pre-existing and should not block the PR — purely a "while you're here" suggestion.
1 participant
What does this PR do?
Fixes a debug assertion / capacity underflow in
ESMConditions.inittriggered by passing several customconditionstoBun.build().The capacity reservation used:
which Zig parses as:
When
allow_addonsistrue(the default),conditions.lenwas never counted toward the reserved capacity, so the subsequentputAssumeCapacitycalls overflowed the map once a handful of custom conditions were supplied.Replaced the ternary with
@intFromBoolto avoid the precedence footgun.How did you verify your code works?
Repro that panics on
mainand passes with this change:Added a regression test in
test/bundler/bun-build-api.test.ts.Found by Fuzzilli (fingerprint
4b17c3a7de281a5f).