spawn: release the buffer-stdin writer's start() ref when its write fails on POSIX by robobun · Pull Request #37774 · oven-sh/bun

robobun · 2026-08-12T06:29:03Z

Problem

On POSIX, a Buffer or Blob passed as a child's stdin leaks the native writer that pumps it into the child when the write fails before the buffer drains, typically EPIPE because the child closed its stdin without reading it.
Reached by the shell's cmd < ${buffer} redirect on every POSIX configuration, and by Bun.spawn always on macOS but on Linux only when memfd is unavailable.
Under LeakSanitizer the shell's stranded writers appear as Direct leak of 1024 byte(s) in 2 object(s) from StaticPipeWriter<ShellSubprocess>::create; with refcount logging the count stays at 1 forever after onError(err=EPIPE: Broken pipe (send())) and onClose().
Cause: the writer holds a ref on itself while a write is in flight, and only a drained buffer or the owner closing it at exit released that ref. After a failed write the writer closes itself and the owner empties its slot, so nothing that could release the ref can still reach the writer.

Fix

The writer's close callback releases the in-flight ref on every platform, not only Windows. Sound because close is the last callback a parent receives for an error, and every release site claims the same token first, so exactly one of them (drain completion, close after a failure, or the owner) releases.
The POSIX drain loop now only writes and cannot invoke callbacks itself; an error is always returned and reported once by the caller. This deletes the arm that spawn(windows): release StaticPipeWriter start() ref when buffer-stdin write completes #35297 cited when it fixed the same leak on Windows only; the arm was unreachable, so no behaviour changes here.
Two rarer error paths get the same report-then-close shape: a failed poll re-registration used to report without closing, and a zero-byte write closed without releasing. Both are reasoned about, not tested. Out of scope, pre-existing: on Windows a write failing synchronously inside start() still strands the ref; tracked separately.
Verification: a new ASAN-lane test runs a fixture under detect_leaks=1 that drives both owners through failing and draining writes. It fails 4 of 4 runs without the fix with the report above and passes 5 of 5 with it. LSan only reports the shell-owned writers; the Bun.spawn cases exercise the same teardown without being what fails.

Background

The static pipe writer is the native object that writes one fixed in-memory buffer (a Buffer or Blob stdin) into a child's stdin pipe. It is refcounted: one ref lives in the owning process's stdin slot, a second is taken when the write starts so the object outlives the write.
It has two owners: a Bun.spawn subprocess and a shell subprocess for the < ${buffer} redirect. On Linux Bun.spawn normally hands the child a memfd and never creates the writer, which is why the test disables memfd; the shell redirect always uses it.
The buffered pipe writer underneath reports to its parent through three callbacks: bytes drained, error, closed. Parents are written assuming closed is the last one they receive, so freeing the object there is safe only if nothing underneath touches it afterwards.
The started flag is the token for the in-flight ref: whichever site swaps it to false does the release, so three possible release sites cannot release twice.
EPIPE is what writing to a pipe returns once the reader has closed its end; it is how a child closing its stdin reaches the parent while the buffer is still being written.

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-stdin-pipe-fd-leak.test.ts

Original description

On POSIX, a Buffer/Blob stdin whose write fails before it has drained (typically EPIPE, because the child closed its stdin without reading it) leaks the native StaticPipeWriter that pumps the buffer into the child. Two owners share that writer: the shell's cmd < ${buffer} redirect, which uses it on every POSIX configuration, and Bun.spawn, which on Linux only uses it when memfd is unavailable (BUN_FEATURE_FLAG_DISABLE_MEMFD=1, or no memfd_create; otherwise the child gets a memfd) and on macOS always.

Repro

// BUN_FEATURE_FLAG_DISABLE_MEMFD=1 BUN_DEBUG_StaticPipeWriter=1 BUN_DEBUG_ref_count=1 bun-debug run repro.ts
const big = Buffer.alloc(1 << 20, 0x61);
const proc = Bun.spawn({ cmd: ["sh", "-c", "exec 0<&-; sleep 0.3"], stdin: big, stdout: "ignore" });
console.log("exit", await proc.exited);

Before, for the writer:

StaticPipeWriter(0x..) start()
0x..   ref 1 -> 2                      start()
StaticPipeWriter(0x..) onError(err=EPIPE: Broken pipe (send()))
StaticPipeWriter(0x..) onClose()
0x.. deref 2 -> 1                      owner drops create()'s ref in on_close_io

