Fix crash in Bun.build with custom conditions#30528
Conversation
ESMConditions.init computed the capacity for the condition maps as `defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len`, which Zig parses as `... + (if (allow_addons) 1 else (0 + conditions.len))`. When allow_addons was true (the default), conditions.len was dropped from the reservation and the subsequent putAssumeCapacity calls overflowed the MultiArrayList, crashing the bundler thread.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR refactors ChangesESMConditions capacity refactoring 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 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 — minimal, obviously-correct precedence fix with a regression test; the inline notes are non-blocking nits.
Extended reasoning...
Overview
This PR touches two files: a 4-line change in src/bundler/options.zig (ESMConditions.init) that hoists if (allow_addons) 1 else 0 into a named const addons_count: usize so the capacity arithmetic is unambiguous, and a new regression test in test/bundler/bun-build-api.test.ts that exercises Bun.build with several custom conditions in a subprocess.
Correctness
The Zig precedence issue is well-explained in the PR description and the fix is the canonical remedy — pull the conditional into a local. I verified the new reservation defaults.len + 2 + addons_count + conditions.len covers every putAssumeCapacity call that follows for the import/require/default maps (it actually over-reserves the default map by 1, which is harmless and pre-existing). The style map's reservation was already correct and is untouched. The test mirrors the existing loader map with an empty-string key subprocess test pattern in the same file.
Security risks
None. This is a capacity-reservation arithmetic fix in bundler option setup; no auth, crypto, parsing of untrusted input, or permission logic is involved. The previous behavior was an under-reservation (crash), and the fix only increases the reservation to the intended value.
Level of scrutiny
Low. The change is mechanical, ~4 effective lines of production code, fully covered by a new regression test, and the failure mode it fixes was a hard crash found by fuzzing. No design decisions are involved. Neither file is covered by CODEOWNERS.
Other factors
The two inline comments are non-blocking: one flags a pre-existing instance of the same precedence pattern elsewhere in the file (harmless there because it uses getOrPutValue, so just a missed pre-reservation), and the other is a style nit about test.concurrent. Neither affects the correctness of this fix and both can be addressed in a follow-up if desired.
| }, | ||
| ); | ||
|
|
||
| test("does not crash with many custom conditions", async () => { |
There was a problem hiding this comment.
🟡 nit: Per test/CLAUDE.md, tests that spawn processes or write files should use test.concurrent — the nearly-identical test just above (loader map with an empty-string key...) follows that pattern. Consider switching this to test.concurrent for consistency and to avoid serializing the suite.
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 does not crash with many custom conditions does both — it writes files via tempDirWithFiles and spawns a subprocess via Bun.spawn — but is declared with plain test() rather than test.concurrent().
Why this applies here
The most directly comparable test in the same describe block is loader map with an empty-string key is ignored without leaving uninitialized slots (test/bundler/bun-build-api.test.ts:390). It has effectively identical structure:
tempDirWithFiles(...)to create an isolated entry fileawait using proc = Bun.spawn({ cmd: [bunExe(), "-e", ...], env: bunEnv, stdout: "pipe", stderr: "pipe" })await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])- Assertions on stdout/stderr/exit code
That test uses test.concurrent, as do the surrounding subprocess-spawning tests in this file (rebuilding busts the directory entries cache, hash considers cross chunk imports, etc.). The new test is structurally indistinguishable from these and should follow the same convention.
Safety check
There is nothing preventing this test from running concurrently:
- It uses a unique temp directory (
tempDirWithFiles("bun-build-api-conditions", ...)), so there is no filesystem contention. - It does not call
process.chdiror mutate any global/shared state (unlike thetsconfig optiondescribe block, which intentionally stays sequential because it changes cwd). - The subprocess is fully isolated.
So the "unless it's very difficult" escape hatch in the guideline does not apply.
Impact
Purely a style/convention nit. The test is functionally correct as written; using plain test() just serializes it against other sequential tests in the file, marginally slowing the suite. The file is admittedly not 100% consistent (some older tests in this describe block predate the convention and still use plain test()), but both the documented guideline and the nearest precedent point to test.concurrent.
Fix
- test("does not crash with many custom conditions", async () => {
+ test.concurrent("does not crash with many custom conditions", async () => {| 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_count: usize = if (allow_addons) 1 else 0; |
There was a problem hiding this comment.
🟣 Heads up: the same if-expression precedence footgun also exists in loadersFromTransformOptions in this file (input_loaders.extensions.len + if (target.isBun()) default_loader_ext_bun.len else 0 + if (target == .browser) ... else 0 + default_loader_ext.len). It doesn't crash there because the inserts use getOrPutValue rather than putAssumeCapacity, so it just under-reserves and rehashes — but since this PR is specifically about this parsing gotcha, it might be worth fixing that occurrence too while you're here. (Pre-existing, not introduced by this PR.)
Extended reasoning...
What
This PR fixes a Zig if-expression precedence bug in ESMConditions.init where defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len parsed the else branch as (0 + conditions.len), dropping conditions.len from the reservation when allow_addons was true. The exact same pattern exists elsewhere in the same file, in loadersFromTransformOptions:
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,Why this is the same bug
Per the Zig grammar (and as the PR description itself explains), the else branch of an if expression consumes the full remaining expression. 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 example
Take target = .bun with no custom loaders (input_loaders.extensions.len == 0):
target.isBun()is true, so the firstifevaluates todefault_loader_ext_bun.len == 2.- Because the then-branch was taken, the entire trailing expression (the second
ifand+ default_loader_ext.len) is part of the untakenelsebranch and is skipped. - Total capacity passed to
stringHashMapFromArrays=0 + 2 = 2. - The intended capacity was
0 + 2 + 0 + 18 = 20(sincedefault_loader_ext.len == 18).
Similarly with target = .browser: the first if is false → 0 + (second if); the second if is true → default_loader_ext_browser.len == 1; default_loader_ext.len is again dropped. Total = extensions.len + 1 instead of extensions.len + 1 + 18.
Why it doesn't crash here
Unlike ESMConditions.init, this occurrence is harmless in practice:
stringHashMapFromArraysonly callsensureTotalCapacitywhenkeys.len > 0, and the onlyputAssumeCapacitycalls it makes are forinput_loaders.extensions— andtotal_capacity >= input_loaders.extensions.lenholds in every branch of the mis-parsed expression.- The subsequent inserts of
default_loader_ext,default_loader_ext_bun, anddefault_loader_ext_browserall usegetOrPutValue(withtry), which grows the map on demand rather than assuming capacity.
So the only effect is a missed pre-reservation and an extra rehash/realloc when custom loaders are passed with a bun/browser target — a minor perf inefficiency, not a correctness issue.
Suggested fix
Same approach as the PR's fix — hoist the conditionals into locals so the arithmetic is unambiguous:
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,
);Severity
Pre-existing — the PR doesn't touch loadersFromTransformOptions, and there's no functional/correctness impact. But it's the identical footgun, in the same file, that this PR is explicitly about, so it seemed worth flagging as a "while you're here".
1 participant
What
Fixes a crash in
Bun.build()(and anything going throughBundleOptions.fromApi) when passing customconditionswith native addons enabled (the default).Why
ESMConditions.initreserved capacity for its condition maps using:In Zig the
elsebranch of anifexpression consumes the full remaining expression, so this parses as:When
allow_addonswastrue,conditions.lenwas dropped from the reservation entirely. The subsequentputAssumeCapacitycalls then overflowed theMultiArrayListbacking store, tripping an assert in debug builds and segfaulting in release.How
Hoist the conditional into a
const addons_count: usizeso the arithmetic is unambiguous.Found by Fuzzilli (
f2ec65306ddfaf9f).