bundler: fix ESMConditions capacity miscomputation with custom conditions - #30535
bundler: fix ESMConditions capacity miscomputation with custom conditions#30535robobun wants to merge 1 commit into
Conversation
…ions The expression `if (allow_addons) 1 else 0 + conditions.len` parses as `if (allow_addons) 1 else (0 + conditions.len)`, so when node-addons are enabled (the default), the user-provided conditions were not counted in the reserved capacity. With enough custom conditions, the subsequent putAssumeCapacity calls overflowed the map and asserted in debug builds.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR fixes a type-casting issue in ChangesESMConditions Capacity and Regression Test
🚥 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 catches 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 a pre-existing, harmless nit and optional.
Extended reasoning...
Overview
This PR touches two files: a 3-line change in src/bundler/options.zig wrapping if (allow_addons) 1 else 0 in @as(usize, ...) so Zig doesn't absorb the trailing + conditions.len into the else branch, and a ~15-line regression test in test/bundler/bun-build-api.test.ts that calls Bun.build with 12 custom conditions. The fix is mechanical and the explanation in the PR body matches Zig's actual if-else precedence semantics.
I verified the post-fix capacity arithmetic against the body of ESMConditions.init: each map's ensureTotalCapacity argument is now >= the number of subsequent putAssumeCapacity calls for every combination of allow_addons and conditions.len (import/require hit the bound exactly; default and style over-reserve by 1 and conditions.len respectively, which is safe).
Security risks
None. This only adjusts a capacity hint passed to ensureTotalCapacity for internal StringArrayHashMaps; it strictly increases the reservation relative to the buggy computation and cannot introduce new out-of-bounds behavior. No user input parsing, auth, or crypto is involved.
Level of scrutiny
Low. The diff is tiny, self-contained, and the transformation (@as(usize, if (...) 1 else 0)) is the idiomatic Zig fix for this exact precedence footgun. The added test is concurrent, uses a temp dir, and follows existing patterns in the same file.
Other factors
- No CODEOWNERS entries cover these paths.
- No outstanding human review comments; only the robobun build-status comment is on the timeline.
- The one bug-hunter finding is a pre-existing, correctness-neutral instance of the same pattern in
loadersFromTransformOptions(the defaults there usegetOrPutValue, which grows). It's a reasonable optional follow-up but does not block this fix.
| try default_condition_amp.ensureTotalCapacity(defaults.len + 2 + @as(usize, if (allow_addons) 1 else 0) + conditions.len); | ||
| try import_condition_map.ensureTotalCapacity(defaults.len + 2 + @as(usize, if (allow_addons) 1 else 0) + conditions.len); | ||
| try require_condition_map.ensureTotalCapacity(defaults.len + 2 + @as(usize, if (allow_addons) 1 else 0) + conditions.len); |
There was a problem hiding this comment.
🟣 Nit (pre-existing): the same if-else precedence footgun exists ~480 lines down in loadersFromTransformOptions's total_capacity argument — when target.isBun() is true, default_loader_ext.len (and the browser term) get absorbed into the else branch and dropped. It's harmless there since only the user-provided extensions use putAssumeCapacity and the defaults use getOrPutValue which grows the map, so it's just a missed pre-sizing — but since this PR is specifically about this pattern in this file, you may want to give it the same @as(usize, ...) treatment.
Extended reasoning...
Summary
This PR fixes the Zig if-else precedence bug in ESMConditions.init where A + if (cond) 1 else 0 + B parses as A + (if (cond) 1 else (0 + B)). However, the identical anti-pattern still exists in the same file in loadersFromTransformOptions (around 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,How it parses
Because Zig's if-else binds tighter on the else-branch than the surrounding +, the trailing additions are absorbed into the preceding else. So when target.isBun() is true, the whole expression evaluates to just input_loaders.extensions.len + default_loader_ext_bun.len (= extensions.len + 2), silently dropping both default_loader_ext.len (16) and the browser term. When target.isBun() is false and target == .browser, it evaluates to extensions.len + 0 + default_loader_ext_browser.len (= extensions.len + 1), again dropping default_loader_ext.len. Only the fall-through case (else 0 + else 0 + default_loader_ext.len) yields the +16 the author intended as the baseline.
Why it isn't a correctness bug here
Unlike ESMConditions.init, this miscomputation cannot corrupt memory or assert:
stringHashMapFromArrays(src/bundler/options.zig:41-51) only callsputAssumeCapacityfor the user-provided keys (input_loaders.extensions). The miscomputedtotal_capacityis always>= input_loaders.extensions.lenbecause that term is unconditional and first, and every if-branch contributes a non-negative addend. So theputAssumeCapacityloop is always covered.- The subsequent default-extension inserts (
default_loader_ext,default_loader_ext_bun,default_loader_ext_browser) all usetry loaders.getOrPutValue(...), which grows the map on demand.
So the only effect is a missed pre-sizing optimization — the map will rehash a couple of times during the default-extension inserts instead of being sized up-front.
Step-by-step example
Take target = .bun, input_loaders.extensions.len = 0:
- Author intent:
0 + 2 (bun) + 0 (browser) + 16 (default) = 18. - Actual parse: outer if takes the then branch →
0 + default_loader_ext_bun.len = 0 + 2 = 2. The entireelse 0 + if (...) ... + default_loader_ext.lentail is the unevaluated else-branch. stringHashMapFromArraysis called withtotal_capacity = 2andkeys.len = 0, soensureTotalCapacityisn't even called (gated onkeys.len > 0).- The 16
getOrPutValuecalls fordefault_loader_extthen grow the map from empty — correct, just not pre-sized.
Suggested fix
Apply the same treatment as in this PR:
input_loaders.extensions.len +
@as(usize, if (target.isBun()) default_loader_ext_bun.len else 0) +
@as(usize, if (target == .browser) default_loader_ext_browser.len else 0) +
default_loader_ext.len,This is pre-existing and nit-level — it has no correctness impact and shouldn't block the PR. It's only worth mentioning because the PR's entire purpose is fixing this exact precedence footgun in this exact file, so applying it consistently removes a known-misleading capacity computation.
1 participant
What does this PR do?
Fixes an operator precedence bug in
ESMConditions.initthat causedBun.build({ conditions: [...] })to assert (debug) or potentially corrupt memory (release) when enough custom conditions were passed.The capacity reservation used:
which in Zig parses as:
So when
allow_addonsis true (the default),conditions.lenwas dropped from the reserved capacity entirely. The subsequentputAssumeCapacitycalls then overflowed the reservation once the user-provided conditions plus the built-in defaults exceeded the miscomputed capacity.How did you verify your code works?
Added a regression test in
test/bundler/bun-build-api.test.tsthat builds with 12 custom conditions. It asserts on the unfixed debug binary and passes after the fix.Fuzzer fingerprint:
f0f7002504dcbd1d