and the count stays at 1 forever. After, the same trace continues with deref 1 -> 0. $\cmd < ${big}`` with a command that exits without reading stdin shows the same before/after (no memfd flag involved), and under LeakSanitizer the shell's stranded writers come out as

Direct leak of 1024 byte(s) in 2 object(s) allocated from:
    ...
    #12 <StaticPipeWriter<ShellSubprocess>>::create src/spawn/static_pipe_writer.rs:110
    #13 <shell::subproc::Writable>::init src/runtime/shell/subproc.rs:1149

Cause

start() takes a ref on the writer and sets started; that ref was released from on_write when the buffer drained, or by the owner (Subprocess::on_process_exit / close_io) if it closed the writer while the slot still held it. A failed write takes neither route: PosixBufferedWriter::_on_error runs on_error and then close(), on_close tells the owner, the owner empties its slot and drops create()'s ref, and nothing is left that can find the writer to release start()'s ref.

#35297 fixed exactly this on Windows by releasing in on_close, and left POSIX alone because of the shape of PosixPipeWriter::drain_buffered_data: it had an arm that, for an error after a partial drain, called on_error inline (closing the writer) and returned Wrote(n), after which on_poll delivered on_write(n, Drained) to the same object, so a release in on_close looked unsafe. That arm was in fact unreachable (try_write reports a short write as Pending, never as Wrote, so the drain loop never gets to an error with bytes already drained), but as written it was the documented reason the POSIX release site did not exist.

Fix

