Skip to content

Fix crash in Bun.build when passing many conditions - #30498

Closed
robobun wants to merge 1 commit into
mainfrom
farm/492ed177/fix-esm-conditions-capacity
Closed

Fix crash in Bun.build when passing many conditions#30498
robobun wants to merge 1 commit into
mainfrom
farm/492ed177/fix-esm-conditions-capacity

Conversation

@robobun

@robobun robobun commented May 11, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a crash in Bun.build() (and the CLI bundler) when the user passes enough conditions to exceed the under-reserved capacity of the ESM conditions maps.

Root cause

In ESMConditions.init, the capacity reservation used:

defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len

Due to Zig's if-expression parsing, this is actually:

defaults.len + 2 + (if (allow_addons) 1 else (0 + conditions.len))

When allow_addons is true (the default), conditions.len was dropped from the capacity calculation entirely. The subsequent putAssumeCapacity calls then overflowed the map, tripping an assertion in debug builds and corrupting memory / segfaulting in release builds.

Fix

Replace the if expression with @intFromBool(allow_addons), matching the style already used elsewhere in this file and avoiding the precedence footgun.

How did you verify your code works?

Added a regression test in test/bundler/bun-build-api.test.ts that calls Bun.build with 16 conditions. This segfaults on current canary and passes with the fix.

Also verified existing packagejson/ExportsCustomConditions* bundler tests still pass.

Found by Fuzzilli (fingerprint d5aa656b6197a654).

…s true

The expression `defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len`
was parsed as `defaults.len + 2 + (if (allow_addons) 1 else (0 + conditions.len))`,
dropping conditions.len from the reserved capacity when allow_addons was true
(the default). With enough user-supplied conditions, putAssumeCapacity would
overflow the map and crash.
@robobun

robobun commented May 11, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 7:18 AM PT - May 11th, 2026

@robobun, your commit 8fba47a has some failures in Build #53342 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 30498

That installs a local version of the PR into your bun-30498 executable, so you can run:

bun-30498 --bun

@coderabbitai

coderabbitai Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 20515ac6-9c62-4cdf-b96a-0583554e2e1e

📥 Commits

Reviewing files that changed from the base of the PR and between 450072b and 8fba47a.

📒 Files selected for processing (2)
  • src/bundler/options.zig
  • test/bundler/bun-build-api.test.ts

Walkthrough

ESMConditions.init now uses @intFromBool for capacity calculation instead of if/else expressions. A regression test validates Bun.build succeeds when building with many conditions.

Changes

ESMConditions Capacity Calculation

Layer / File(s) Summary
Capacity Calculation Refactor
src/bundler/options.zig
ESMConditions.init replaces if (allow_addons) 1 else 0 with @as(usize, @intFromBool(allow_addons)) for capacity preallocation of default_condition_map, import_condition_map, and require_condition_map.
Regression Test
test/bundler/bun-build-api.test.ts
New test "many conditions does not crash" builds a simple JS entrypoint with a long conditions array and asserts the build succeeds with exactly one output.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the main fix: resolving a crash in Bun.build when many conditions are passed, which matches the core change across both files.
Description check ✅ Passed The description comprehensively covers both required sections: 'What does this PR do?' explains the fix with root cause analysis, and 'How did you verify your code works?' details the regression test and verification approach.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@robobun

robobun commented May 11, 2026

Copy link
Copy Markdown
Collaborator Author

Duplicate of #30466.

@robobun robobun closed this May 11, 2026
@robobun
robobun deleted the farm/492ed177/fix-esm-conditions-capacity branch May 11, 2026 14:17
@github-actions

github-actions Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Fix crash in Bun.build with many conditions #30466 - Fixes the same ESMConditions.init() operator precedence bug in src/bundler/options.zig with the same @intFromBool(allow_addons) approach

🤖 Generated with Claude Code

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — targeted fix for the Zig if-expression precedence footgun in capacity reservation, with a regression test.

Extended reasoning...

Overview

This PR fixes a crash in Bun.build() when many conditions are passed. The root cause is a Zig if-expression precedence footgun in ESMConditions.init (src/bundler/options.zig): defaults.len + 2 + 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 is dropped from the capacity reservation and subsequent putAssumeCapacity calls overflow the map. The fix replaces the if-expression with @as(usize, @intFromBool(allow_addons)), which is unambiguous and matches the style used elsewhere in the file. A regression test in bun-build-api.test.ts exercises 16 conditions.

Security risks

