Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 3 additions & 1 deletion src/install/PackageManager/install_with_manager.zig
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,9 @@ pub fn installWithManager(
error.SecurityScannerInWorkspace => {
Output.pretty("<red>Security scanner cannot be a dependency of a workspace package. It must be a direct dependency of the root package.<r>\n", .{});
},
else => {},
else => {
Output.errGeneric("Security scanner failed: {s}", .{@errorName(err)});
},
}

Global.exit(1);
Expand Down
2 changes: 1 addition & 1 deletion src/install/PackageManager/scanner-entry.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import fs from "node:fs";

const scannerModuleName = "__SCANNER_MODULE__";
const packages = __PACKAGES_JSON__;
const packages = JSON.parse(fs.readFileSync(0, "utf-8"));
Copy link
Contributor

Choose a reason for hiding this comment

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

🟑 Nit: scanner-entry-globals.d.ts still declares __PACKAGES_JSON__ on line 1, but this PR removed all usage of that global from scanner-entry.ts (replaced with fs.readFileSync(0)) and removed the placeholder substitution from security_scanner.zig. The stale type declaration should be removed to keep the .d.ts file consistent with the actual code.

Extended reasoning...

What the bug is

The file src/install/PackageManager/scanner-entry-globals.d.ts line 1 declares:

declare const __PACKAGES_JSON__: Bun.Security.Package[];

This global type declaration was used to provide type information for the __PACKAGES_JSON__ placeholder that was previously embedded in scanner-entry.ts. The PR replaced this mechanism with reading package data from stdin via fs.readFileSync(0, "utf-8"), making the declaration stale.

The specific code path

Previously, security_scanner.zig had code that found the __PACKAGES_JSON__ placeholder in the embedded scanner-entry.ts source and replaced it with the actual JSON data at runtime. The .d.ts file existed so TypeScript tooling could type-check scanner-entry.ts during development, providing the type for the injected global.

This PR removed: (1) the __PACKAGES_JSON__ usage from scanner-entry.ts line 4 (now JSON.parse(fs.readFileSync(0, "utf-8"))), and (2) the placeholder replacement block from security_scanner.zig. However, scanner-entry-globals.d.ts was not updated.

Why existing code does not prevent it

The .d.ts file is only used for development-time type checking and is not included in the build (the .ts source is embedded via @embedFile). So there is no build error or runtime failure from the stale declaration β€” it simply goes unnoticed.

Impact

This is a minor consistency issue. Future developers reading scanner-entry-globals.d.ts would see __PACKAGES_JSON__ declared as a global and might be confused about where it comes from, since it is no longer used anywhere in the codebase. It could also lead to someone mistakenly thinking the global injection mechanism still exists.

How to fix

Remove line 1 from scanner-entry-globals.d.ts:

-declare const __PACKAGES_JSON__: Bun.Security.Package[];
 declare const __SUPPRESS_ERROR__: boolean;

Step-by-step proof

  1. scanner-entry.ts line 4 now reads: const packages = JSON.parse(fs.readFileSync(0, "utf-8")); β€” no reference to __PACKAGES_JSON__.
  2. The security_scanner.zig diff shows the __PACKAGES_JSON__ placeholder replacement block (lines 676-685 in the old file) was deleted entirely.
  3. scanner-entry-globals.d.ts line 1 still declares declare const __PACKAGES_JSON__: Bun.Security.Package[];.
  4. Searching the codebase confirms no remaining references to __PACKAGES_JSON__ other than this stale declaration.

const suppressError = __SUPPRESS_ERROR__;

type IPCMessage =
Expand Down
45 changes: 33 additions & 12 deletions src/install/PackageManager/security_scanner.zig
Original file line number Diff line number Diff line change
Expand Up @@ -673,17 +673,6 @@ fn attemptSecurityScanWithRetry(manager: *PackageManager, security_scanner: []co
temp_source = code.items;
}

const packages_placeholder = "__PACKAGES_JSON__";
if (std.mem.indexOf(u8, temp_source, packages_placeholder)) |index| {
var new_code = std.array_list.Managed(u8).init(manager.allocator);
try new_code.appendSlice(temp_source[0..index]);
try new_code.appendSlice(json_data);
try new_code.appendSlice(temp_source[index + packages_placeholder.len ..]);
code.deinit();
code = new_code;
temp_source = code.items;
}