drain_buffered_data (src/io/PipeWriter.rs) loses that arm and takes &self, so it structurally cannot dispatch callbacks: an error is always returned as Err and reported once by the caller, and on_close is the last callback a parent sees. That is the contract the parents are written against (FileSink::on_close releases its keep-alive as "the last thing", Terminal::on_writer_close derefs the terminal, FileSink::on_auto_flush documents flush() returning Err as the error contract); this makes it explicit rather than a property of which arms happen to be reachable. No behaviour changes here.
StaticPipeWriter::on_close (src/spawn/static_pipe_writer.rs) claims and releases start()'s ref on every platform, not just Windows. This is the behavioural fix. The release is the last access: the frames underneath it (close_impl -> close -> _on_error -> on_poll) do nothing with the writer after the callback, which is the same shape SecurityScanSubprocess (whose owner ref is the last one) already relies on in on_close_io today. on_write keeps taking the token before it closes and the owners keep taking it before they close, so exactly one site releases the ref.
PosixBufferedWriter::register_poll reports a failed re-registration through _on_error (report, then close()), like every other error report and like the streaming writer's register_poll already did, instead of reporting and leaving the writer open. Raised in review: that (ENOMEM-class) failure was the one report not followed by the on_close that now releases the ref; Bun.spawn would still have picked the writer up at exit, the shell had nothing to pick it up with. Reasoned about rather than tested, since it needs the registration syscall itself to fail.
The EndOfFile arm of on_write releases the ref as well (non-final, since the owner's ref is dropped by the on_close the buffered writer delivers right after) instead of closing the writer itself and leaving the ref outstanding. That arm needs write(2) to return 0 on a pipe, so it is reasoned about rather than tested; it is the same change io: report to pipe writer parents through raw pointers, release StaticPipeWriter through its backref #37755 makes to this arm.

Known gap, pre-existing and not touched here: on Windows a uv_write that fails synchronously inside start() closes the writer (on_close runs, the owner drops its ref) before start() sets started, which strands the ref the same way. Fixing it needs SecurityScanSubprocess's post-start() release to become token-aware as well, so it is tracked separately.

Related: #34697 proposes the same drain_buffered_data change as one item of a broader hardening pass, but is 600+ commits behind with conflicts in three files. #37755 moves these callbacks onto raw this pointers and leaves the error chain (and this leak) alone; the two conflict textually in on_close / on_poll but compose, whichever lands second needs to keep the unconditional release in on_close and the Err return in drain_buffered_data.

Test

test/js/bun/spawn/spawn-stdin-pipe-fd-leak.test.ts runs a fixture on the ASAN lane with detect_leaks=1 (the setup from shell-worker-terminate-leak.test.ts). The fixture drives both owners through both endings: children that close their stdin, report it and wait to be killed (so the write fails while the child is alive and the exit path cannot be what cleans up), and children that drain it; memfd is disabled so that the Bun.spawn scenarios reach the writer at all. Unfixed it fails every time (4 of 4) with the report quoted above: the two shell-owned writers. The two stranded Bun.spawn-owned writers from the same run have their refcount stuck at 1 just the same (checked with BUN_DEBUG_ref_count, and their Subprocess structs are freed before exit), but LSan still finds their address somewhere and does not report them, so those scenarios only exercise the same teardown under ASAN. Fixed, the fixture is clean, 5 of 5 runs, about 0.7s each.

An earlier revision used a live-writer counter in bun:internal-for-testing; it was removed per review in favour of LSan.

Verification

Debug (ASAN) build: the file above, plus spawn.test.ts, spawnSync.test.ts, spawn-empty-arrayBufferOrBlob, spawn-streaming-stdin, spawn-stdin-readable-stream, spawn-stdin-destroy, spawn-pipe-start-error, spawn-many-teardown, memfd-disabled, bunshell.test.ts (418 pass), bunshell-instance, epipe, shell-blocking-pipe, shell-write-fault, file-io, pipeline_stack, filesink.test.ts, terminal.test.ts, terminal-spawn, child_process.test.ts and bun-security-scanner-workspaces all pass. cargo check of bun_io and bun_spawn for x86_64-pc-windows-msvc and aarch64-apple-darwin, cargo clippy on both, and cargo fmt --check are clean. CI on the two previous revisions (the first of which ran its tests on every lane, Windows included) was green apart from flaky tests that passed on retry.

…ails on POSIX A StaticPipeWriter (Buffer/Blob stdin for Bun.spawn and the shell's `< ${buffer}` redirect) takes a ref in start() and released it only from on_write, so a write that failed before draining (EPIPE after the child closed its stdin) went on_error -> close() -> on_close -> on_close_io, dropped the owner's ref, and stranded start()'s one: one writer leaked per such spawn. #35297 fixed the same thing on Windows by releasing in on_close, but could not do so on POSIX because drain_buffered_data reported an error that followed a partial drain inline and on_poll then still delivered on_write to the same object. drain_buffered_data now returns Err for that case as well, so an error is reported exactly once and on_close is the last callback a parent sees (FileSink and Terminal relied on that already; the post-error on_write was a no-op for the shell's IOWriter). With that, on_close claims and releases start()'s ref on every platform, and the EOF arm of on_write releases it too instead of leaving it outstanding. Adds subprocessInternals.staticPipeWriterLiveCount to bun:internal-for-testing; the leaked writer is about a hundred bytes, so RSS-based tests cannot see it.

coderabbitai · 2026-08-12T06:29:12Z

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 9 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: a48462aa-e398-4a5c-ba0e-8f54419d44e2

📥 Commits

Reviewing files that changed from the base of the PR and between 65baeda and 53f4fb5.

📒 Files selected for processing (5)

src/io/PipeWriter.rs
src/runtime/api/bun/subprocess.rs
src/spawn/static_pipe_writer.rs
test/js/bun/spawn/spawn-stdin-buffer-writer-leak-fixture.ts
test/js/bun/spawn/spawn-stdin-pipe-fd-leak.test.ts

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

robobun · 2026-08-12T06:30:50Z

Status: reproduced on the debug build with BUN_DEBUG_StaticPipeWriter=1 BUN_DEBUG_ref_count=1 (the writer's refcount stops at 1 after onError(EPIPE) -> onClose(), for the shell's < ${buffer} redirect on any POSIX config and for Bun.spawn with memfd disabled); with this branch both traces end in deref 1 -> 0. The test (ASAN lane, LeakSanitizer) fails on the unfixed build with the stranded shell-owned writers reported as direct leaks and passes here.

Review: the internals binding is removed as requested; the register_poll sibling raised in review is closed in 53f4fb5; the unrelated Windows start() gap is tracked separately. No open threads.

CI for 53f4fb5 (build 93157): 192 of 194 jobs passed, including the darwin 14 aarch64 test shards; the only two that did not run are the darwin 26 aarch64 test shards, which expired without ever getting an agent (that lane is currently in the same state on every build in the pipeline, so a retrigger would not change it). The three red tests in the run all passed on retry and are unrelated (fs read stream pos, watch-mode kill signal, solc). Ready for review.

claude

I reviewed this PR and didn't find any bugs. Because it changes the refcount-release protocol for StaticPipeWriter and the drain_buffered_data callback contract that every PosixPipeWriter parent (FileSink, Terminal, shell IOWriter) relies on, a human look would still be worthwhile.

What was reviewed:

Traced on_poll → _on_error → close → close_impl → Parent::on_close in PipeWriter.rs/pipes.rs — no access to the writer after the callback returns on POSIX, so freeing in on_close is safe on that path.
Checked PosixStreamingWriter::flush (the other drain_buffered_data caller) — a partial-drain error now hits the _ => outgoing.reset() arm and returns Err to the caller instead of dispatching on_error inline; matches the documented contract.
Verified the started token is claimed exactly once across on_write (Drained/EOF), on_close, and Subprocess::take_pending_start_writer; the shell's separate drain_buffered_data copy in IOWriter.rs is unaffected.

Extended reasoning...

Overview

Fixes a POSIX-only refcount leak of StaticPipeWriter when a Buffer/Blob stdin write fails (typically EPIPE). The fix has two parts: (1) PosixPipeWriter::drain_buffered_data now returns Err on any write error rather than dispatching on_error inline for the partial-drain case, so an error report is terminal and on_close is guaranteed to be the last callback; (2) StaticPipeWriter::on_close releases start()'s ref unconditionally (dropping the #[cfg(windows)] gate), and the EndOfFile arm of on_write releases it non-finally instead of leaving it stranded. A live-count atomic and bun:internal-for-testing binding are added so tests can observe the ~100-byte leak, and two new tests exercise the EPIPE path via Bun.spawn (fixture with memfd disabled) and the shell's < ${buffer} redirect.

Security risks

None. This is native lifetime/refcount management for subprocess stdin buffers; no user-controlled parsing, auth, or trust boundaries.

Level of scrutiny

High — this is exactly the "memory safety / reference counts provably balanced on every terminal path" category REVIEW.md calls out as most-blocked. The drain_buffered_data protocol change affects every PosixPipeWriter parent (FileSink, Terminal, shell IOWriter, StaticPipeWriter), and the on_close release relies on the guarantee that no frame below it touches the writer after the callback returns. I traced those frames (close_impl → close → _on_error → on_poll) and confirmed they don't, and the PR notes SecurityScanSubprocess already depends on the same shape today. The author's analysis of each parent's behavior under the new contract (FileSink's keep-alive release, Terminal's deref, shell IOWriter's queue-clearing on_error) reads correctly, but the number of paths and the interaction with #37755 warrant a maintainer's sign-off.

