From d2847434c34e1072594ad702529d9e52dee4914d Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 13 Sep 2026 13:29:59 +0000 Subject: [PATCH] terminal: take the reader's ref before the reader starts When the poll registration fails, the POSIX PosixBufferedReader::start() calls on_reader_error and still returns Ok. Terminal's on_reader_error ends by releasing the reader's ref, but init_terminal took that ref only after start() returned. The release dropped the last ref, and the constructor then used the freed Terminal and handed it to JS. init_terminal now takes the reader's ref before start(). A reader that is already done after start(), or after the constructor's first read, makes the constructor throw "Failed to start terminal reader" and release the pty, the same as a writer that fails to start. --- src/runtime/api/bun/Terminal.rs | 31 ++++- .../bun/spawn/spawn-pipe-start-error.test.ts | 130 ++++++++++++------ 2 files changed, 113 insertions(+), 48 deletions(-) diff --git a/src/runtime/api/bun/Terminal.rs b/src/runtime/api/bun/Terminal.rs index 8276a3f4bd40..1d97dc961ae7 100644 --- a/src/runtime/api/bun/Terminal.rs +++ b/src/runtime/api/bun/Terminal.rs @@ -473,23 +473,25 @@ impl Terminal { } } - // Start reader with the read fd - adds a ref + // Start reader with the read fd. The reader's ref is taken first: when + // the poll registration fails, POSIX `start()` calls `on_reader_error`, + // which releases that ref, and still returns Ok. + terminal.ref_(); match terminal .reader .with_mut(|r| r.start(pty_result.read_fd, true)) { sys::Result::Err(_) => { - // Reader never started: closeInternal skips reader.close() but - // runs writer.close() → onWriterClose → deref (2→1). Then drop - // the initial ref (1→0). + // No callback ran: the reader took neither read_fd nor its ref. terminal.read_fd.get().close(); terminal.read_fd.set(Fd::INVALID); - terminal.close_internal(); terminal.deref_(); - return Err(InitError::ReaderStartFailed); + return Err(terminal.fail_reader_start()); + } + sys::Result::Ok(()) if terminal.flags.get().contains(Flags::READER_DONE) => { + return Err(terminal.fail_reader_start()); } sys::Result::Ok(()) => { - terminal.ref_(); #[cfg(unix)] { terminal.reader.with_mut(|r| { @@ -509,6 +511,10 @@ impl Terminal { // SAFETY: the reader cell is live for the terminal's lifetime; `read` // is the raw re-entrancy-safe entry (its dispatch runs user JS). unsafe { IOReader::read(terminal.reader.as_ptr()) }; + // The first read can end the reader too: EOF, a read error, or a failed re-arm. + if terminal.flags.get().contains(Flags::READER_DONE) { + return Err(terminal.fail_reader_start()); + } // Get or create the JS wrapper let this_value = existing_js_value.unwrap_or_else(|| js::to_js(parent_ptr, global_object)); @@ -539,6 +545,17 @@ impl Terminal { }) } + /// `init_terminal` error path for a reader that finished before the + /// terminal reached JS, with the reader's ref already released. + /// `close_internal` closes what is still open (a writer that is still + /// open releases its ref through `on_writer_close`), then the initial ref + /// is dropped, which may free `self`. + fn fail_reader_start(&self) -> InitError { + self.close_internal(); + self.deref_(); + InitError::ReaderStartFailed + } + /// Constructor for Terminal - called from JavaScript /// With constructNeedsThis: true, we receive the JSValue wrapper directly. /// Thunk emitted by `.classes.ts` codegen (`TerminalClass__construct` in diff --git a/test/js/bun/spawn/spawn-pipe-start-error.test.ts b/test/js/bun/spawn/spawn-pipe-start-error.test.ts index 7bec71df7ae0..2e981d41497e 100644 --- a/test/js/bun/spawn/spawn-pipe-start-error.test.ts +++ b/test/js/bun/spawn/spawn-pipe-start-error.test.ts @@ -81,6 +81,13 @@ try { // EPOLL_CTL_ADD asking for writability with ENOSPC (what an exhausted // fs.epoll.max_user_watches returns). Readable registrations, and uSockets, // which uses the wrapper, are unaffected. +// +// FAIL_EPOLL_CTL=pty-reader-add or pty-reader-mod fails one readable +// registration of a pty master instead, and nothing else: the EPOLL_CTL_ADD +// that Bun.Terminal's reader makes when it starts, or the EPOLL_CTL_MOD that +// re-arms it after the first read. The kernel fails the ADD when watches or +// memory run out. It does not fail the MOD that way: that mode only stands in +// for any error that ends the reader during the constructor's first read. const cc = Bun.which("cc") || Bun.which("gcc") || Bun.which("clang"); const SHIM_C = /* c */ ` @@ -88,19 +95,31 @@ const SHIM_C = /* c */ ` #include #include #include +#include +#include #include +#include #include static long (*real_syscall)(long, ...); +static int should_fail(long op, int fd, struct epoll_event *event) { + if (!event) return 0; + const char *mode = getenv("FAIL_EPOLL_CTL"); + if (!mode) return op == EPOLL_CTL_ADD && (event->events & EPOLLOUT); + long failing_op = strcmp(mode, "pty-reader-add") == 0 ? EPOLL_CTL_ADD : EPOLL_CTL_MOD; + unsigned int pty_number; + // TIOCGPTN succeeds on a pty master only. + return op == failing_op && (event->events & EPOLLIN) && ioctl(fd, TIOCGPTN, &pty_number) == 0; +} + long syscall(long number, ...) { va_list ap; va_start(ap, number); long a1 = va_arg(ap, long), a2 = va_arg(ap, long), a3 = va_arg(ap, long); long a4 = va_arg(ap, long), a5 = va_arg(ap, long), a6 = va_arg(ap, long); va_end(ap); - if (number == SYS_epoll_ctl && a2 == EPOLL_CTL_ADD && a4 != 0 && - (((struct epoll_event *)a4)->events & EPOLLOUT)) { + if (number == SYS_epoll_ctl && should_fail(a2, (int)a3, (struct epoll_event *)a4)) { errno = ENOSPC; return -1; } @@ -128,11 +147,14 @@ const wrappers = () => { // Parked on globalThis so the baseline keeps counting it: a local that is never // read again is not kept alive across the awaits below. -if (kind === "terminal") { - globalThis.anchor = Bun.Terminal.prototype; -} else { - globalThis.anchor = Bun.spawn({ cmd: ["true"], stdin: "ignore", stdout: "ignore", stderr: "ignore" }); - await globalThis.anchor.exited; +globalThis.anchor = []; +if (kind.includes("terminal")) { + globalThis.anchor.push(Bun.Terminal.prototype); +} +if (kind !== "terminal") { + const child = Bun.spawn({ cmd: ["true"], stdin: "ignore", stdout: "ignore", stderr: "ignore" }); + globalThis.anchor.push(child); + await child.exited; } const fdBaseline = openFds(); const wrapperBaseline = wrappers(); @@ -149,6 +171,9 @@ try { case "terminal": new Bun.Terminal({}); break; + case "spawn-terminal": + Bun.spawn({ cmd: ["true"], terminal: {} }); + break; } } catch (e) { error = { code: e.code, message: e.message }; @@ -161,43 +186,44 @@ while ((openFds() > fdBaseline || wrappers() > wrapperBaseline) && performance.n console.log(JSON.stringify({ error, leakedFds: openFds() - fdBaseline, leakedWrappers: wrappers() - wrapperBaseline })); `; -describe.skipIf(!isLinux || !cc)( - "a pipe writer whose event loop registration fails leaves its fd to the caller", - () => { - let dir: ReturnType; - - beforeAll(async () => { - dir = tempDir("writer-start-error", { "shim.c": SHIM_C, "fixture.js": FIXTURE }); - await using ccProc = Bun.spawn({ - cmd: [cc!, "-shared", "-fPIC", "-o", join(String(dir), "shim.so"), join(String(dir), "shim.c"), "-ldl"], - env: bunEnv, - stdout: "pipe", - stderr: "pipe", - }); - const [ccOut, ccErr, ccExit] = await Promise.all([ccProc.stdout.text(), ccProc.stderr.text(), ccProc.exited]); - if (ccExit !== 0) throw new Error(`shim compile failed: ${ccErr || ccOut}`); - }); +let dir: ReturnType | undefined; - afterAll(() => { - dir?.[Symbol.dispose](); - }); +beforeAll(async () => { + if (!isLinux || !cc) return; + dir = tempDir("poll-start-error", { "shim.c": SHIM_C, "fixture.js": FIXTURE }); + await using ccProc = Bun.spawn({ + cmd: [cc, "-shared", "-fPIC", "-o", join(String(dir), "shim.so"), join(String(dir), "shim.c"), "-ldl"], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [ccOut, ccErr, ccExit] = await Promise.all([ccProc.stdout.text(), ccProc.stderr.text(), ccProc.exited]); + if (ccExit !== 0) throw new Error(`shim compile failed: ${ccErr || ccOut}`); +}); - async function runFixture(kind: string, env: Record = {}) { - await using proc = Bun.spawn({ - cmd: [bunExe(), "fixture.js", kind], - cwd: String(dir), - env: { ...bunEnv, ...env, LD_PRELOAD: join(String(dir), "shim.so") }, - stdout: "pipe", - stderr: "pipe", - }); - const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - let report: unknown = stdout; - try { - report = JSON.parse(stdout); - } catch {} - return { report, stderr, exitCode }; - } +afterAll(() => { + dir?.[Symbol.dispose](); +}); +async function runFixture(kind: string, env: Record = {}) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "fixture.js", kind], + cwd: String(dir), + env: { ...bunEnv, ...env, LD_PRELOAD: join(String(dir), "shim.so") }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + let report: unknown = stdout; + try { + report = JSON.parse(stdout); + } catch {} + return { report, stderr, exitCode }; +} + +describe.skipIf(!isLinux || !cc)( + "a pipe writer whose event loop registration fails leaves its fd to the caller", + () => { test.concurrent("Bun.spawn with stdin: 'pipe' closes the stdin pipe exactly once", async () => { expect(await runFixture("stdin-pipe")).toEqual({ // The spawn bindings report a failed stdin setup generically, so only @@ -235,3 +261,25 @@ describe.skipIf(!isLinux || !cc)( }); }, ); + +// The POSIX reader reports a failed registration by calling on_reader_error +// from inside start(), and start() still returns Ok. Terminal's reader +// callbacks end by releasing the reader's ref, which the constructor used to +// take only after start() returned: the release freed the Terminal while the +// constructor was still using it. A reader that ends during the constructor's +// first read (the injected EPOLL_CTL_MOD failure) did not free early. It left +// a constructed Terminal that was already dead and could never be collected. +describe.skipIf(!isLinux || !cc)("a Bun.Terminal whose reader fails to register with the event loop", () => { + describe.each(["pty-reader-add", "pty-reader-mod"])("FAIL_EPOLL_CTL=%s", mode => { + test.concurrent.each([ + ["new Bun.Terminal()", "terminal"], + ["Bun.spawn() with terminal options", "spawn-terminal"], + ])("%s throws and releases the pty", async (_, kind) => { + expect(await runFixture(kind, { FAIL_EPOLL_CTL: mode })).toEqual({ + report: { error: { message: "Failed to start terminal reader" }, leakedFds: 0, leakedWrappers: 0 }, + stderr: "", + exitCode: 0, + }); + }); + }); +});