const suppress_placeholder = "__SUPPRESS_ERROR__";
if (std.mem.indexOf(u8, temp_source, suppress_placeholder)) |index| {
var new_code = std.array_list.Managed(u8).init(manager.allocator);
Expand Down Expand Up @@ -724,6 +713,19 @@ fn attemptSecurityScanWithRetry(manager: *PackageManager, security_scanner: []co
return try scanner.handleResults(&collector.package_paths, start_time, packages_scanned, security_scanner, security_scanner_pkg_id, command_ctx, original_cwd, is_retry);
}

fn writeAllToPipe(fd: bun.FileDescriptor, data: []const u8) void {
var remaining = data;
while (remaining.len > 0) {
switch (bun.sys.write(fd, remaining)) {
.result => |written| {
if (written == 0) return;
remaining = remaining[written..];
},
.err => return,
}
}
}
Comment on lines +716 to +727
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟑 Minor

writeAllToPipe silently ignores write errors.

The function returns without indication when bun.sys.write fails. This could mask issues during large data transfer (e.g., broken pipe, disk quota). The caller (spawn) has no way to detect that the child received incomplete data.

Consider returning a boolean or error indicator so the caller can handle failures appropriately.

πŸ”§ Proposed fix to return success/failure
-fn writeAllToPipe(fd: bun.FileDescriptor, data: []const u8) void {
+fn writeAllToPipe(fd: bun.FileDescriptor, data: []const u8) bool {
     var remaining = data;
     while (remaining.len > 0) {
         switch (bun.sys.write(fd, remaining)) {
             .result => |written| {
-                if (written == 0) return;
+                if (written == 0) return false;
                 remaining = remaining[written..];
             },
-            .err => return,
+            .err => return false,
         }
     }
+    return true;
 }

Then in spawn:

-        writeAllToPipe(stdin_pipe_fds[1], this.json_data);
+        if (!writeAllToPipe(stdin_pipe_fds[1], this.json_data)) {
+            stdin_pipe_fds[1].close();
+            return error.StdinWriteFailed;
+        }
πŸ€– Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/install/PackageManager/security_scanner.zig` around lines 716 - 727, The
writeAllToPipe function currently swallows errors from bun.sys.write and returns
early on write errors or zero bytes written, so modify writeAllToPipe to return
an error or bool (e.g., !void or bool) instead of void, propagate any .err from
bun.sys.write and treat written == 0 as a failure, and update the caller spawn
to check the returned success/error and handle/propagate the failure (cleanup,
close pipe, and surface an error) so callers can detect incomplete writes;
reference the writeAllToPipe function and the spawn caller when making these
changes.


pub const SecurityScanSubprocess = struct {
manager: *PackageManager,
code: []const u8,
Expand Down Expand Up @@ -752,6 +754,17 @@ pub const SecurityScanSubprocess = struct {
.result => |fds| fds,
};

// Create a stdin pipe to pass package JSON data to the child process.
// This avoids embedding the (potentially large) JSON in the -e argument,
// which can exceed the OS per-argument size limit (MAX_ARG_STRLEN = 128KB on Linux).
const stdin_pipe_result = bun.sys.pipe();
const stdin_pipe_fds = switch (stdin_pipe_result) {
.err => {
return error.StdinPipeFailed;
},
.result => |fds| fds,
Comment on lines +760 to +765
Copy link
Contributor

Choose a reason for hiding this comment

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

🟑 Two minor error-handling issues in the new stdin pipe code: (1) If pipe() for stdin fails at line 760, the already-created IPC pipe_fds are leaked since there's no errdefer to close them. Similarly, if spawnProcess fails, both pipe pairs (4 FDs) leak. (2) writeAllToPipe silently returns void on write errors, so a broken pipe produces a confusing downstream JSON parse error instead of a clear pipe write failure message. Both are mitigated by the process exiting shortly after, but adding cleanup and at least a debug log would improve robustness.

Extended reasoning...

FD leak on stdin pipe creation failure

In SecurityScanSubprocess.spawn(), the IPC pipe is created at lines 749-755 via bun.sys.pipe(). If the second pipe() call for stdin at line 760 fails, the function returns error.StdinPipeFailed without closing pipe_fds[0] and pipe_fds[1]. There are no errdefer statements anywhere in the function to handle this cleanup. The same pattern applies if spawnProcess fails at line 795 β€” all four file descriptors (both pipe_fds and stdin_pipe_fds) would leak.

The idiomatic Zig fix would be to add errdefer statements after each successful pipe() call:

const pipe_fds = switch (pipe_result) { ... };
errdefer { pipe_fds[0].close(); pipe_fds[1].close(); }

And similarly for stdin_pipe_fds.

Concrete example of the FD leak

Step-by-step: (1) bun.sys.pipe() succeeds for IPC, creating pipe_fds[0] and pipe_fds[1]. (2) bun.sys.pipe() for stdin fails (e.g., system is near the FD limit). (3) The function returns error.StdinPipeFailed. (4) pipe_fds[0] and pipe_fds[1] are never closed β€” they are leaked until the process exits. In practice, the caller catches the error and calls Global.exit(1), so the OS reclaims the FDs almost immediately, but it's still not correct resource management.

Silent write errors in writeAllToPipe

The writeAllToPipe function (line 713-727) returns void and silently discards write errors. If the child process dies before reading all stdin data (e.g., crashes during startup), write() will return EPIPE or similar, and the function silently returns. The child then receives truncated JSON, fails to parse it, and reports a generic SCAN_FAILED error via IPC (or exits with a non-zero code). The parent sees "Security scanner sent invalid JSON" or "Security scanner failed" rather than "pipe write to child stdin failed". This is ironic given that the PR description notes the old code "silently swallowed" errors.

Impact assessment

Both issues are low-impact in practice: (1) pipe() failing is extremely rare and only happens at the OS FD limit, and the process exits immediately after. (2) Write failures are still detected indirectly through the child's exit status or IPC error, so the system doesn't silently succeed β€” it just produces a less helpful error message. The fix for both is straightforward: add errdefer for FD cleanup, and have writeAllToPipe return an error (or at least emit a debug log) so the root cause is visible.

Pre-existing pattern note

The IPC pipe already had a similar leak pattern before this PR (if spawnProcess failed, pipe_fds would leak). However, this PR introduces a new leak path: stdin pipe failure after IPC pipe success. The writeAllToPipe issue is entirely new to this PR.

};

const exec_path = try bun.selfExePath();

var argv = [_]?[*:0]const u8{
Expand All @@ -771,7 +784,7 @@ pub const SecurityScanSubprocess = struct {
const spawn_options = bun.spawn.SpawnOptions{
.stdout = .inherit,
.stderr = .inherit,
.stdin = .inherit,
.stdin = .{ .pipe = stdin_pipe_fds[0] },
.cwd = spawn_cwd,
.extra_fds = &.{.{ .pipe = pipe_fds[1] }},
.windows = if (Environment.isWindows) .{
Expand All @@ -782,6 +795,14 @@ pub const SecurityScanSubprocess = struct {
var spawned = try (try bun.spawn.spawnProcess(&spawn_options, @ptrCast(&argv), @ptrCast(std.os.environ.ptr))).unwrap();

pipe_fds[1].close();
stdin_pipe_fds[0].close();

// Write JSON data to the child's stdin pipe then close it to signal EOF.
// The child process reads this data synchronously via fs.readFileSync(0).
// The write may block briefly if data exceeds the pipe buffer, but the child
// is already running and will drain it.
writeAllToPipe(stdin_pipe_fds[1], this.json_data);
stdin_pipe_fds[1].close();

if (comptime bun.Environment.isPosix) {
_ = bun.sys.setNonblocking(pipe_fds[0]);
Expand Down
95 changes: 95 additions & 0 deletions test/regression/issue/27716.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import { afterAll, beforeAll, expect, test } from "bun:test";
import { existsSync, readFileSync } from "fs";
import { bunEnv, bunExe, tempDir } from "harness";
import { join } from "path";

// Regression test for https://github.com/oven-sh/bun/issues/27716
// bun install silently fails when a security scanner is configured and
// the project has enough packages that the inline JSON would exceed the
// OS per-argument size limit (MAX_ARG_STRLEN = 128KB on Linux).

const PACKAGE_COUNT = 850;
const tgzPath = join(import.meta.dir, "..", "..", "cli", "install", "bar-0.0.2.tgz");
const tgzData = readFileSync(tgzPath);

let server: ReturnType<typeof Bun.serve>;
let registryUrl: string;

beforeAll(() => {
server = Bun.serve({
port: 0,
async fetch(req) {
const url = new URL(req.url);
const path = url.pathname;

if (path.endsWith(".tgz")) {
return new Response(tgzData);
}

// Package metadata request
const name = decodeURIComponent(path.slice(1));
return new Response(
JSON.stringify({
name,
versions: {
"1.0.0": {
name,
version: "1.0.0",
dist: {
tarball: `${registryUrl}${name}-1.0.0.tgz`,
},
},
},
"dist-tags": { latest: "1.0.0" },
}),
);
},
});
registryUrl = `http://localhost:${server.port}/`;
});

