bundler: fix capacity miscalculation in ESMConditions.init - #30577
bundler: fix capacity miscalculation in ESMConditions.init#30577robobun wants to merge 1 commit into
Conversation
The expression `if (allow_addons) 1 else 0 + conditions.len` parsed as `if (allow_addons) 1 else (0 + conditions.len)`, so when allow_addons was true the user-supplied conditions count was dropped from the reserved capacity. With enough custom conditions passed to Bun.build, the subsequent putAssumeCapacity calls overflowed and tripped an assertion (or wrote past the allocation in release).
|
Updated 12:10 PM PT - May 12th, 2026
❌ @robobun, your commit 2f1b793 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30577That installs a local version of the PR into your bun-30577 --bun |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughESMConditions capacity preallocation in ChangesESMConditions optimization
🚥 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 the second instance of this 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 notes are non-blocking.
Extended reasoning...
Overview
This PR fixes a Zig operator-precedence bug in ESMConditions.init (src/bundler/options.zig). The expression defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len parsed such that conditions.len was swallowed into the else branch, so when allow_addons was true (the default) the reserved capacity omitted the user-supplied conditions count entirely. Subsequent putAssumeCapacity calls then overflowed the allocation. The fix extracts @intFromBool(allow_addons) into a usize local and uses plain addition, making the arithmetic unambiguous. A 13-line regression test in test/bundler/bun-build-api.test.ts exercises Bun.build with 10 custom conditions.
Security risks
The change removes a heap buffer overflow reachable from user-controlled input (the conditions array passed to Bun.build). It introduces no new attack surface — it only corrects a capacity reservation. No auth, crypto, or permission code is touched.
Level of scrutiny
Low. This is a 4-line mechanical correction whose intent and correctness are verifiable by inspection: counting the putAssumeCapacity calls against each map confirms the new capacities are sufficient (and slightly over-reserve for default and style, which is harmless). The accompanying test is a minimal does-not-crash check with no shared state.
Other factors
Neither modified file is covered by CODEOWNERS. The two inline comments from the bug-hunting pass are non-blocking: one points out a pre-existing instance of the same precedence pattern elsewhere in the file (where it only defeats a pre-sizing optimization, not a safety issue), and the other is a test.concurrent style nit per test/CLAUDE.md. Both are reasonable follow-ups but do not affect the correctness of this fix.
| }, | ||
| ); | ||
|
|
||
| test("many custom conditions does not crash", async () => { |
There was a problem hiding this comment.
🟡 nit: Per test/CLAUDE.md, tests that write files should use test.concurrent when there's no shared state. This test uses a unique tempDirWithFiles dir with no process.chdir or shared globals, and the sibling tests in this describe block (e.g. ignoreDCEAnnotations works, hash considers cross chunk imports) already use test.concurrent for the same pattern.
Extended reasoning...
What
test/CLAUDE.md (line 22) states:
Prefer concurrent tests over sequential tests: When multiple tests in the same file spawn processes or write files, make them concurrent with
test.concurrentordescribe.concurrentunless it's very difficult to make them concurrent.
The new test added at test/bundler/bun-build-api.test.ts:653 writes files via tempDirWithFiles and runs Bun.build, but is declared with plain test(...) rather than test.concurrent(...).
Why it qualifies
The test creates its own isolated temp directory with a unique prefix (bun-build-api-many-conditions), does not call process.chdir, does not touch any module-level mutable state, and does not depend on ordering relative to other tests in the file. There is nothing that would make concurrency "very difficult" here — it is exactly the pattern the guideline targets.
Local precedent
Within the same describe("Bun.build") block, structurally identical tests already use test.concurrent:
hash considers cross chunk importsignoreDCEAnnotations worksemitDCEAnnotations worksloader map with an empty-string key is ignored ...rebuilding busts the directory entries cache
Each of these creates a temp dir, runs Bun.build, and asserts on the result — the same shape as the new test.
Step-by-step
- Test calls
tempDirWithFiles("bun-build-api-many-conditions", {...})→ unique per-invocation directory, no collision with concurrent siblings. - Test calls
await Bun.build({ entrypoints: [...], conditions: [...] })→ in-process build with no shared global state. - Test asserts
result.success === true→ pure read of the local result.
None of these steps require sequential execution; running concurrently with siblings cannot change the outcome.
Fix
- test("many custom conditions does not crash", async () => {
+ test.concurrent("many custom conditions does not crash", async () => {This is purely a style/convention nit per the repo's test guidelines — the test is functionally correct as written.
| const addon_count = @as(usize, @intFromBool(allow_addons)); | ||
| 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); |
There was a problem hiding this comment.
🟣 Heads up: the same if (cond) X else 0 + Y precedence pattern this PR fixes still exists ~480 lines down in loadersFromTransformOptions (the stringHashMapFromArrays capacity argument). It doesn't crash there because the subsequent inserts use getOrPutValue which grows on demand, so it only defeats the pre-sizing — but since this PR exists specifically to fix this pattern, it might be worth applying the same parenthesization/extraction there so the broken pattern doesn't get copied again.
Extended reasoning...
Same precedence bug pattern remains in loadersFromTransformOptions
This PR correctly fixes the if (cond) X else 0 + Y operator-precedence bug in ESMConditions.init. However, the identical pattern still exists 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,
);How it parses
In Zig, an if expression's else branch consumes a full expression, and the whole if is then a primary expression on the left side of +. So this parses as:
input_loaders.extensions.len + (
if (target.isBun())
default_loader_ext_bun.len // = 2
else
0 + (
if (target == .browser)
default_loader_ext_browser.len // = 1
else
0 + default_loader_ext.len // = 18
)
)
Step-by-step proof
Take target = .bun (so target.isBun() is true) and input_loaders.extensions.len = 0:
- The outer
iftakes the then branch → yieldsdefault_loader_ext_bun.len = 2. - The trailing
+ if (target == .browser) ... + default_loader_ext.lenis entirely inside the else branch, so it is never evaluated. - Total capacity passed =
0 + 2 = 2. - Intended capacity =
0 + 2 + 0 + 18 = 20.
Similarly for target = .browser: the inner if takes the then branch (= 1), dropping default_loader_ext.len, so capacity = 0 + 1 instead of 0 + 1 + 18.
Why it doesn't crash here
Unlike ESMConditions.init, this site does not overflow:
stringHashMapFromArraysonly callsputAssumeCapacityfor theinput_loaders.extensionskeys, andinput_loaders.extensions.lenis the one term that is always included in the sum regardless of which branch is taken.- All subsequent inserts (
default_loader_ext,default_loader_ext_bun,default_loader_ext_browser) go throughtry loaders.getOrPutValue(...), which grows the map on demand.
So the only effect is that ensureTotalCapacity reserves too little and the map reallocates a few times during the getOrPutValue loop — a defeated optimization, not a memory-safety bug.
Why flag it on this PR
This is pre-existing and not introduced here. But it is the exact bug class this PR exists to fix, in the same file. Fixing it alongside (e.g. by extracting the conditional counts into usize locals, mirroring the addon_count approach) would prevent the next reader from copying the broken pattern back into a putAssumeCapacity context.
Suggested 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;
var loaders = try stringHashMapFromArrays(
bun.StringArrayHashMap(Loader),
allocator,
input_loaders.extensions.len + bun_count + browser_count + default_loader_ext.len,
input_loaders.extensions,
loader_values,
);1 participant
Fuzzer fingerprint:
800f7f8cb62e7ad8What
ESMConditions.initreserved capacity with:In Zig this parses as
defaults.len + 2 + (if (allow_addons) 1 else (0 + conditions.len)). Whenallow_addonsis true (the default), the user-suppliedconditions.lenwas dropped entirely from the reserved capacity. The subsequentputAssumeCapacitycalls then wrote past the allocation once enough custom conditions were provided, tripping a debug assertion inMultiArrayList.addOneAssumeCapacity(and a heap buffer overflow in release).Minimal repro:
Fix
Pull the addon count out into a
usizeso the addition is unambiguous.