None. This is a pure arithmetic fix to a hashmap capacity reservation. No user input parsing, auth, crypto, or permissions are involved. The change strictly increases the reserved capacity to the intended value.

Level of scrutiny

Low. The diff is 3 identical one-line expression rewrites plus a 12-line regression test. I verified the new capacity matches the actual number of putAssumeCapacity calls that follow (1 sentinel + conditions.len + defaults.len + optional node-addons + defaultdefaults.len + 2 + intFromBool(allow_addons) + conditions.len for all three maps). The style_condition_map line is untouched and was already correct (it doesn't insert node-addons or user conditions).

Other factors

The one inline comment from the bug-hunting system is explicitly flagged as pre-existing and not a blocker — it points out the same precedence pattern in loadersFromTransformOptions, but that site uses getOrPutValue (which grows the map) so there's no correctness impact, just a missed pre-reservation. It's a reasonable cleanup suggestion but orthogonal to this fix. No CODEOWNERS apply, no outstanding reviewer comments, and CodeRabbit found nothing actionable.

Comment thread src/bundler/options.zig
Comment on lines +1090 to +1092
try default_condition_amp.ensureTotalCapacity(defaults.len + 2 + @as(usize, @intFromBool(allow_addons)) + conditions.len);
try import_condition_map.ensureTotalCapacity(defaults.len + 2 + @as(usize, @intFromBool(allow_addons)) + conditions.len);
try require_condition_map.ensureTotalCapacity(defaults.len + 2 + @as(usize, @intFromBool(allow_addons)) + conditions.len);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Heads-up (pre-existing, not a blocker): the same if-expression precedence footgun this PR fixes also lives in loadersFromTransformOptions() at src/bundler/options.zig:1574-1577 — when target.isBun() is true, the else branch greedily swallows the trailing + ... + default_loader_ext.len, so the capacity reservation drops 16+ entries. It doesn't crash there because the subsequent inserts use getOrPutValue() (which grows the map) rather than putAssumeCapacity(), so the only effect is a missed pre-reservation; might be worth applying the same @intFromBool fix while you're here for consistency.

Extended reasoning...

What & where

loadersFromTransformOptions() computes the initial capacity for the loaders map as:

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,

This is the identical Zig if-expression precedence footgun that this PR just fixed in ESMConditions.init. Zig parses the else branch greedily, so the trailing + expressions bind into the else rather than into the outer sum.

Step-by-step evaluation

Take target = .bun (so target.isBun() == true, target == .browser == false), default_loader_ext_bun.len = 2, default_loader_ext_browser.len = 1, default_loader_ext.len = 18, and say input_loaders.extensions.len = 0:

  1. Innermost: if (target == .browser) 1 else (0 + 18) → since false, evaluates to 18. (This branch is fine-ish on its own.)
  2. Next: if (target.isBun()) 2 else (0 + <inner>) → since true, evaluates to 2. The entire else (0 + 18) arm — including default_loader_ext.len — is discarded.
  3. Outer: 0 + 2 = 2.

The author clearly intended 0 + 2 + 0 + 18 = 20. So the map is reserved for 2 entries instead of 20.

Why it doesn't crash (addressing the "already handled" objection)

Unlike ESMConditions.init, this site is saved by two things:

  • stringHashMapFromArrays only calls putAssumeCapacity for the input_loaders.extensions keys, and input_loaders.extensions.len is the one term that's always included in the sum (it's outside the if), so those inserts never overflow.
  • The subsequent default_loader_ext / default_loader_ext_bun / default_loader_ext_browser inserts use getOrPutValue(), which grows the map on demand.

So there is no correctness or memory-safety impact today — just a few unnecessary rehashes during option setup. I agree with the objection that this is not a functional bug.

Why it's still worth mentioning on this PR

This PR's entire premise is that a + if (b) 1 else 0 + c is a footgun that silently miscomputes capacity and that @intFromBool is the safer spelling. The same miscomputed-by-construction expression sits ~480 lines away in the same file. The intent of that code was clearly to reserve space for all the default extensions; it just happens that the consumer is forgiving. If anyone later swaps getOrPutValue for putAssumeCapacity (matching the pattern this PR just fixed), it becomes the same crash.

Suggested fix

Same transformation as in this PR:

input_loaders.extensions.len +
    default_loader_ext_bun.len * @as(usize, @intFromBool(target.isBun())) +
    default_loader_ext_browser.len * @as(usize, @intFromBool(target == .browser)) +
    default_loader_ext.len,

(or just parenthesize the if expressions). Purely a consistency/cleanup suggestion — not a blocker for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant