Skip to content

Fix crash in Bun.build when passing many custom conditions #30660

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 custom conditions #30660

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 remains in loadersFromTransformOptions 

Pre-existing, non-blocking: the identical `if (cond) X else 0 + Y` precedence footgun also exists in `loadersFromTransformOptions` in this same file (lines 1574-1577). It's harmless there — the under-reserved capacity is followed by `getOrPutValue` calls that grow the map, so it only costs an extra rehash — but since this PR exists specifically to eliminate this pattern, you may want to apply the same `@intFromBool` fix there for consistency.
Comment on lines +1090 to +1092

@claude claude Bot May 14, 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.

🟣 Pre-existing, non-blocking: the identical if (cond) X else 0 + Y precedence footgun also exists in loadersFromTransformOptions in this same file (lines 1574-1577). It's harmless there — the under-reserved capacity is followed by getOrPutValue calls that grow the map, so it only costs an extra rehash — but since this PR exists specifically to eliminate this pattern, you may want to apply the same @intFromBool fix there for consistency.

Extended reasoning...

What the issue is

This PR fixes a Zig precedence footgun in ESMConditions.init where if (allow_addons) 1 else 0 + conditions.len parses as if (allow_addons) 1 else (0 + conditions.len). The exact same construct appears ~480 lines later in the same file, in loadersFromTransformOptions (src/bundler/options.zig:1574-1577):

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,

Zig's else branch is greedy, so this parses 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))))

Step-by-step walkthrough

Take target = .bun (so target.isBun() == true):

  1. The outer if evaluates to default_loader_ext_bun.len (= 2).
  2. The entire else branch — including the nested browser check and default_loader_ext.len (= 16) — is skipped.
  3. Reserved capacity = input_loaders.extensions.len + 2.
  4. The intended capacity was input_loaders.extensions.len + 2 + 0 + 16 = input_loaders.extensions.len + 18.

Similarly for target = .browser: the inner if returns default_loader_ext_browser.len (= 1) and again drops default_loader_ext.len, giving extensions.len + 0 + 1 instead of extensions.len + 0 + 1 + 16. Only target = .node accidentally gets the right answer (both conditions false → 0 + 0 + 16).

Why this is not a crash (unlike the ESMConditions case)

The fixed bug in ESMConditions.init was dangerous because the under-reserved map was followed by putAssumeCapacity calls for the user-supplied conditions. Here the safety story is different:

  • stringHashMapFromArrays only calls putAssumeCapacity for input_loaders.extensions, and input_loaders.extensions.len is always the first addend, so it is always covered regardless of how the rest mis-parses.
  • The 16 default extensions, the 2 bun extensions, and the 1 browser extension are all inserted afterward via try loaders.getOrPutValue(...), which grows the map as needed.
  • Additionally, when no user loaders are passed (keys.len == 0, the common case), stringHashMapFromArrays skips ensureTotalCapacity entirely, so the hint is unused.

So the only effect is a wrong capacity hint causing one or two extra rehashes on a ~20-entry map during one-time options initialization — no observable behavior change, no crash, not measurable.

Addressing the "out of scope" objection

It's fair to note that this PR doesn't touch loadersFromTransformOptions, doesn't call into it, and the under-reservation there has zero functional impact — on its own it would not warrant a comment. The reason it's worth a non-blocking mention here is narrower than "same file": it is literally the same syntactic trap (else 0 + ...) that this PR's title and description identify as the root cause of a crash. Leaving an identical instance of the pattern a few hundred lines down means the next person to change those getOrPutValue calls to putAssumeCapacity (a plausible micro-optimization) reintroduces the exact crash this PR fixes. Applying the same @intFromBool rewrite there is a one-line consistency change, and the author already has the pattern in their editor.

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 equivalently, parenthesize each ternary). This is pre-existing and should not block the PR — purely a "while you're here" suggestion.

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

import_condition_map.putAssumeCapacity("import", {});
Expand Down
11 changes: 11 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 @@ -422,6 +422,17 @@ describe("Bun.build", () => {
expect(x.logs[0].position).toEqual(null);
});

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

test.concurrent("warnings do not fail a build", async () => {
const x = await Bun.build({
entrypoints: [join(import.meta.dir, "./fixtures/jsx-warning/index.jsx")],
Expand Down
Loading