Skip to content

Fix crash in Bun.build() with many conditions #30557

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
+16 −3
Closed

Fix crash in Bun.build() with many conditions #30557

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
7 changes: 4 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,10 +1087,11 @@
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);
const addon_count: usize = if (allow_addons) 1 else 0;

Check warning on line 1090 in src/bundler/options.zig

View check run for this annotation

Claude / Claude Code Review

Same if-else precedence pattern remains in loadersFromTransformOptions 

nit (pre-existing): the same `if (cond) a else 0 + ...` precedence pattern this PR fixes also exists in `loadersFromTransformOptions` (~line 1575): `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 subsequent inserts use `getOrPutValue` (which grows) rather than `putAssumeCapacity`, so the only effect is an unnecessary reallocation — but since this PR is specific

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

🟡 nit (pre-existing): the same if (cond) a else 0 + ... precedence pattern this PR fixes also exists in loadersFromTransformOptions (~line 1575): 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 subsequent inserts use getOrPutValue (which grows) rather than putAssumeCapacity, so the only effect is an unnecessary reallocation — but since this PR is specifically about this precedence trap, it might be worth giving that callsite the same const x: usize = if (...) n else 0; treatment for consistency.

Extended reasoning...

What

loadersFromTransformOptions in the same file computes its hashmap capacity 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,

Per Zig grammar, an IfExpr's else branch consumes a full expression, so this parses as:

input_loaders.extensions.len + (if (target.isBun()) 2 else (0 + (if (target == .browser) 1 else (0 + 18))))

This is the exact precedence pitfall this PR fixes in ESMConditions.init.

Step-by-step

With target = .bun and a user-supplied loaders map of 1 entry:

  1. target.isBun() → true, so the whole if evaluates to default_loader_ext_bun.len = 2.
  2. The trailing + if (target == .browser) ... + default_loader_ext.len is part of the else branch and is never evaluated.
  3. Reserved capacity = 1 + 2 = 3.
  4. Intended capacity = 1 + 2 + 0 + 18 = 21.
  5. stringHashMapFromArrays calls ensureTotalCapacity(3) then putAssumeCapacity exactly input_loaders.extensions.len = 1 time → fits.
  6. The 18 default_loader_ext entries and 2 default_loader_ext_bun entries are inserted via getOrPutValue, which grows the map → no crash, just a redundant rehash/realloc.

(Note also that stringHashMapFromArrays only calls ensureTotalCapacity at all when input_loaders.extensions.len > 0, i.e. only when the user passes custom loaders.)

Why it doesn't crash here but did in ESMConditions

ESMConditions.init follows the under-reserved ensureTotalCapacity with putAssumeCapacity for all inserts, so under-allocation overflows the map and trips the assertion. loadersFromTransformOptions only putAssumeCapacitys the first input_loaders.extensions.len items — and that term is the unconditional first addend, always counted regardless of how the if parses — so the assumed-capacity inserts always fit. Everything after that uses getOrPutValue.

Addressing the objection

One verifier argued this shouldn't be flagged because there's no incorrect behavior — just a sub-optimal capacity hint. That's accurate: this is not a correctness bug and would not justify a standalone report. It's flagged here only because the PR's stated purpose is fixing this specific precedence trap in this file, and an identical instance sits ~500 lines away. Filing it as a nit / pre-existing so the author can decide whether to roll it into the same change; it should not block the PR.

Suggested fix

Same shape as the PR's fix:

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;
... input_loaders.extensions.len + bun_count + browser_count + default_loader_ext.len ...

try default_condition_amp.ensureTotalCapacity(defaults.len + 2 + addon_count + conditions.len);
try import_condition_map.ensureTotalCapacity(defaults.len + 2 + addon_count + conditions.len);
try require_condition_map.ensureTotalCapacity(defaults.len + 2 + addon_count + conditions.len);
try style_condition_map.ensureTotalCapacity(defaults.len + 2 + conditions.len);

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

View check run for this annotation

Claude / Claude Code Review

style_condition_map reserves capacity for user conditions but init() never inserts them 

Pre-existing, not introduced here, but since you're touching this exact block: `style_condition_map` reserves capacity for `conditions.len` on the line right below, yet the `for (conditions)` loop a few lines down only inserts into import/require/default — never style. Meanwhile `ESMConditions.appendSlice` *does* add to `self.style`, so whether a user condition applies to CSS `@import` package-exports resolution depends on which codepath populated the struct. Probably worth a follow-up (either a

@claude claude Bot May 12, 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, not introduced here, but since you're touching this exact block: style_condition_map reserves capacity for conditions.len on the line right below, yet the for (conditions) loop a few lines down only inserts into import/require/default — never style. Meanwhile ESMConditions.appendSlice does add to self.style, so whether a user condition applies to CSS @import package-exports resolution depends on which codepath populated the struct. Probably worth a follow-up (either add style_condition_map.putAssumeCapacity(condition, {}) to the loop, or drop + conditions.len from style's reservation if the omission is intentional).

Extended reasoning...

What the inconsistency is

ESMConditions.init reserves capacity in style_condition_map for the user-supplied conditions slice (src/bundler/options.zig:1094):

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

but the loop that actually inserts those conditions skips style entirely (lines 1100-1104):

for (conditions) |condition| {
    import_condition_map.putAssumeCapacity(condition, {});
    require_condition_map.putAssumeCapacity(condition, {});
    default_condition_amp.putAssumeCapacity(condition, {});
}

So style ends up with only defaults.len + 2 entries ("style", the target defaults, and "default"). The extra conditions.len of reserved capacity is never filled — harmless on its own, but it's a strong hint the loop was meant to insert into style too.

Why it's a behavioral inconsistency, not just over-reservation

ESMConditions.appendSlice (lines 1152-1163) and append (1166-1171) do insert into self.style:

pub fn appendSlice(self: *ESMConditions, conditions: []const string) bun.OOM!void {
    try self.default.ensureUnusedCapacity(conditions.len);
    try self.import.ensureUnusedCapacity(conditions.len);
    try self.require.ensureUnusedCapacity(conditions.len);
    try self.style.ensureUnusedCapacity(conditions.len);
    for (conditions) |condition| {
        ...
        self.style.putAssumeCapacity(condition, {});
    }
}

The resolver consults r.opts.conditions.style for ImportKind.at / .at_conditional (resolver.zig:1871), i.e. CSS @import "pkg" going through package.json "exports". So whether a given condition is honored for CSS resolution depends on which API added it:

Path Reaches style?
Bun.build({ conditions: ["custom"] }) → transform.conditions → ESMConditions.init ❌
conditions.appendSlice(&.{"development"}) (JSBundleCompletionTask.zig:217, build_command.zig:240, bake.zig:764) ✅
conditions.appendSlice(&.{"react-server"}) (bake.zig:760, build_command.zig:269) ✅
conditions.appendSlice(&.{"node"}) (bake.zig:769) ✅

Step-by-step example

  1. User calls Bun.build({ entrypoints: ["./app.css"], conditions: ["custom"] }).
  2. BundleOptions.fromApi calls ESMConditions.init(allocator, target.defaultConditions(), true, &.{"custom"}).
  3. style_condition_map reserves space for defaults.len + 2 + 1, but the for (conditions) loop never inserts "custom" into it. style ends up as {"style", "browser", "module", "default"}.
  4. app.css contains @import "some-pkg"; where some-pkg/package.json has "exports": { "custom": "./custom.css", "default": "./index.css" }.
  5. Resolver hits ImportKind.at → uses conditions.style → "custom" is absent → resolves to ./index.css.
  6. Yet the same "custom" condition is in import/require/default, so JS imports from the same build would see "custom". And if the condition had been added later via appendSlice (as the framework codepaths do for "development"), CSS would see it.

Why nothing prevents it

There's no fallback — style is the only map consulted for CSS @import. The over-reservation is silently tolerated because ensureTotalCapacity just allocates more than needed; nothing asserts that the reserved slots get used. The asymmetry with appendSlice/append has no guard either.

Impact

User-supplied conditions passed to Bun.build() (or bun build --conditions) are silently ignored for CSS @import package-exports resolution, while framework-injected conditions like development/react-server are honored. Niche but surprising; the capacity-reservation line and the symmetry with appendSlice both suggest the omission in the init loop was accidental.

Fix

Add the missing line to the loop:

for (conditions) |condition| {
    import_condition_map.putAssumeCapacity(condition, {});
    require_condition_map.putAssumeCapacity(condition, {});
    default_condition_amp.putAssumeCapacity(condition, {});
    style_condition_map.putAssumeCapacity(condition, {});
}

(Capacity is already reserved on line 1094, so putAssumeCapacity is safe.) Alternatively, if excluding user conditions from CSS resolution is intentional, drop + conditions.len from line 1094 and add a comment — but appendSlice's behavior argues against that being the intent.

Relationship to this PR

Not introduced here — the PR only touches lines 1090-1093. But line 1094 is literally the next line, in the same function, operating on the same conditions parameter that this PR is fixing the capacity math for. Flagging as adjacent context; should be its own PR rather than folded into this crash fix.


import_condition_map.putAssumeCapacity("import", {});
require_condition_map.putAssumeCapacity("require", {});
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 @@ -134,6 +134,18 @@ describe("Bun.build", () => {
).toThrow();
});

test("many conditions does not crash", async () => {
const dir = tempDirWithFiles("bun-build-api-conditions", {
"index.js": `console.log("hello");`,
});
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", "q", "r", "s", "t"],
});
expect(build.success).toBe(true);
expect(build.outputs).toHaveLength(1);
});

test("returns errors properly", async () => {
Bun.gc(true);
const build = await buildNoThrow({
Expand Down
Loading