Fix capacity under-reservation in ESMConditions.init#30531
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 user-provided conditions were not counted toward the reserved capacity. With enough conditions passed to Bun.build(), putAssumeCapacity would overflow the backing MultiArrayList and trip a debug assertion (or write past the end in release).
WalkthroughThis PR improves capacity planning in ChangesESMConditions Capacity Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/bundler/bun-build-api.test.ts`:
- Around line 658-667: The test currently runs nested loops over targets and
counts inside one test body, which hides which specific (target, count) failed;
refactor the looped scenarios into parameterized tests using describe.each or
test.each so each (target, count) pair is a separate test case. Replace the for
(...) loops that call Bun.build({ entrypoints: [join(dir, "entry.js")], target,
conditions }) with a data table of [target, count] and use test.each to generate
individual tests that construct conditions (Array.from({ length: count }, (_, i)
=> `custom${i}`)), call Bun.build, and assert result.success; keep the same
assertion and inputs but ensure each case is reported independently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3a72f41-f717-494d-a1c1-f0ca465f94a4
📒 Files selected for processing (2)
src/bundler/options.zigtest/bundler/bun-build-api.test.ts
| for (const target of ["node", "bun", "browser"] as const) { | ||
| for (let count = 1; count <= 20; count++) { | ||
| const conditions = Array.from({ length: count }, (_, i) => `custom${i}`); | ||
| const result = await Bun.build({ | ||
| entrypoints: [join(dir, "entry.js")], | ||
| target, | ||
| conditions, | ||
| }); | ||
| expect(result.success).toBe(true); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use parameterized cases instead of nested loops for clearer failures.
This currently packs 60 scenarios into one test body, so a single failure obscures which target/count case broke. Prefer describe.each()/test.each() so each case is independently reported.
As per coding guidelines: "Use describe.each() for parameterized tests".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/bundler/bun-build-api.test.ts` around lines 658 - 667, The test
currently runs nested loops over targets and counts inside one test body, which
hides which specific (target, count) failed; refactor the looped scenarios into
parameterized tests using describe.each or test.each so each (target, count)
pair is a separate test case. Replace the for (...) loops that call Bun.build({
entrypoints: [join(dir, "entry.js")], target, conditions }) with a data table of
[target, count] and use test.each to generate individual tests that construct
conditions (Array.from({ length: count }, (_, i) => `custom${i}`)), call
Bun.build, and assert result.success; keep the same assertion and inputs but
ensure each case is reported independently.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #30466, which also catches the second instance of this 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.
Extended reasoning...
Overview
This PR fixes a Zig operator-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), dropping conditions.len from the capacity reservation whenever allow_addons was true. The fix extracts @as(usize, @intFromBool(allow_addons)) into a local and adds it explicitly, removing the ambiguity. A regression test sweeps 1–20 user conditions across node/bun/browser targets.
Security risks
None introduced. This actually fixes a memory-safety issue: the under-reservation led to putAssumeCapacity writing past the backing allocation in release builds (and tripping an assertion in debug). The new arithmetic is straightforward and matches the actual number of putAssumeCapacity calls that follow.
Level of scrutiny
Low-to-moderate. The diff is 4 lines of arithmetic in option setup plus a self-contained test. The transformation from if (b) 1 else 0 to @intFromBool(b) is mechanical and idiomatic Zig; the resulting sum (defaults.len + 2 + addons_count + conditions.len) is easy to verify against the subsequent inserts. No control-flow, API, or behavioral changes beyond reserving the correct capacity.
Other factors
- The one inline finding is explicitly pre-existing and benign (a similar precedence pattern in
loadersFromTransformOptionsthat only causes a harmless rehash because it usesgetOrPutValue); it's a non-blocking suggestion, not a defect in this change. - CodeRabbit's nit about
test.eachis a style preference and doesn't affect correctness. - A duplicate-PR bot flagged #30466 as covering the same fix — maintainers may want to dedupe, but that doesn't change the correctness of this patch.
- Author verified the new test segfaults on unpatched canary and passes after the fix; existing conditions tests still pass.
| 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 = @as(usize, @intFromBool(allow_addons)); |
There was a problem hiding this comment.
🟣 Pre-existing (not introduced by this PR), but the same if (x) a else 0 + b precedence footgun also exists ~480 lines down 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. That instance is benign (the subsequent inserts use getOrPutValue, which grows, so it just under-reserves and rehashes rather than corrupting memory), but since this PR is specifically about fixing this pattern in this file, it might be worth applying the same @intFromBool treatment there for consistency.
Extended reasoning...
What the issue is
This PR fixes a Zig operator-precedence bug in ESMConditions.init where if (allow_addons) 1 else 0 + conditions.len was being parsed as if (allow_addons) 1 else (0 + conditions.len). 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,
);How it parses
In Zig, the else branch of an if expression consumes a full Expr, so if (c) a else b + x parses as if (c) a else (b + x). Walking through the cases:
target.isBun()is true: the outerifevaluates todefault_loader_ext_bun.len(which is 2), and everything in theelsebranch — including the nestedifand+ default_loader_ext.len— is skipped. Result:extensions.len + 2, when the intended value isextensions.len + 2 + 0 + 18 = extensions.len + 20.target == .browser: outerifis false, so we evaluate0 + (if (target == .browser) default_loader_ext_browser.len else ...), which is0 + 1. Result:extensions.len + 1, intendedextensions.len + 19.target == .node: both conditions false, so0 + (0 + default_loader_ext.len)=extensions.len + 18. Coincidentally correct.
Why it doesn't crash
Unlike ESMConditions.init, this under-reservation is harmless at runtime:
stringHashMapFromArraysonly callsputAssumeCapacityforkeys.lenitems (i.e.input_loaders.extensions.len), and the computed capacity is always ≥extensions.lenin every branch above, so the assume-capacity inserts are within bounds.- The subsequent
default_loader_ext/default_loader_ext_bun/default_loader_ext_browserinserts all usetry loaders.getOrPutValue(...), which grows the map on demand.
So the only effect is one or two unnecessary rehashes during the loop — no UB, no observable misbehavior.
Why mention it
This is pre-existing and not introduced by this PR. But the PR's entire purpose is fixing this exact if (x) a else 0 + b precedence trap in this file, and the author already has the fix pattern (@intFromBool) right above. Applying the same treatment here would prevent someone from later "optimizing" the getOrPutValue calls to putAssumeCapacity and reintroducing the same class of bug.
Suggested fix
input_loaders.extensions.len +
@as(usize, @intFromBool(target.isBun())) * default_loader_ext_bun.len +
@as(usize, @intFromBool(target == .browser)) * default_loader_ext_browser.len +
default_loader_ext.len,or simply parenthesize each if expression. Non-blocking either way.
1 participant
What does this PR do?
Fixes a debug assertion / out-of-bounds write in
Bun.build()when passing several customconditions.In
ESMConditions.init, the capacity reservation used:which Zig parses as:
So when
allow_addonsistrue(the default forBun.build),conditions.lenwas dropped from the reservation entirely. The subsequentputAssumeCapacitycalls would then exceed the backingMultiArrayListcapacity once enough user conditions were supplied, trippingassert(self.len < self.capacity)in debug (and writing past the allocation in release).Repro that crashes before this change:
How did you verify your code works?
bun bd test test/bundler/bun-build-api.test.ts— all 39 pass + 1 todo, including the newmany conditions does not crashtest which sweeps 1–20 conditions acrossnode/bun/browsertargets.USE_SYSTEM_BUN=1).bun bd test test/bundler/esbuild/packagejson.test.ts -t Conditions— existing conditions tests still pass.Fuzzer fingerprint:
c9b09baffd7363d6