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
18 changes: 14 additions & 4 deletions src/bun.js/bindings/c-bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -473,19 +473,25 @@ extern "C" void bun_initialize_process()
} while (devNullFd_ < 0 and errno == EINTR);
};

if (devNullFd_ < 0) {
// open("/dev/null") failed (e.g., in macOS App Sandbox).
// Continue without redirecting; this is best-effort.
return;
}
Comment on lines +476 to +480

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.

馃煛 Minor: When open("/dev/null") fails, setDevNullFd returns early without resetting bun_is_stdio_null[target_fd] back to 0, leaving stale state. The dup2 failure path at line 494 correctly resets the flag, but this early-return path does not. While both code paths currently lead to errors downstream, the inconsistency could mislead future code that checks isStdoutNull/isStderrNull/isStdinNull.

Why this is a problem

Bug Description

The setDevNullFd lambda in c-bindings.cpp (line 468) unconditionally sets bun_is_stdio_null[target_fd] = 1 at line 469 before attempting to open /dev/null. If the open("/dev/null") call fails (line 476 check, devNullFd_ < 0), the function returns early at line 479 without resetting the flag back to 0. This leaves bun_is_stdio_null[target_fd] in a semantically incorrect state: it claims the fd was redirected to /dev/null when in fact the fd is still invalid (EBADF).

Code Path Analysis

The specific trigger is the macOS App Sandbox scenario that this PR was designed to address. When isatty(fd) returns 0 and errno is EBADF for any of the stdio fds (0, 1, 2), setDevNullFd(fd) is called. Inside the lambda, bun_is_stdio_null[target_fd] is set to 1 optimistically. The code then attempts open("/dev/null", O_RDWR | O_CLOEXEC, 0). In an App Sandbox, this open can fail, causing the early return at line 479.

Notably, the dup2 failure path at line 493-495 does properly reset bun_is_stdio_null[target_fd] = 0, creating an asymmetry between the two error paths within the same function. This asymmetry strongly suggests the missing reset on the open failure path was an oversight rather than an intentional design choice.

Step-by-Step Proof

  1. Bun starts inside a macOS App Sandbox.
  2. The initialization loop at line 498 iterates over fds 0, 1, 2.
  3. For some fd (say fd=1, stdout), isatty(1) returns 0 and errno == EBADF.
  4. setDevNullFd(1) is called.
  5. Line 469: bun_is_stdio_null[1] = 1 is set unconditionally.
  6. Line 470-474: Since devNullFd_ is -1, the code attempts open("/dev/null", O_RDWR | O_CLOEXEC, 0).
  7. The open fails because the App Sandbox restricts filesystem access. devNullFd_ remains negative.
  8. Line 476-480: The devNullFd_ < 0 check is true, so the function returns early.
  9. bun_is_stdio_null[1] remains 1, falsely indicating stdout was redirected to /dev/null.

Practical Impact

In the current codebase, both code paths (flag=1 and flag=0) lead to errors in the downstream shell interpreter consumers. When the flag is 1, bun.sys.openNullDevice() is called, which itself calls open("/dev/null") and also fails in the sandbox. When the flag is 0, ShellSyscall.dup(fd) is called on an EBADF fd, which also fails. Both errors are handled by the same .err branch.

However, the flag's name (bun_is_stdio_null) and its consumers (isStdoutNull, isStderrNull, isStdinNull) describe the current state of the fd, not the intended state. Setting it to 1 when the redirect did not actually occur misrepresents reality and could mislead future code.

Recommended Fix

Add bun_is_stdio_null[target_fd] = 0; before the return; on line 479, mirroring the existing reset on the dup2 failure path at line 494:

Suggested change
if (devNullFd_ < 0) {
// open("/dev/null") failed (e.g., in macOS App Sandbox).
// Continue without redirecting; this is best-effort.
return;
}
if (devNullFd_ < 0) {
// open("/dev/null") failed (e.g., in macOS App Sandbox).
// Continue without redirecting; this is best-effort.
bun_is_stdio_null[target_fd] = 0;
return;
}


if (devNullFd_ == target_fd) {
devNullFd_ = -1;
return;
}

ASSERT(devNullFd_ != -1);
int err;
do {
err = dup2(devNullFd_, target_fd);
} while (err < 0 && errno == EINTR);

if (err != 0) [[unlikely]] {
abort();
// dup2 returns the new fd on success (not 0), or -1 on error.
if (err < 0) [[unlikely]] {
bun_is_stdio_null[target_fd] = 0;
}
};

Expand All @@ -497,14 +503,18 @@ extern "C" void bun_initialize_process()
setDevNullFd(fd);
}
} else {
bun_stdio_tty[fd] = 1;
int err = 0;

do {
err = tcgetattr(fd, &termios_to_restore_later[fd]);
} while (err == -1 && errno == EINTR);

if (err == 0) [[likely]] {
// Only mark as TTY if we successfully captured termios state.
// In macOS App Sandbox, tcgetattr fails with EPERM even though
// isatty() returns true. We must not try to restore state we
// never captured.
bun_stdio_tty[fd] = 1;
anyTTYs = true;
}
}
Expand Down
122 changes: 122 additions & 0 deletions test/js/bun/test-macos-app-sandbox.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import { describe, expect, test } from "bun:test";

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.

馃煛 Minor: Nit: This PR closes #15661, so per CLAUDE.md convention this test should be placed at test/regression/issue/15661.test.ts rather than test/js/bun/. That said, this is arguably a feature test (modeled after Node.js's test/parallel/test-macos-app-sandbox.js), so the current placement in test/js/bun/ is also defensible if intentional.

