Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 1 addition & 1 deletion src/resolver/resolver.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1904,7 +1904,7 @@ pub const Resolver = struct {
dir_info = source_dir_info;

// this is the magic!
if (global_cache.canUse(any_node_modules_folder) and r.usePackageManager() and esm_ != null) {
if (global_cache.canUse(any_node_modules_folder) and r.usePackageManager() and esm_ != null and strings.isNPMPackageName(esm_.?.name)) {
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.

🟣 The new gate at resolver.zig:1907 relies on strings.isNPMPackageName, but that function has a pre-existing bug where empty-scope specifiers like @/pkg incorrectly pass the check. This means @/pkg-style specifiers still reach the auto-install path, potentially triggering the same reentrant EventLoop.tick() SIGSEGV this PR aims to prevent. The fix in isNPMPackageNameIgnoreLength should use slash_index > 1 instead of slash_index > 0 to require at least one character between @ and /.

Extended reasoning...

What the bug is:
isNPMPackageNameIgnoreLength in src/string/immutable.zig uses the condition slash_index > 0 when validating scoped package names, but this condition accepts empty-scope specifiers like @/pkg. A valid scoped npm package requires at least one character between @ and / (e.g., @scope/pkg), so the condition should be slash_index > 1 (or equivalently >= 2).

The specific code path:
In isNPMPackageNameIgnoreLength, for input @/pkg: target[0] == '@' so scoped = true. The loop iterates over target[1..] = /pkg; at i=0, it encounters / and sets slash_index = i + 1 = 1. The final return is !scoped or slash_index > 0 and slash_index + 1 < target.len. Due to Zig's operator precedence (and binds tighter than or), this evaluates as (!true) or ((1 > 0) and (2 < 5)) = false or (true and true) = true. So @/pkg passes the validator despite having an empty scope name.

Why existing code doesn't prevent it:
The bug is structural: slash_index is set to i + 1 where i is 0-based within target[1..]. When the slash is immediately after @ (index 1 in the original string, index 0 in the slice), slash_index becomes 1. The condition slash_index > 0 is satisfied by any scoped name, including those with empty scope. The correct threshold is slash_index > 1 (meaning the slash must be at least at position 2 in the original string, requiring one non-empty scope character).

Impact:
The new guard at resolver.zig:1907 was added specifically to prevent garbage specifiers from reaching enqueueDependencyToRoot → sleepUntil → EventLoop.tick() reentry. Specifiers matching @/anything bypass this guard and still reach the auto-install path. This means: (1) wasteful registry requests for @/anything packages that can never exist, and (2) potential exposure to the same reentrant crash the PR aims to block, via @/pkg-style specifiers.

How to fix it:
In src/string/immutable.zig, inside isNPMPackageNameIgnoreLength, change the final return condition from slash_index > 0 to slash_index > 1. This ensures the scope portion has at least one character.

Step-by-step proof with @/pkg (length 5):

  1. ESModule.Package.parseName("@/pkg") — first char is @, finds first slash at index 1, slash2 at index 3 (end of "pkg"), returns specifier[0..5] = "@/pkg".
  2. ESModule.Package.parse("@/pkg") — returns a non-null Package with name = "@/pkg" (no leading dot, backslash, or percent).
  3. At resolver.zig:1907 — esm_ != null (step 2) and strings.isNPMPackageName("@/pkg") is called.
  4. Inside isNPMPackageNameIgnoreLength("@/pkg"): scoped = true; loop over "/pkg" sets slash_index = 1; return !true or (1>0 and 2<5) = true.
  5. Gate passes → enqueueDependencyToRoot called → PackageManager.sleepUntil → reentrant EventLoop.tick() → potential SIGSEGV.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch — isNPMPackageName("@/pkg") does return true because slash_index is 1 when / immediately follows @, and the final return only checks > 0.

That said, it's a pre-existing limitation of the shared validator (used by bun add, bun link, bun pm view, pnpm import, the bundler, and now here), not something this PR introduces — @/pkg already reached enqueueDependencyToRoot before this change. The Fuzzilli crash this PR targets (function f2() {\n...}) is blocked, and the gate is a strict improvement over no validation.

Tightening the final return to slash_index > 1 is correct but touches all seven call sites and deserves its own unit coverage for isNPMPackageName — I'd rather land that as a separate one-liner than widen this crash fix.

const esm = esm_.?.withAutoVersion();
load_module_from_cache: {
// If the source directory doesn't have a node_modules directory, we can
Expand Down
94 changes: 94 additions & 0 deletions test/js/bun/resolve/resolve-autoinstall-invalid-name.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, tempDir } from "harness";

// When auto-install is enabled, the resolver must not attempt to install a
// package whose name is not a valid npm package name. Attempting to do so
// reentrantly ticks the JS event loop from inside the resolver, which has led
// to crashes when reached via mock.module() / vi.mock() / Bun.resolveSync()
// with arbitrary user-provided strings.
test("resolver does not attempt auto-install for invalid npm package names", async () => {
let requests = 0;
await using server = Bun.serve({
port: 0,
fetch() {
requests++;
return new Response("{}", { status: 404, headers: { "content-type": "application/json" } });
},
});

using dir = tempDir("resolve-autoinstall-invalid-name", {
"index.js": `
const specifiers = [
"function f2() {\\n const v6 = new ArrayBuffer();\\n}",
"has spaces",
"(parens)",
"{braces}",
"line1\\nline2",
"foo\\tbar",
"a\\u0000b",
];
for (const s of specifiers) {
try {
Bun.resolveSync(s, import.meta.dir);
} catch {}
}
console.log("ok");
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "--install=force", "index.js"],
env: {
...bunEnv,
BUN_CONFIG_REGISTRY: server.url.href,
NPM_CONFIG_REGISTRY: server.url.href,
},
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout).toContain("ok");
expect(requests).toBe(0);
expect(exitCode).toBe(0);
});

test("resolver still attempts auto-install for valid npm package names", async () => {
let requests = 0;
await using server = Bun.serve({
port: 0,
fetch() {
requests++;
return new Response("{}", { status: 404, headers: { "content-type": "application/json" } });
},
});

using dir = tempDir("resolve-autoinstall-valid-name", {
"index.js": `
try {
Bun.resolveSync("some-package-that-does-not-exist-abc123", import.meta.dir);
} catch {}
console.log("ok");
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "--install=force", "index.js"],
env: {
...bunEnv,
BUN_CONFIG_REGISTRY: server.url.href,
NPM_CONFIG_REGISTRY: server.url.href,
},
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout).toContain("ok");
expect(requests).toBeGreaterThan(0);
expect(exitCode).toBe(0);
});
20 changes: 20 additions & 0 deletions test/js/bun/test/mock/mock-module-non-string.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,23 @@ test("mock.module still works with valid string argument", async () => {
const m = await import("mock-module-non-string-test-fixture");
expect(m.default).toBe(42);
});

test("mock.module does not crash on specifiers that are not valid npm package names", () => {
const specifiers = [
"function f2() {\n const v6 = new ArrayBuffer();\n v6.transferToFixedLength();\n}",
"foo\nbar",
"foo\rbar",
"has spaces",
"(parens)",
"{braces}",
"[brackets]",
];
for (const specifier of specifiers) {
expect(() => mock.module(specifier, () => ({ default: 1 }))).not.toThrow();
}
for (const specifier of specifiers) {
// @ts-expect-error missing callback on purpose
expect(() => mock.module(specifier)).toThrow("mock(module, fn) requires a function");
}
Bun.gc(true);
});
Loading