Fix capacity under-allocation in ESMConditions.init with user conditions - #30479
Fix capacity under-allocation in ESMConditions.init with user conditions#30479robobun wants to merge 1 commit into
Conversation
The expression `if (allow_addons) 1 else 0 + conditions.len` parses as `if (allow_addons) 1 else (0 + conditions.len)`, so when allow_addons is true (the default), conditions.len was dropped from the capacity calculation and putAssumeCapacity would overflow the map when enough user 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 pre-allocation in the bundler's ESMConditions initialization to compute the extra capacity size once instead of repeatedly, and adds a regression test verifying that builds with 64 custom conditions complete successfully without crashing. ChangesCondition Map Capacity Refactoring
🚥 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 predates this and also fixes a second instance of the same 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 — straightforward operator-precedence fix with a regression test.
Extended reasoning...
Overview
This PR fixes a Zig if-expression precedence bug in ESMConditions.init (src/bundler/options.zig) where if (allow_addons) 1 else 0 + conditions.len parsed as if (allow_addons) 1 else (0 + conditions.len), causing ensureTotalCapacity to under-reserve and subsequent putAssumeCapacity calls to overflow when user-supplied conditions were present. The fix hoists the conditional into const addons: usize and uses plain addition. A subprocess-based regression test with 64 conditions is added to test/bundler/bun-build-api.test.ts.
Security risks
None. This is purely a capacity-hint arithmetic fix for an internal hash map. No user input handling, auth, crypto, or permission logic is touched. The change strictly increases reserved capacity to match what the existing code already inserts.
Level of scrutiny
Low. The diff is 4 lines of Zig that mechanically rewrite an expression to avoid a well-understood parser footgun, plus a test that mirrors an existing pattern in the same file (spawning a subprocess so a crash surfaces as a test failure). I verified the new capacity (defaults.len + 2 + addons + conditions.len) exactly covers the worst-case map (import/require: 1 sentinel + conditions + defaults + node-addons + "default") and over-covers the others.
Other factors
- No CODEOWNERS apply to the touched paths.
- The one inline comment is an FYI about a pre-existing, benign instance of the same pattern in
loadersFromTransformOptions; it's explicitly not a blocker and doesn't affect correctness. - No prior reviewer comments to address; CI is building.
| 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 addons: usize = if (allow_addons) 1 else 0; |
There was a problem hiding this comment.
🟣 Heads up: the same if ... else 0 + ... precedence footgun this PR fixes also exists in loadersFromTransformOptions in this file (lines 1575-1578). It's harmless there — the under-reserved capacity still covers the putAssumeCapacity calls and the remaining inserts use getOrPutValue which grows the map — so it's just a missed pre-allocation, but you may want to fix both occurrences together. (Pre-existing, not introduced by this PR.)
Extended reasoning...
What the bug is
This PR fixes a Zig operator-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 identical pattern still exists in the same file at loadersFromTransformOptions (src/bundler/options.zig:1575-1578):
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,In Zig grammar, an IfExpr's else-branch greedily consumes a full expression, so each trailing + ... binds into the preceding else rather than into the outer sum. The indentation suggests four addends, but the parser sees nested if-expressions.
Step-by-step proof
Take target = .bun (so target.isBun() is true, target == .browser is false). With default_loader_ext_bun.len = 2, default_loader_ext_browser.len = 1, default_loader_ext.len = 18:
- The expression is
extensions.len + (if (true) 2 else (0 + (if (false) 1 else (0 + 18)))). - The outer if takes the then branch →
2. Everything afterelse(includingdefault_loader_ext.len) is discarded. - Result:
extensions.len + 2. - Intended result:
extensions.len + 2 + 0 + 18 = extensions.len + 20.
So when target.isBun(), the capacity hint drops 18 entries. (When the target is neither bun nor browser, the nested elses do happen to sum to 0 + 0 + 18, which is accidentally correct.)
Why it doesn't crash
Unlike ESMConditions.init, this instance is benign:
stringHashMapFromArraysonly callsputAssumeCapacityforkeys=input_loaders.extensions. Sinceinput_loaders.extensions.lenis always the first addend (outside anyif), the reserved capacity is always ≥ the number ofputAssumeCapacitycalls regardless of how the rest mis-parses.- The subsequent
default_loader_ext/default_loader_ext_bun/default_loader_ext_browserinserts usetry loaders.getOrPutValue(...), which grows the map on demand. - Additionally, when
keys.len == 0(no user-supplied loaders, the common case),stringHashMapFromArraysskipsensureTotalCapacityentirely, so the hint is already best-effort.
Impact
Zero correctness impact — purely a missed pre-allocation that causes one or two extra rehashes during options setup. No crash, no behavioral difference.
How to fix
Same approach as this PR — hoist the conditionals into locals so the addition is unambiguous:
const bun_exts: usize = if (target.isBun()) default_loader_ext_bun.len else 0;
const browser_exts: usize = if (target == .browser) default_loader_ext_browser.len else 0;
// ...
input_loaders.extensions.len + bun_exts + browser_exts + default_loader_ext.len,This is pre-existing and untouched by the diff; mentioning it only because the PR's express purpose is eliminating this exact footgun in options.zig, so fixing both occurrences together seems natural. Not a blocker.
1 participant
What does this PR do?
Fixes a debug assertion / heap overflow in
ESMConditions.initwhenBun.build()is called with customconditions.The capacity reservation used:
which in Zig parses as:
Since
allow_addonsdefaults totrue,conditions.lenwas never counted toward the reserved capacity, and the subsequentputAssumeCapacitycalls overflowed the map once enough user conditions were supplied.How did you verify your code works?
Reproduced reliably with:
Crashes before, passes after. Added a regression test in
test/bundler/bun-build-api.test.ts.Found by Fuzzilli (fingerprint
1a2e21437cb0aea9).