Other factors

The verification section is thorough (ASAN debug across spawn, shell, filesink, terminal, child_process suites; cross-target cargo check). Tests are well-constructed: the fixture keeps the child alive after closing stdin so process-exit cleanup cannot mask the leak, and both scenarios (write fails, write drains) are covered. The PR explicitly flags textual conflicts with #37755 and #34697 and how to compose them, which a human should confirm when merging.

… the failed write before killing them The fixture's children only need to close or drain stdin, which sh does in a few milliseconds where a debug bun child spends most of a second loading node:fs; Windows keeps the bun children. Waiting (bounded) for the failed write to free the writer before the children are killed makes it explicit that the error path, not the child-exit path, is what is being checked. The test no longer needs a per-test timeout.

Jarred-Sumner

Remove the internals? LSAN should cover it.

robobun · 2026-08-12T07:25:57Z

Will do. Removing the binding and the counter; the tests will run their scenarios in a child with detect_leaks=1 (the same setup as shell-worker-terminate-leak.test.ts) so LSAN reports the stranded writer on the ASAN lane. I'll confirm here once I've checked that LSAN actually flags it on the unfixed build.

robobun · 2026-08-12T08:05:23Z

^{Updated 11:05 AM PT - Aug 12th, 2026}

❌ @robobun, your commit 53f4fb5 has some failures in Build #93157 (All Failures)

🧪 To try this PR locally:

bunx bun-pr 37774

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

bun-37774 --bun

…a testing counter Drops the live-writer counter and its bun:internal-for-testing binding. The scenarios now run in one fixture process with detect_leaks=1 on the ASAN lane, covering both owners of the writer (Bun.spawn and the shell redirect) and both ways a write ends; the stranded writers show up as direct leaks allocated from StaticPipeWriter::create.

robobun · 2026-08-12T08:29:13Z

Done in f2009c8: the counter and the bun:internal-for-testing binding are gone. The scenarios now run in one fixture process on the ASAN lane with detect_leaks=1 (same setup as shell-worker-terminate-leak.test.ts), covering both owners of the writer and both ways a write ends.