Why this is a problem

Convention Violation: Test File Placement

The test file test/js/bun/test-macos-app-sandbox.test.ts is added as part of a PR that explicitly closes GitHub issue #15661. According to the CLAUDE.md convention under "Test Organization": "If a test is for a specific numbered GitHub Issue, it should be placed in test/regression/issue/${issueNumber}.test.ts. Ensure the issue number is REAL and not a placeholder!" The same rule is echoed in test/CLAUDE.md: "Regression tests for specific issues go in /test/regression/issue/${issueNumber}.test.ts."

Based on these guidelines, because the PR is tied to issue #15661, the test file should be located at test/regression/issue/15661.test.ts. Other tests for nearby issue numbers (e.g., test/regression/issue/15276.test.ts, test/regression/issue/15314.test.ts) follow this convention consistently.

Counterargument

One reviewer disagreed, arguing this is not purely a regression test for a single bug but rather a feature test for macOS App Sandbox support. The test file itself notes on line 75 that it is "Modeled after Node.js's test/parallel/test-macos-app-sandbox.js", and the test covers general sandbox functionality (executing JavaScript inside a sandbox, verifying the sandbox container path) rather than reproducing a narrow regression scenario. The test/CLAUDE.md also states: "Unit tests for specific features are organized by module (e.g., /test/js/bun/, /test/js/node/)", which supports the current placement.

This is a reasonable interpretation. The test does read more like a feature test than a minimal reproduction of a single bug. However, the CLAUDE.md rule about issue-linked tests is fairly explicit, and the commit message directly ties this work to issue #15661.

Recommendation

Since this is a nit-level convention issue, the simplest fix would be to move the file to test/regression/issue/15661.test.ts to align with the documented convention. Alternatively, if the author considers this a feature test that should live alongside other Bun-specific API tests, they could keep the current placement but should be aware it diverges from the stated guideline for issue-linked tests.

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.

Test comment

import { copyFileSync } from "fs";
import { bunEnv, bunExe, isMacOS, tempDir } from "harness";
import { join } from "path";

// Match Bun's own entitlements from entitlements.plist, plus app-sandbox.
const entitlementsPlist = `<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>com.apple.security.app-sandbox</key>
<true/>
<key>com.apple.security.cs.allow-jit</key>
<true/>
<key>com.apple.security.cs.allow-unsigned-executable-memory</key>
<true/>
<key>com.apple.security.cs.disable-executable-page-protection</key>
<true/>
<key>com.apple.security.cs.allow-dyld-environment-variables</key>
<true/>
<key>com.apple.security.cs.disable-library-validation</key>
<true/>
<key>com.apple.security.network.client</key>
<true/>
</dict>
</plist>`;

function makeInfoPlist(bundleId: string) {
return `<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>CFBundleExecutable</key>
<string>bun</string>
<key>CFBundleIdentifier</key>
<string>${bundleId}</string>
<key>CFBundleInfoDictionaryVersion</key>
<string>6.0</string>
<key>CFBundleName</key>
<string>bun_sandboxed</string>
<key>CFBundlePackageType</key>
<string>APPL</string>
<key>CFBundleShortVersionString</key>
<string>1.0</string>
<key>CFBundleSupportedPlatforms</key>
<array>
<string>MacOSX</string>
</array>
<key>CFBundleVersion</key>
<string>1</string>
</dict>
</plist>`;
}

function createSandboxedApp(prefix: string, bundleId: string) {
const dir = tempDir(prefix, {
"entitlements.plist": entitlementsPlist,
"bun_sandboxed.app": {
"Contents": {
"Info.plist": makeInfoPlist(bundleId),
"MacOS": {},
},
},
});

const bunPath = join(String(dir), "bun_sandboxed.app", "Contents", "MacOS", "bun");
const appBundlePath = join(String(dir), "bun_sandboxed.app");
const entitlementsPath = join(String(dir), "entitlements.plist");

copyFileSync(bunExe(), bunPath);

const codesignResult = Bun.spawnSync({
cmd: ["/usr/bin/codesign", "--entitlements", entitlementsPath, "--force", "-s", "-", appBundlePath],
env: bunEnv,
stderr: "inherit",
});
expect(codesignResult.exitCode).toBe(0);

return { dir, bunPath, bundleId };
}

// Modeled after Node.js's test/parallel/test-macos-app-sandbox.js
describe.skipIf(!isMacOS)("macOS App Sandbox", () => {
test("bun can execute JavaScript inside the app sandbox", async () => {
const { dir, bunPath } = createSandboxedApp("macos-sandbox-test", "dev.bun.test.sandbox_exec");
using _dir = dir;

await using proc = Bun.spawn({
cmd: [bunPath, "-e", "console.log('hello sandbox')"],
env: bunEnv,
stdout: "pipe",
stderr: "inherit",
});

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

expect(stdout.trim()).toBe("hello sandbox");
expect(exitCode).toBe(0);
});

test("sandboxed bun runs inside the sandbox container", async () => {
const { dir, bunPath, bundleId } = createSandboxedApp(
"macos-sandbox-test-container",
"dev.bun.test.sandbox_container",
);
using _dir = dir;

// When running inside a macOS App Sandbox, os.homedir() should return
// the sandbox container path, not the real home directory.
await using proc = Bun.spawn({
cmd: [bunPath, "-e", "console.log(require('os').homedir())"],
env: bunEnv,
stdout: "pipe",
stderr: "inherit",
});

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

expect(stdout.trim()).toContain(`Library/Containers/${bundleId}`);
expect(exitCode).toBe(0);
});
});