Skip to content

Fix crash in Bun.build with custom conditions #30528

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 with custom conditions #30528

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,9 +1087,10 @@
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 addons_count: usize = if (allow_addons) 1 else 0;

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

View check run for this annotation

Claude / Claude Code Review

Same if-expression precedence bug in loadersFromTransformOptions 

Heads up: the same `if`-expression precedence footgun also exists in `loadersFromTransformOptions` in this file (`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 inserts use `getOrPutValue` rather than `putAssumeCapacity`, so it just under-reserves and rehashes — but since this PR is specifically about this parsing gotcha, it might be worth fixing that occurren

@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.

🟣 Heads up: the same if-expression precedence footgun also exists in loadersFromTransformOptions in this file (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 inserts use getOrPutValue rather than putAssumeCapacity, so it just under-reserves and rehashes — but since this PR is specifically about this parsing gotcha, it might be worth fixing that occurrence too while you're here. (Pre-existing, not introduced by this PR.)

Extended reasoning...

What

This PR fixes a Zig if-expression precedence bug in ESMConditions.init where defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len parsed the else branch as (0 + conditions.len), dropping conditions.len from the reservation when allow_addons was true. The exact same pattern exists elsewhere in the same file, 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,

Why this is the same bug

Per the Zig grammar (and as the PR description itself explains), the else branch of an if expression consumes the full remaining expression. So this parses as:

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

Step-by-step example

Take target = .bun with no custom loaders (input_loaders.extensions.len == 0):

  1. target.isBun() is true, so the first if evaluates to default_loader_ext_bun.len == 2.
  2. Because the then-branch was taken, the entire trailing expression (the second if and + default_loader_ext.len) is part of the untaken else branch and is skipped.
  3. Total capacity passed to stringHashMapFromArrays = 0 + 2 = 2.
  4. The intended capacity was 0 + 2 + 0 + 18 = 20 (since default_loader_ext.len == 18).

Similarly with target = .browser: the first if is false → 0 + (second if); the second if is true → default_loader_ext_browser.len == 1; default_loader_ext.len is again dropped. Total = extensions.len + 1 instead of extensions.len + 1 + 18.

Why it doesn't crash here

Unlike ESMConditions.init, this occurrence is harmless in practice:

  1. stringHashMapFromArrays only calls ensureTotalCapacity when keys.len > 0, and the only putAssumeCapacity calls it makes are for input_loaders.extensions — and total_capacity >= input_loaders.extensions.len holds in every branch of the mis-parsed expression.
  2. The subsequent inserts of default_loader_ext, default_loader_ext_bun, and default_loader_ext_browser all use getOrPutValue (with try), which grows the map on demand rather than assuming capacity.

So the only effect is a missed pre-reservation and an extra rehash/realloc when custom loaders are passed with a bun/browser target — a minor perf inefficiency, not a correctness issue.

Suggested fix

Same approach as the PR's fix — hoist the conditionals into locals so the arithmetic is unambiguous:

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;
var loaders = try stringHashMapFromArrays(
    bun.StringArrayHashMap(Loader),
    allocator,
    input_loaders.extensions.len + bun_count + browser_count + default_loader_ext.len,
    input_loaders.extensions,
    loader_values,
);

Severity

Pre-existing — the PR doesn't touch loadersFromTransformOptions, and there's no functional/correctness impact. But it's the identical footgun, in the same file, that this PR is explicitly about, so it seemed worth flagging as a "while you're here".

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

import_condition_map.putAssumeCapacity("import", {});
Expand Down
30 changes: 30 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 @@ -649,6 +649,36 @@
expect(await html?.text()).toContain("<meta name='injected-by-plugin' content='true'>");
},
);

test("does not crash with many custom conditions", async () => {

Check warning on line 653 in test/bundler/bun-build-api.test.ts

View check run for this annotation

Claude / Claude Code Review

New test should use test.concurrent 

nit: Per `test/CLAUDE.md`, tests that spawn processes or write files should use `test.concurrent` — the nearly-identical test just above (`loader map with an empty-string key...`) follows that pattern. Consider switching this to `test.concurrent` for consistency and to avoid serializing the suite.

@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: Per test/CLAUDE.md, tests that spawn processes or write files should use test.concurrent — the nearly-identical test just above (loader map with an empty-string key...) follows that pattern. Consider switching this to test.concurrent for consistency and to avoid serializing the suite.

Extended reasoning...

What

test/CLAUDE.md (line 22) states:

Prefer concurrent tests over sequential tests: When multiple tests in the same file spawn processes or write files, make them concurrent with test.concurrent or describe.concurrent unless it's very difficult to make them concurrent.

The new test does not crash with many custom conditions does both — it writes files via tempDirWithFiles and spawns a subprocess via Bun.spawn — but is declared with plain test() rather than test.concurrent().

Why this applies here

The most directly comparable test in the same describe block is loader map with an empty-string key is ignored without leaving uninitialized slots (test/bundler/bun-build-api.test.ts:390). It has effectively identical structure:

  1. tempDirWithFiles(...) to create an isolated entry file
  2. await using proc = Bun.spawn({ cmd: [bunExe(), "-e", ...], env: bunEnv, stdout: "pipe", stderr: "pipe" })
  3. await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])
  4. Assertions on stdout/stderr/exit code

That test uses test.concurrent, as do the surrounding subprocess-spawning tests in this file (rebuilding busts the directory entries cache, hash considers cross chunk imports, etc.). The new test is structurally indistinguishable from these and should follow the same convention.

Safety check

There is nothing preventing this test from running concurrently:

  • It uses a unique temp directory (tempDirWithFiles("bun-build-api-conditions", ...)), so there is no filesystem contention.
  • It does not call process.chdir or mutate any global/shared state (unlike the tsconfig option describe block, which intentionally stays sequential because it changes cwd).
  • The subprocess is fully isolated.

So the "unless it's very difficult" escape hatch in the guideline does not apply.

Impact

Purely a style/convention nit. The test is functionally correct as written; using plain test() just serializes it against other sequential tests in the file, marginally slowing the suite. The file is admittedly not 100% consistent (some older tests in this describe block predate the convention and still use plain test()), but both the documented guideline and the nearest precedent point to test.concurrent.

Fix

-  test("does not crash with many custom conditions", async () => {
+  test.concurrent("does not crash with many custom conditions", async () => {

// ESMConditions.init under-reserved capacity when allow_addons was true
// (the default) due to `if`-expression precedence, so passing several
// custom conditions overflowed putAssumeCapacity and crashed the bundler
// thread. Run in a subprocess since the crash aborts the whole process.
const dir = tempDirWithFiles("bun-build-api-conditions", {
"entry.ts": "export const x = 1;",
});
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const result = await Bun.build({
entrypoints: [${JSON.stringify(join(dir, "entry.ts"))}],
conditions: ["a", "b", "c", "d", "e", "f", "g", "h"],
});
console.log("success:" + result.success);
`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(stdout.trim()).toBe("success:true");
expect(exitCode).toBe(0);
});
});

test.concurrent("macro with nested object", async () => {
Expand Down
Loading