What LSan sees, checked against a build with the fix reverted (4 of 4 runs):

Direct leak of 1024 byte(s) in 2 object(s) allocated from:
    #12 <StaticPipeWriter<ShellSubprocess>>::create src/spawn/static_pipe_writer.rs:110
    #13 <shell::subproc::Writable>::init src/runtime/shell/subproc.rs:1149

Those are the two shell-owned writers. The two stranded Bun.spawn-owned writers from the same run are not reported: their refcount is stuck at 1 exactly like the shell ones (verified with BUN_DEBUG_ref_count), and the Subprocess structs that owned them are freed before exit, but LSan still finds their addresses somewhere and treats them as reachable; I did not track down where. Since the fix is entirely inside the shared writer, the shell half is what makes the test fail before / pass after, and the Bun.spawn half runs the same teardown under ASAN. With the fix the fixture is clean, 5 of 5 runs, about 0.7s each.

Also trimmed the comments in the last push.

claude

I reviewed this PR and didn't find any bugs. Because it changes the drain_buffered_data callback contract shared by every PosixPipeWriter parent and reworks StaticPipeWriter's ref-release ordering across three sites, a human look at the refcount reasoning would still be worthwhile.

What was reviewed:

Traced the on_poll → _on_error → close() → close_impl → Parent::on_close chain on POSIX; the callback is the last statement in each frame, so the deref in on_close is not followed by any access through the freed writer.
Checked the second drain_buffered_data caller, PosixStreamingWriter::flush(): the zero-drain error already returned Err, so callers already handle that arm; the partial case now takes the same route.
The EndOfFile arm's deref is guarded by the owner's slot ref and the buffered writer's own follow-up close; the token swap makes it single-claim.

Extended reasoning...

Overview

The PR fixes a POSIX-only leak of StaticPipeWriter when a Buffer/Blob stdin write fails (EPIPE) before draining. Three source files change: src/io/PipeWriter.rs (the shared PosixPipeWriter::drain_buffered_data trait method now returns Err for a partial-drain-then-error instead of calling on_error inline and returning Wrote(n); it also drops to &self), src/spawn/static_pipe_writer.rs (removes the #[cfg(windows)] gate on on_close's ref release, splits the on_write EndOfFile arm so it derefs without closing, and rewrites the ownership comments), and src/runtime/api/bun/subprocess.rs (comment-only). A new LSan-gated fixture and test case are added.

Security risks

None identified. This is internal refcount bookkeeping on a subprocess stdin writer; no user-controlled input reaches new validation or parsing.

Level of scrutiny

High. Per REVIEW.md, refcount balance on every terminal path is the most-blocked category, and this PR both changes a shared trait method's contract (affecting PosixBufferedWriter and PosixStreamingWriter, whose parents include FileSink, Terminal, the shell's IOWriter, and StaticPipeWriter) and removes a platform gate that #35297 placed deliberately. The safety of the new POSIX on_close deref rests on the claim that every frame below it (close_impl → close → _on_error → on_poll) does nothing with the writer after the callback — I traced this and it holds today, but a human should confirm the reasoning for the other PosixPipeWriter parents whose on_error/on_close ordering changed on the partial-drain path.

Other factors

The PR description is unusually thorough (ref-count traces before/after, LSan repro, cross-target cargo check, and a large targeted test-suite run) and the bug-hunting pass found nothing. All comment-cop bot findings are marked resolved after de04367. The test is properly gated to the ASAN lane, uses the harness tempDir/bunEnv conventions, drains pipes concurrently, and asserts a combined {stdout, stderr, exitCode} object. Still, the change is not mechanical — it reorders which of three sites releases start()'s ref and changes when partial-drain errors surface — so I'm deferring rather than approving.

… test try_write never reports a short write as Wrote, so the removed partial-drain arm was unreachable; say that instead of describing it as a live case. The memfd flag only affects the Bun.spawn scenarios: the shell redirect builds the writer on every configuration.

claude

Beyond the inline nit, I traced the drain_buffered_data protocol change through its other callers: the removed drained > 0 inline-on_error branch was unreachable (try_write only returns Wrote for a full slice, so the outer loop never iterates past a partial write), so PosixStreamingWriter::flush() and on_poll's Err handling see identical results. Also confirmed PosixBufferedWriter::_on_error → close() does nothing after Parent::on_close returns, matching the SAFETY comment on the new deref.

