mirror of
https://github.com/oven-sh/bun
synced 2026-02-10 10:58:56 +00:00
fix(Terminal): add proper error handling for PTY session setup
After macOS sleep/wake or extended runtime (~17 hours), PTY allocation can become corrupted - openpty() may return FDs where the slave isn't properly linked to the master. This caused spawned shells to immediately exit with code 0. Changes: - Check setsid() return value and fail spawn if it returns -1 - Check ioctl(TIOCSCTTY) return value and fail spawn if it returns -1 - Add isatty() validation after openpty() to detect corrupted PTY pairs Fixes #25912 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -425,6 +425,20 @@ fn createPtyPosix(cols: u16, rows: u16) CreatePtyError!PtyResult {
|
||||
const master_fd_desc = bun.FD.fromNative(master_fd);
|
||||
const slave_fd_desc = bun.FD.fromNative(slave_fd);
|
||||
|
||||
// Validate PTY pair: ensure slave FD is actually a TTY.
|
||||
// After macOS sleep/wake or extended runtime, PTY allocation can become
|
||||
// corrupted - openpty() may return FDs where the slave isn't properly
|
||||
// linked to the master. Detect this early rather than having the child
|
||||
// process fail mysteriously.
|
||||
if (!std.posix.isatty(slave_fd)) {
|
||||
if (comptime bun.Environment.allow_assert) {
|
||||
bun.sys.syslog("PTY validation failed: slave_fd={d} is not a TTY", .{slave_fd});
|
||||
}
|
||||
master_fd_desc.close();
|
||||
slave_fd_desc.close();
|
||||
return error.OpenPtyFailed;
|
||||
}
|
||||
|
||||
// Configure sensible terminal defaults matching node-pty behavior.
|
||||
// These are "cooked mode" defaults that most terminal applications expect.
|
||||
if (std.posix.tcgetattr(slave_fd)) |termios| {
|
||||
|
||||
@@ -190,17 +190,38 @@ extern "C" ssize_t posix_spawn_bun(
|
||||
sigaction(i, &sa, 0);
|
||||
}
|
||||
|
||||
// Make "detached" work, or set up PTY as controlling terminal
|
||||
// Make "detached" work, or set up PTY as controlling terminal.
|
||||
// setsid() creates a new session and makes this process the session leader.
|
||||
// This is required for TIOCSCTTY to work - only a session leader can acquire
|
||||
// a controlling terminal.
|
||||
if (request->detached || request->pty_slave_fd >= 0) {
|
||||
setsid();
|
||||
if (setsid() == -1) {
|
||||
// For PTY spawns, setsid() failure is fatal since we need a new session
|
||||
// to set the controlling terminal. For detached processes, it's also
|
||||
// fatal since the whole point is to create a new session.
|
||||
return childFailed();
|
||||
}
|
||||
}
|
||||
|
||||
// Set PTY slave as controlling terminal for proper job control.
|
||||
// TIOCSCTTY may fail if the terminal is already the controlling terminal
|
||||
// of another session. This is non-fatal - the process can still run,
|
||||
// just without proper job control.
|
||||
// TIOCSCTTY requires the calling process to be a session leader (which we
|
||||
// established above with setsid()) and the terminal must not already be
|
||||
// the controlling terminal of another session.
|
||||
// After macOS sleep/wake or extended runtime, PTY allocation can become
|
||||
// corrupted - openpty() may return FDs where the slave isn't properly
|
||||
// linked to the master. In these cases, TIOCSCTTY will fail, and without
|
||||
// a controlling terminal the shell will immediately exit.
|
||||
if (request->pty_slave_fd >= 0) {
|
||||
(void)ioctl(request->pty_slave_fd, TIOCSCTTY, 0);
|
||||
if (ioctl(request->pty_slave_fd, TIOCSCTTY, 0) == -1) {
|
||||
// TIOCSCTTY failure means we can't establish proper terminal control.
|
||||
// This typically happens when:
|
||||
// 1. The PTY slave is not valid (corrupted after sleep/wake)
|
||||
// 2. The terminal is already a controlling terminal of another session
|
||||
// 3. We're not a session leader (shouldn't happen given setsid above)
|
||||
// Without a controlling terminal, shells will exit immediately,
|
||||
// so we treat this as a fatal error for PTY spawns.
|
||||
return childFailed();
|
||||
}
|
||||
}
|
||||
|
||||
int current_max_fd = 0;
|
||||
|
||||
204
test/regression/issue/25912.test.ts
Normal file
204
test/regression/issue/25912.test.ts
Normal file
@@ -0,0 +1,204 @@
|
||||
import { describe, expect, test } from "bun:test";
|
||||
import { bunEnv, bunExe, isWindows } from "harness";
|
||||
|
||||
// Issue #25912: PTY allocation fails after extended runtime (~17 hours) or system
|
||||
// sleep/wake cycles on macOS. New Terminal instances appear to create successfully,
|
||||
// but spawned shells immediately exit with code 0.
|
||||
//
|
||||
// The fix adds proper error handling for:
|
||||
// 1. setsid() - now fails the spawn if it returns -1 for PTY spawns
|
||||
// 2. ioctl(TIOCSCTTY) - now fails the spawn if it returns -1 for PTY spawns
|
||||
// 3. PTY validation - verifies slave FD is actually a TTY after openpty()
|
||||
//
|
||||
// Since we can't reliably reproduce the sleep/wake corruption in a test, we verify:
|
||||
// - The PTY slave FD is correctly identified as a TTY (via isatty check in spawn)
|
||||
// - Proper session setup happens (setsid) before TIOCSCTTY
|
||||
// - The spawned shell gets a proper controlling terminal
|
||||
|
||||
describe.todoIf(isWindows)("Issue #25912 - PTY session and controlling terminal setup", () => {
|
||||
test("spawned shell has controlling terminal and proper session", async () => {
|
||||
// This test verifies the shell gets a proper controlling terminal.
|
||||
// Before the fix, setsid() and TIOCSCTTY failures were silently ignored,
|
||||
// causing shells to exit immediately with code 0.
|
||||
const dataChunks: Uint8Array[] = [];
|
||||
|
||||
const proc = Bun.spawn(
|
||||
[
|
||||
bunExe(),
|
||||
"-e",
|
||||
`
|
||||
const tty = require("tty");
|
||||
const fs = require("fs");
|
||||
|
||||
// Check that we have a controlling terminal
|
||||
const hasTTY = tty.isatty(0) && tty.isatty(1) && tty.isatty(2);
|
||||
|
||||
// Check that our session ID matches (we should be session leader)
|
||||
// getsid(0) returns the session ID of the calling process
|
||||
const pid = process.pid;
|
||||
|
||||
// Try to get ttyname - this only works if TIOCSCTTY succeeded
|
||||
let ttyName = "unknown";
|
||||
try {
|
||||
// On Unix, /dev/tty is the controlling terminal
|
||||
// It will error if there's no controlling terminal
|
||||
fs.statSync("/dev/tty");
|
||||
ttyName = "has_ctty";
|
||||
} catch (e) {
|
||||
ttyName = "no_ctty";
|
||||
}
|
||||
|
||||
console.log(JSON.stringify({
|
||||
hasTTY,
|
||||
ttyName,
|
||||
pid
|
||||
}));
|
||||
`,
|
||||
],
|
||||
{
|
||||
env: bunEnv,
|
||||
terminal: {
|
||||
cols: 80,
|
||||
rows: 24,
|
||||
data: (_terminal: Bun.Terminal, data: Uint8Array) => {
|
||||
dataChunks.push(data);
|
||||
},
|
||||
},
|
||||
},
|
||||
);
|
||||
|
||||
await proc.exited;
|
||||
|
||||
// The shell should exit successfully (exit code 0)
|
||||
// Before the fix, it might exit with code 0 but have no controlling terminal
|
||||
expect(proc.exitCode).toBe(0);
|
||||
|
||||
const combinedOutput = Buffer.concat(dataChunks).toString();
|
||||
|
||||
// Parse the JSON output - find the JSON object in the output
|
||||
const jsonMatch = combinedOutput.match(/\{[^}]+\}/);
|
||||
expect(jsonMatch).not.toBeNull();
|
||||
|
||||
const result = JSON.parse(jsonMatch![0]);
|
||||
|
||||
// Verify we have a TTY
|
||||
expect(result.hasTTY).toBe(true);
|
||||
|
||||
// Verify we have a controlling terminal
|
||||
// If TIOCSCTTY failed, /dev/tty would not exist for this process
|
||||
expect(result.ttyName).toBe("has_ctty");
|
||||
|
||||
proc.terminal!.close();
|
||||
});
|
||||
|
||||
test("multiple sequential terminal spawns all get controlling terminals", async () => {
|
||||
// This test simulates the scenario from the bug report where multiple
|
||||
// terminal sessions are created over time. After sleep/wake, new sessions
|
||||
// would fail while old ones continued working.
|
||||
|
||||
for (let i = 0; i < 3; i++) {
|
||||
const dataChunks: Uint8Array[] = [];
|
||||
|
||||
const proc = Bun.spawn(
|
||||
[
|
||||
bunExe(),
|
||||
"-e",
|
||||
`
|
||||
const fs = require("fs");
|
||||
try {
|
||||
fs.statSync("/dev/tty");
|
||||
console.log("ctty:ok");
|
||||
} catch (e) {
|
||||
console.log("ctty:failed");
|
||||
}
|
||||
`,
|
||||
],
|
||||
{
|
||||
env: bunEnv,
|
||||
terminal: {
|
||||
data: (_terminal: Bun.Terminal, data: Uint8Array) => {
|
||||
dataChunks.push(data);
|
||||
},
|
||||
},
|
||||
},
|
||||
);
|
||||
|
||||
await proc.exited;
|
||||
|
||||
expect(proc.exitCode).toBe(0);
|
||||
|
||||
const output = Buffer.concat(dataChunks).toString();
|
||||
expect(output).toContain("ctty:ok");
|
||||
|
||||
proc.terminal!.close();
|
||||
}
|
||||
});
|
||||
|
||||
test("terminal spawn fails gracefully if PTY is invalid", async () => {
|
||||
// This test verifies that the PTY validation (isatty check) catches
|
||||
// corrupted PTYs early. We can't directly test corrupted PTYs, but
|
||||
// we can verify that a closed terminal properly throws an error.
|
||||
|
||||
const terminal = new Bun.Terminal({});
|
||||
terminal.close();
|
||||
|
||||
expect(() => {
|
||||
Bun.spawn(["echo", "test"], { terminal });
|
||||
}).toThrow("terminal is closed");
|
||||
});
|
||||
|
||||
test("shell process is properly set as session leader", async () => {
|
||||
// Verify setsid() is being called and succeeding.
|
||||
// The spawned process should be a session leader (its PID should equal its SID).
|
||||
const dataChunks: Uint8Array[] = [];
|
||||
|
||||
const proc = Bun.spawn(
|
||||
[
|
||||
bunExe(),
|
||||
"-e",
|
||||
`
|
||||
// In Node.js/Bun, we can use child_process to get session ID
|
||||
const { execSync } = require("child_process");
|
||||
const pid = process.pid;
|
||||
|
||||
// Use ps to get the session ID (SID) of our process
|
||||
// ps -o pid=,sid= -p <pid> shows PID and SID
|
||||
try {
|
||||
const result = execSync(\`ps -o pid=,sid= -p \${pid}\`, { encoding: "utf-8" });
|
||||
const [, sid] = result.trim().split(/\\s+/);
|
||||
|
||||
// As session leader, our PID should equal our SID
|
||||
console.log(JSON.stringify({ pid, sid: parseInt(sid), isSessionLeader: pid === parseInt(sid) }));
|
||||
} catch (e) {
|
||||
// ps command failed, just report we couldn't verify
|
||||
console.log(JSON.stringify({ pid, error: e.message }));
|
||||
}
|
||||
`,
|
||||
],
|
||||
{
|
||||
env: bunEnv,
|
||||
terminal: {
|
||||
data: (_terminal: Bun.Terminal, data: Uint8Array) => {
|
||||
dataChunks.push(data);
|
||||
},
|
||||
},
|
||||
},
|
||||
);
|
||||
|
||||
await proc.exited;
|
||||
expect(proc.exitCode).toBe(0);
|
||||
|
||||
const output = Buffer.concat(dataChunks).toString();
|
||||
const jsonMatch = output.match(/\{[^}]+\}/);
|
||||
expect(jsonMatch).not.toBeNull();
|
||||
|
||||
const result = JSON.parse(jsonMatch![0]);
|
||||
|
||||
// If we got the session info, verify we're session leader
|
||||
if (!result.error) {
|
||||
expect(result.isSessionLeader).toBe(true);
|
||||
}
|
||||
|
||||
proc.terminal!.close();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user