Fix capacity miscalculation in ESMConditions.init - #30604
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 reserved capacity. With enough user-supplied conditions, putAssumeCapacity would overflow the backing array and hit a debug assert / write OOB.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR refactors ChangesESM Conditions 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 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 precedence fix with a regression test; the inline note about loadersFromTransformOptions is pre-existing and non-blocking.
Extended reasoning...
Overview
This PR fixes a Zig if-else precedence bug in ESMConditions.init (src/bundler/options.zig). The expression defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len was being parsed with the else-branch consuming 0 + conditions.len, so when allow_addons is true (the default) the user-supplied conditions.len was dropped from the reserved capacity, and subsequent putAssumeCapacity calls could overflow the backing array. The fix hoists the conditional into const addons_count: usize = if (allow_addons) 1 else 0; and uses it in the three ensureTotalCapacity calls — a 4-line, mechanically obvious change. A regression test in test/bundler/bun-build-api.test.ts exercises Bun.build with 0–16 conditions.
Security risks
None. This only adjusts an integer capacity hint passed to ensureTotalCapacity; the new value is strictly >= the old one in all branches, so it can only reserve more (correct) space, not less. No new inputs, no parsing changes, no auth/crypto/permissions involvement.
Level of scrutiny
Low. The diff is tiny, the root cause and fix are both fully explained in the PR description, and the corrected arithmetic is trivially verifiable against the putAssumeCapacity calls immediately below (1 for import/require, conditions.len, defaults.len, optional node-addons, and default → defaults.len + 2 + addons_count + conditions.len). The style_condition_map line correctly omits addons_count since node-addons is never inserted there.
Other factors
The one inline finding is explicitly a pre-existing, non-blocking nit about the same precedence pattern in loadersFromTransformOptions, which is harmless there because subsequent inserts use getOrPutValue (auto-grows). It's a reasonable "while you're here" but does not affect this PR's correctness. No CODEOWNERS apply to the touched files, and there are no outstanding reviewer comments.
| const addons_count: usize = if (allow_addons) 1 else 0; | ||
| try default_condition_amp.ensureTotalCapacity(defaults.len + 2 + addons_count + conditions.len); | ||
| try import_condition_map.ensureTotalCapacity(defaults.len + 2 + addons_count + conditions.len); | ||
| try require_condition_map.ensureTotalCapacity(defaults.len + 2 + addons_count + conditions.len); |
There was a problem hiding this comment.
🟣 Nit (pre-existing, not introduced here): the same if-else precedence trap also lives ~480 lines down in loadersFromTransformOptions — input_loaders.extensions.len + if (target.isBun()) ... else 0 + if (...) ... else 0 + default_loader_ext.len. That one is harmless because the subsequent inserts use getOrPutValue (which grows), so it just under-reserves and triggers a couple of extra reallocations rather than overflowing. But since it's the identical root cause in the same file, you may want to give it the same const x: usize = if (...) ... else 0; treatment while you're here.
Extended reasoning...
What the issue is
This PR correctly fixes the Zig if-else precedence bug in ESMConditions.init, where if (allow_addons) 1 else 0 + conditions.len parses as if (allow_addons) 1 else (0 + conditions.len). However, the exact same pattern exists further down in the same file, in loadersFromTransformOptions:
var loaders = try stringHashMapFromArrays(
bun.StringArrayHashMap(Loader),
allocator,
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,
input_loaders.extensions,
loader_values,
);In Zig, the else-branch of an if expression greedily consumes a full expression. So a + if (c) x else 0 + y parses as a + (if (c) x else (0 + y)), not (a + (if (c) x else 0)) + y. The chained ifs here therefore do not produce the sum the indentation suggests.
Step-by-step proof
Take target = .bun (so target.isBun() is true and target == .browser is false), with input_loaders.extensions.len = 3:
- The inner
if (target == .browser) default_loader_ext_browser.len else 0 + default_loader_ext.lenparses with the else-branch consuming0 + default_loader_ext.len. Since the condition is false, it evaluates to0 + 16 = 16. - The outer
if (target.isBun()) default_loader_ext_bun.len else 0 + (16)— condition is true, so it evaluates todefault_loader_ext_bun.len = 2. The else-branch (0 + 16) is discarded entirely. total_capacity = input_loaders.extensions.len + 2 = 5.
The intended value was 3 + 2 + 0 + 16 = 21. So the map is reserved for 5 entries instead of 21.
Why it doesn't crash (unlike the ESMConditions case)
stringHashMapFromArraysonly doesputAssumeCapacityforkeys.lenitems (the user-supplied extensions). Sinceextensions.lenis always a term in the sum regardless of which if-branch is taken, the reserved capacity is always>= extensions.len, so those inserts never overflow.- The subsequent
default_loader_ext/default_loader_ext_bun/default_loader_ext_browserinserts all usegetOrPutValue, which grows the map on demand. - Additionally, when
input_loaders.extensions.len == 0(no custom loaders),stringHashMapFromArraysskipsensureTotalCapacityentirely, so the reservation hint is ignored anyway.
So the only impact is a few unnecessary reallocations during build setup when custom loaders are passed — no correctness or safety issue.
Why mention it
It's the identical root cause this PR is fixing (Zig if-else precedence in a capacity calculation), in the same file, and the same fix style applies cleanly:
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 ...This is pre-existing — the PR doesn't touch, call, or otherwise interact with loadersFromTransformOptions — so it's purely a non-blocking "while you're here" suggestion, not something that should hold up the merge.
1 participant
What does this PR do?
Fixes a debug assert / potential OOB write in
Bun.build()/bun buildwhen passing several entries inconditions.In
ESMConditions.init:Zig parses
if (allow_addons) 1 else 0 + conditions.lenasif (allow_addons) 1 else (0 + conditions.len). Sinceallow_addonsdefaults totrue,conditions.lenwas effectively dropped from the reserved capacity for the default/import/require condition maps. With enough user-supplied conditions (≥4 fortarget: "bun"),putAssumeCapacityoverflows the backing array.Repro:
How did you verify your code works?
Added a regression test in
test/bundler/bun-build-api.test.tsthat builds with 0–16 conditions. Verified it crashes on main and passes with this fix.fuzzer