Extended reasoning...

The bug hunter's nit on register_poll will be posted inline. This note records two additional concerns I checked and ruled out: (1) whether the &mut self → &self + dropped-branch change in drain_buffered_data alters behavior for the streaming writer's flush() or any on_poll caller — it does not, because the drained > 0 && Err state was structurally unreachable given try_write_with_write_fn's contract; and (2) whether the frames beneath the new POSIX on_close deref (close() at PipeWriter.rs:471–487, _on_error at 389–395) touch self after the callback — they do not. The refcount ownership reasoning is sound as far as I traced it, but this is memory-safety-critical native code with a self-freeing deref inside a callback chain, so a human look at the overall ref protocol is still warranted.

PosixBufferedWriter::register_poll reported the failure through on_error but left the writer open, unlike every other error report (including the streaming writer's register_poll), so the parent never got the on_close that now releases the buffer-stdin writer's start() ref; the owner had to pick it up at exit instead, and the shell had nothing to pick it up with.

claude

Thanks for addressing the register_poll sibling — the _on_error route now matches PosixStreamingWriter and makes the "every error pairs with on_close" invariant hold for the buffered writer too. I re-traced the frames under the new POSIX on_close deref (close_impl → close → _on_error/_on_write → on_poll) and confirmed none touch self after the callback returns; also checked that shell IOWriter (the other PosixBufferedWriter parent, which calls register_poll() directly at IOWriter.rs:803) has a trivial on_close and nothing follows the call, so the added close() there is safe.

Given this is refcount-lifecycle code with UAF potential across three callback sites and two owner types, a human look is still warranted before merge.

(The open comment-cop note on PipeWriter.rs:433 is on a two-line comment stating the invariant; looks like a false positive.)

Extended reasoning...

Overview

Fixes a POSIX-only leak of StaticPipeWriter when a Buffer/Blob stdin's write fails (EPIPE) before draining. Touches src/io/PipeWriter.rs (drain_buffered_data → &self, drops an unreachable arm; PosixBufferedWriter::register_poll now routes errors through _on_error), src/spawn/static_pipe_writer.rs (on_close releases start()'s ref on all platforms; on_write's EndOfFile arm reordered to deref instead of self-close), a comment trim in subprocess.rs, and an LSan-gated regression test + fixture.

What changed since my last review

Commit 53f4fb5 took the suggestion from my prior inline comment: PosixBufferedWriter::register_poll's Err arm now calls self._on_error(err) (which follows with close()) instead of bare parent_on_error(err), matching the streaming writer. I verified the two external callers of that method — on_poll's Pending arm (last statement, safe to free) and shell IOWriter::on_write_pollable at IOWriter.rs:803 (last statement in the function; shell's on_close only flips is_writing) — both tolerate the writer being freed inside the call.

Security risks

None user-facing. The risk here is memory-safety (over-deref → UAF, or under-deref → leak). I re-walked: (a) EPIPE path — _on_error → close → close_impl invokes on_close_fn last and touches nothing after; (b) EndOfFile path — _on_write calls close_without_reporting first, then Parent::on_write (derefs start's ref, owner's ref keeps it live), then close() → Parent::on_close (started already false, owner drops create's ref → 0); _on_write does not read this after close(). (c) Drained path unchanged in ordering. The started token is claimed with mem::replace at every site so exactly one releases.

Level of scrutiny

High. This is intrusive-refcount lifecycle code where the last deref runs inside a callback with caller frames still on the stack, across three release sites (on_write, on_close, owner's take_pending_start_writer) and three owner types (Subprocess, ShellSubprocess, SecurityScanSubprocess). REVIEW.md lists this as the most-blocked category. The reasoning in the PR description is thorough and I found no fault with it, but the blast radius of a mistake is a UAF, so a maintainer sign-off is appropriate.

Other factors

The LSan test follows the shell-worker-terminate-leak pattern, is ASAN-gated, and the description records it fails 4/4 unfixed and passes 5/5 fixed. The drain_buffered_data &self change structurally enforces the "no callback dispatch" contract the fix depends on. One open comment-cop note (line 433) targets a two-line comment and appears to be a bot false positive.

github-actions Bot added the claude label Aug 12, 2026

claude Bot reviewed Aug 12, 2026

View reviewed changes

Jarred-Sumner requested changes Aug 12, 2026

View reviewed changes