terminal: take the reader's ref before the reader starts

[robobun]

Problem

• new Bun.Terminal() and Bun.spawn({ terminal: {...} }) use a freed Terminal when the pty reader's epoll_ctl(EPOLL_CTL_ADD) fails. ASAN: heap-use-after-free in RefCount<Terminal>::ref_ from Terminal::init_terminal (Terminal.rs:492), freed by Terminal::on_reader_finished (Terminal.rs:1829). JS receives the dangling pointer.
• PosixBufferedReader::start() (src/io/PipeReader.rs:547) reports a failed poll registration by calling on_reader_error, and still returns Ok. Terminal::on_reader_error releases the reader's ref. init_terminal took that ref only after start() returned, so the release dropped the last ref.

Fix

• init_terminal takes the reader's ref before start(), as SubprocessPipeReader::start does for the same contract.
• If the reader is done after start(), init_terminal releases the pty and the constructor throws Failed to start terminal reader, as the writer arm does.
• The same check runs after the constructor's first read(). On Linux only fault injection reaches it (see Notes).
• Verified: test/js/bun/spawn/spawn-pipe-start-error.test.ts (4 new cases, main fails all 4: 2 ASAN reports, 2 leaked wrappers). Also test/js/bun/terminal/. Self-reviewed: 3 concerns raised, 3 addressed (all on the overlap with spawn/shell/Terminal: remove unsafe from subprocess.rs, shell/subproc.rs and Terminal.rs #40204, see Notes).

Background

• Terminal is intrusively refcounted. The JS wrapper, the pty writer and the pty reader hold one ref each. The reader and the writer release theirs in their close or error callback.
• PosixBufferedReader (src/io) is the poll-driven pipe reader. start() registers the fd with epoll or kqueue.
• Reach: the writer's ADD comes first and already throws (io: leave the fd with the caller when a POSIX pipe writer fails to start #38354). The reader's ADD fails alone only with exactly one epoll watch left (fs.epoll.max_user_watches), or on a transient ENOMEM.

Notes

• Reported via a fuzz ledger (entry 50336): one injected epoll_ctl failure under ASAN on main 09bb546, reproduced with strace -f -qq -o /dev/null -e trace=epoll_ctl -e inject=epoll_ctl:error=ENOMEM:when=4 bun t.mjs. Repro without strace: compile the shim from the test file, then run FAIL_EPOLL_CTL=pty-reader-add LD_PRELOAD=./shim.so bun -e 'new Bun.Terminal({})'.
• Overlap with spawn/shell/Terminal: remove unsafe from subprocess.rs, shell/subproc.rs and Terminal.rs #40204 (open, conflicting with main): that refactor also moves the reader's ref before reader.start() and lists it under "pre-existing bugs fixed in passing". It has only the reorder: no READER_DONE check, no throw, no test. With the reorder alone the constructor returns a Terminal that is already dead (no exit callback, wrapper never collected) where main frees it, and the 4 new cases still fail. This PR adds the checks, the teardown and the tests. When spawn/shell/Terminal: remove unsafe from subprocess.rs, shell/subproc.rs and Terminal.rs #40204 rebases over this, init_terminal must keep both READER_DONE checks and the ReaderStartFailed teardown.
• The post-read check is a guard for the same contract, not a second observed failure. Without it, a reader that ends during the constructor's first read() leaves that same dead Terminal. The re-arm after the first read is an EPOLL_CTL_MOD. The kernel's ep_modify does not return ENOSPC or ENOMEM, and a fresh pty master with its slave open reads EAGAIN, not EOF or an error. The pty-reader-mod shim mode injects a failure there to exercise the guard.
• The release canary 1.4.3-canary.1+b99371011 also misbehaves under the ADD injection: it aborts with panic: usockets_loop: uws_loop not initialized (call ensure_waker first), which is a read of the freed Terminal's event loop handle.
• State after on_reader_error runs inside start() on POSIX, with the fix: finish_io_after_eof has closed the reader and ended the writer, so both of their refs are released and only the initial ref is left. fail_reader_start closes the master and slave fds through close_internal and drops the initial ref. The Err arm of start() (reachable on Windows only) returns the reader's ref itself, because no callback ran.
• Windows: the Err arm of start() is not reachable through the Linux shim. I ran it by hand on a Windows x64 debug build with the debug-only BUN_INTERNAL_FAIL_PIPE_READER_START=1 injection: the constructor throws Failed to start terminal reader, the debug RefCount assertions stay quiet, and no wrapper is left after GC.
• I did not change PosixBufferedReader::start() to return Err for a failed registration. Every other POSIX caller depends on the current contract, and SubprocessPipeReader::start documents why it does not want Err there.
• Other callers of start(): filter_run, multi_run, cron, shell IOReader, git_runner, lifecycle_script_runner, security_scanner and FileReader take their ref or counter before start(), or are not refcounted. FileResponseStream::start (src/runtime/server/FileResponseStream.rs:276) is the exception in shape: it takes its read ref after start() returns Ok and does not check for a finished stream. Its release is gated on a flag, so it cannot free early. I served a FIFO through Bun.serve with the same injection and saw no leaked fd: the second failed registration releases the stream. That site is not part of this PR.
• The test extends the LD_PRELOAD shim in that file. The shim interposes syscall() because Bun registers file polls through syscall(SYS_epoll_ctl, ...). The new modes fail only a readable registration of a pty master (TIOCGPTN succeeds on that fd only).
• The test file change moves the shim build and runFixture from the first describe to module scope so that the new describe can share them. The three existing tests are unchanged.

---

no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/spawn/spawn-pipe-start-error.test.ts

[robobun]

Status: ready for review.

How I reproduced it, on main 09bb546 with the debug ASAN build:

1. Compile the LD_PRELOAD shim from test/js/bun/spawn/spawn-pipe-start-error.test.ts (cc -shared -fPIC -o shim.so shim.c -ldl).
2. Run FAIL_EPOLL_CTL=pty-reader-add LD_PRELOAD=./shim.so bun-debug -e 'new Bun.Terminal({})'. The shim fails only the EPOLL_CTL_ADD that registers the pty master for reading.
3. Result on main: AddressSanitizer: heap-use-after-free in RefCount<Terminal>::ref_ from Terminal::init_terminal, freed by Terminal::on_reader_finished. The release canary 1.4.3-canary.1+b99371011 aborts with panic: usockets_loop: uws_loop not initialized.
4. Result with this branch: the constructor throws Failed to start terminal reader, and no fd or wrapper is left behind.

bun bd test test/js/bun/spawn/spawn-pipe-start-error.test.ts: the 4 new cases fail on main and pass on this branch. The 3 existing cases pass on both.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: f70aebb8-b465-472f-98a6-dabeb66601d6

📥 Commits

Reviewing files that changed from the base of the PR and between 09bb546 and d284743.

📒 Files selected for processing (2)

• src/runtime/api/bun/Terminal.rs
• test/js/bun/spawn/spawn-pipe-start-error.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.

---

Walkthrough

Changes

Terminal reader startup

Layer / File(s)	Summary
Reader startup teardown src/runtime/api/bun/Terminal.rs	init_terminal retains the reader reference before startup, detects early completion, and uses fail_reader_start to close resources and release the initial reference.
PTY startup failure tests test/js/bun/spawn/spawn-pipe-start-error.test.ts	The epoll shim injects pty reader registration failures. Tests cover direct and spawned terminal startup for add and re-arm failures without leaked descriptors or wrappers.

Suggested reviewers: jarred-sumner

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to d2847

The terminal startup failure paths now clean up safely and are covered by targeted tests, with no identified merge-blocking risk.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the primary fix: acquiring the terminal reader reference before reader startup.
Description check	✅ Passed	The description explains the problem, fix, verification, affected tests, and platform-specific limitations. It does not use the template headings exactly, but it provides the required information in e…

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

I reviewed this PR and didn't find any bugs. Because it reorders refcount acquisition against a synchronous-teardown callback and adds two new free-on-error paths in Terminal::init_terminal, a human familiar with the Terminal refcount lifecycle should confirm the balance — particularly the Windows Err arm and the state finish_io_after_eof leaves behind.

What was reviewed:

• Traced refcounts through all three fail_reader_start call sites: Err arm (3 refs → read_fd close + reader deref + close_internal's writer deref + initial deref), Ok+READER_DONE arm (finish_io_after_eof already released reader+writer, close_internal's on_writer_close no-ops on WRITER_DONE, initial deref frees), and post-IOReader::read arm (same, with READER_STARTED set) — each reaches zero with no use of terminal after the final deref.
• Confirmed fail_reader_start's deref_() is tail-position per the file's existing convention (Terminal.rs:397-398), and no new provenance pattern is introduced beyond what deref_(&self) already does.
• Test shim: should_fail with FAIL_EPOLL_CTL unset preserves the original EPOLLOUT-ADD behavior, so the three existing writer tests are unaffected; the hoisted runFixture is only reachable from describe.skipIf(!isLinux || !cc) so dir is always defined when used.
• Fixture baseline: JSON.stringify drops code: undefined, so the new { message: "Failed to start terminal reader" } assertion matches exactly under toEqual.

Extended reasoning...

Overview

The PR fixes a use-after-free in Bun.Terminal initialization: on POSIX, PosixBufferedReader::start() reports a failed epoll registration by synchronously invoking on_reader_error (which releases the reader's ref via on_reader_finished → deref_()) while still returning Ok. The old code took the reader's ref only after start() returned, so the callback's deref dropped the count to zero and freed the Terminal mid-constructor. The fix moves terminal.ref_() before start(), adds a READER_DONE check after start() and again after the constructor's first IOReader::read, and centralizes the error-path teardown in fail_reader_start() (close_internal + drop initial ref). The test file extends the existing LD_PRELOAD syscall() shim with FAIL_EPOLL_CTL=pty-reader-add|pty-reader-mod modes gated on ioctl(fd, TIOCGPTN) to target only the pty master's readable registration, and adds four cases (2 entry points × 2 injection modes) asserting the throw, zero leaked fds, and zero leaked wrappers.

Security risks

None user-facing. This is an internal refcount-ordering fix on an error path reachable only under epoll ENOSPC/ENOMEM. No new input parsing, no auth/crypto/permissions surface.

Level of scrutiny

High. This is intrusive-refcount native code where the fix itself introduces two new paths that free self before init_terminal returns. I traced each arm's refcount from the initial heap::into_raw (1) through writer start (2), reader ref (3), and back down to zero, cross-referencing on_reader_finished, finish_io_after_eof, on_writer_close, and close_internal to confirm each deref is paired and WRITER_DONE gates the double-deref in on_writer_close. The Err arm (Windows-only per the reader contract) now does deref_() + fail_reader_start(), which is three total derefs matching three refs — correct only because no callback ran, which the PR's comment states and the reader contract supports. The &self-then-free pattern in fail_reader_start mirrors the file's documented deref_() convention and introduces nothing new provenance-wise. That said, refcount fixes in this file are exactly what REVIEW.md calls the most-blocked category, and the Windows arm is untested by the Linux-only shim.

Other factors

Test quality is strong: fault injection via LD_PRELOAD targets the exact syscall, TIOCGPTN scoping avoids perturbing unrelated fds, the fixture measures both fd and wrapper leaks against a baseline that anchors the relevant prototypes, and the PR states all four cases fail on main (2 ASAN UAF, 2 leaked wrappers). The hoist of beforeAll/runFixture to module scope is a mechanical refactor guarded by the same !isLinux || !cc predicate. The PR description's audit of other start() callers and the note on FileResponseStream show the whole-class check was done. No CODEOWNERS conflict apparent. Deferring because a human should independently confirm the three-arm refcount balance and the Windows Err path assumption.

[robobun]

^{Updated 1:47 PM PT - Sep 13th, 2026}

✅ @robobun, your commit d2847434c34e1072594ad702529d9e52dee4914d passed in Build #115282! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 42654

That installs a local version of the PR into your bun-42654 executable, so you can run:

bun-42654 --bun

[robobun]

On the Windows Err arm: I ran it on a Windows x64 debug build of this branch (d284743).

WindowsBufferedReader::start_with_current_pipe has a debug-only injection point, BUN_INTERNAL_FAIL_PIPE_READER_START=1, that makes reader.start() return Err with no callback. With it set:

• new Bun.Terminal({ data() {}, exit() {} }) throws Failed to start terminal reader.
• The process exits 0 with nothing on stderr. The debug RefCount assertions do not fire.
• heapStats().objectTypeCounts.Terminal is back at its baseline after Bun.gc(true), so no wrapper is left.

Without the injection the same script constructs the terminal, closes it, gets the exit callback, and also leaves no wrapper.

The state after finish_io_after_eof is what the two pty-reader-add cases run on Linux: reader closed, writer ended, both refs released, close_internal closes the master and slave fds, and the initial ref is the last one.