afterAll(() => {
server?.stop(true);
});
Comment on lines +15 to +53
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | πŸ”΅ Trivial

Consider using using for server cleanup to ensure proper resource disposal.

The coding guidelines recommend using using or await using for APIs like Bun.serve to ensure proper resource cleanup. The current afterAll approach may miss cleanup if a test fails before reaching afterAll.

♻️ Proposed refactor using `using` pattern
-let server: ReturnType<typeof Bun.serve>;
-let registryUrl: string;
-
-beforeAll(() => {
-  server = Bun.serve({
+test("security scanner works with many packages", async () => {
+  using server = Bun.serve({
     port: 0,
     async fetch(req) {
       // ... existing fetch logic
     },
   });
-  registryUrl = `http://localhost:${server.port}/`;
-});
-
-afterAll(() => {
-  server?.stop(true);
-});
+  const registryUrl = `http://localhost:${server.port}/`;
 
-test("security scanner works with many packages", async () => {
   using dir = tempDir("issue-27716", {
     // ... rest of test
   });
+  // ... rest of test using registryUrl
+}, 60_000);
πŸ€– Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/regression/issue/27716.test.ts` around lines 15 - 53, Replace manual
beforeAll/afterAll cleanup with a using-based resource scope for Bun.serve:
instead of storing server in the top-level server variable and calling
server?.stop(true) in afterAll, acquire the server via an await using (or using)
so the Bun.serve return value is disposed automatically; update the setup so
registryUrl still reads server.port after creation and remove the afterAll
teardown. Reference the Bun.serve call and the server variable to locate where
to switch to using/await using.


test("security scanner works with many packages", async () => {
using dir = tempDir("issue-27716", {
"scanner.ts": `export const scanner = {
version: "1",
scan: async ({ packages }) => {
return [];
},
};`,
});

// Generate many dependencies
const deps: Record<string, string> = {};
for (let i = 0; i < PACKAGE_COUNT; i++) {
deps[`pkg-with-a-longer-name-for-testing-${String(i).padStart(4, "0")}`] = "1.0.0";
}

await Bun.write(
join(String(dir), "package.json"),
JSON.stringify({ name: "test-27716", version: "1.0.0", dependencies: deps }),
);

await Bun.write(
join(String(dir), "bunfig.toml"),
`[install]\nregistry = "${registryUrl}"\n\n[install.security]\nscanner = "./scanner.ts"\n`,
);

await using proc = Bun.spawn({
cmd: [bunExe(), "install"],
cwd: String(dir),
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

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

expect(stderr).not.toContain("Security scanner failed");
expect(existsSync(join(String(dir), "bun.lock"))).toBe(true);
expect(existsSync(join(String(dir), "node_modules"))).toBe(true);
expect(exitCode).toBe(0);
}, 60_000);
Copy link
Contributor

Choose a reason for hiding this comment

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

🟑 The test sets an explicit 60,000ms timeout at line 95 (}, 60_000);), which violates the test/CLAUDE.md rule: "CRITICAL: Do not set a timeout on tests. Bun already has timeouts." The timeout should be removed.

Extended reasoning...

What the bug is

The newly introduced regression test at test/regression/issue/27716.test.ts line 95 passes a second argument of 60_000 to the test() call, setting an explicit 60-second timeout on the test case.

The rule it violates

The test/CLAUDE.md file explicitly states under its testing guidelines: "CRITICAL: Do not set a timeout on tests. Bun already has timeouts." This is a clear, unambiguous convention for the repository. The project relies on Bun's built-in test timeout mechanism rather than per-test explicit timeouts.

Step-by-step proof

Looking at the test file, line 95 reads:

}, 60_000);

This is the closing of the test("security scanner works with many packages", async () => { ... }, 60_000) call. The 60_000 is the explicit timeout parameter passed to bun:test's test() function, overriding whatever default timeout Bun provides.

Impact

This is a style/convention violation rather than a functional bug. The test will still work correctly with or without the explicit timeout. However, it sets a precedent that could lead to inconsistent timeout practices across the test suite, which is exactly what the CLAUDE.md rule is designed to prevent.

How to fix

Simply remove the , 60_000 argument from line 95, changing:

}, 60_000);

to:

});

Bun's built-in test timeout will handle the case where the test runs too long.