Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/bundler/options.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,9 +1087,9 @@
var require_condition_map = ConditionsMap.init(allocator);
var style_condition_map = ConditionsMap.init(allocator);

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);
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);

Check notice on line 1092 in src/bundler/options.zig

View check run for this annotation

Claude / Claude Code Review

Same if-else precedence footgun in loadersFromTransformOptions 

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 m
Comment on lines +1090 to +1092

@claude claude Bot May 11, 2026

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.

try style_condition_map.ensureTotalCapacity(defaults.len + 2 + conditions.len);

import_condition_map.putAssumeCapacity("import", {});
Expand Down
12 changes: 12 additions & 0 deletions test/bundler/bun-build-api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,18 @@ describe("Bun.build", () => {
throw new Error("should have thrown");
});

test("many conditions does not crash", async () => {
const dir = tempDirWithFiles("bun-build-api-many-conditions", {
"index.js": `export default 1;`,
});
const build = await Bun.build({
entrypoints: [join(dir, "index.js")],
conditions: ["a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p"],
});
expect(build.success).toBe(true);
expect(build.outputs).toHaveLength(1);
});

// https://github.com/oven-sh/bun/issues/12818
test("sourcemap + build error crash case", async () => {
const dir = tempDirWithFiles("build", {
Expand Down
Loading