Skip to content

spawn(windows): free the buffer-stdin writer when its uv_write fails inside start() - #37898

Open
robobun wants to merge 4 commits into
mainfrom
farm/c4cbfc73/static-pipe-writer-sync-write-failure
Open

spawn(windows): free the buffer-stdin writer when its uv_write fails inside start()#37898
robobun wants to merge 4 commits into
mainfrom
farm/c4cbfc73/static-pipe-writer-sync-write-failure

Conversation

@robobun

@robobun robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • On Windows, a child given a Buffer or Blob stdin (Bun.spawn, Bun.spawnSync, the shell's < ${buffer} redirect, or the install security scanner) leaks its stdin writer for the rest of the process when the child's end of the pipe is already gone by the time the write is issued.
  • Cause: on Windows start() is what issues the uv_write, and a synchronous failure closes the writer inside that call, before start() has recorded the extra ref it took. The close therefore releases nothing, the owner drops its own ref and forgets the writer, and the ref start() took has no release site left.
  • Trace from the original: ref 1 -> 2, onClose(), deref 2 -> 1, onError(err=EPIPE: Broken pipe (write())); the count stays at 1.
  • Sibling of spawn(windows): release StaticPipeWriter start() ref when buffer-stdin write completes #35297 (the same write failing asynchronously) and spawn: release the buffer-stdin writer's start() ref when its write fails on POSIX #37774 (the POSIX error path, which names this case as the remaining gap).

Fix

  • On Windows, after starting the buffered writer, start() checks whether the writer already closed underneath it; if so it releases its own ref there, leaves started unset and returns Ok. Property to check: on every path exactly one site releases the ref start() took, and started says which one.
  • Ok rather than Err because the owner was already told through on_error and on_close, the same as for an asynchronous failure; Err would make Bun.spawn kill the child and throw on an ordinary EPIPE.
  • Since that release can free the writer, start() takes a raw pointer and none of its three callers touch the writer afterwards. The security scanner, which releases start()'s ref itself, now does so only while the started token is set; without that it released one ref too many and bun install hung. POSIX behaviour is unchanged.
  • Verification: the failure cannot be produced from JS, so a debug-only BUN_INTERNAL_FAIL_PIPE_WRITER_WRITE flag fails the write synchronously and a new test (Windows debug builds only) counts writers created and freed across all three owners. The injected spawn case fails without the fix (created: 3, freed: 0) and 5/5 pass with it. Those tests do not run on this PR's CI lanes; the existing spawn, shell, scanner and child_process suites were run by hand on Windows and Linux (details in the original).

Background

  • StaticPipeWriter is the object behind a Buffer or Blob stdin: it writes the bytes into the child's stdin pipe once, then closes. One implementation serves Bun.spawn/spawnSync, the $ shell and the install security scanner; each of these is an owner holding a slot that points at it.
  • The writer is intrusively refcounted. The owner holds the ref from create(); start() takes a second one for the duration of the write, and the boolean started records that this second ref is outstanding. Whichever release site sees started set clears it and releases that ref, so exactly one site does.
  • On Windows the pipe is driven by libuv. uv_write normally reports later through a callback, but returns an error immediately if the far end is already closed; Bun's Windows buffered writer then closes and reports the error on the spot, so the owner's close callback runs re-entrantly inside start(). On POSIX, start() only registers the fd with the event loop, so nothing runs re-entrantly.
  • BUN_INTERNAL_* flags compiled only under cfg(debug_assertions) are the repo's existing way to exercise error paths JS cannot reach; release builds contain none of the injection.
Original description

On Windows, the writer that pumps a Buffer/Blob stdin into a child (StaticPipeWriter: used by Bun.spawn/Bun.spawnSync, by the shell's cmd < ${buffer} redirect and by the security scanner for its package-list pipe) is leaked when its uv_write fails synchronously inside start(), i.e. when the child's end of the pipe is already gone by the time the write is issued. Sibling of #35297, which fixed the asynchronous failure of the same write, and of #37774, which fixes the POSIX error path and names this case as the remaining gap.

Cause

StaticPipeWriter::start() takes a ref on the writer, starts the buffered writer, and then records the ref in started. On Windows starting the buffered writer is what issues the uv_write (WindowsBufferedWriter::start_with_current_pipe -> write() in src/io/PipeWriter.rs), and when that returns an error, write() closes the writer on the spot. So, still inside start():

StaticPipeWriter(0x..) start()
0x..   ref 1 -> 2                        start()'s ref
StaticPipeWriter(0x..) onClose()         `started` is still false, so this releases nothing
0x.. deref 2 -> 1                        owner's on_close_io drops create()'s ref and empties its slot
StaticPipeWriter(0x..) onError(err=EPIPE: Broken pipe (write()))

start_with_current_pipe() returns Ok regardless, and start() set started = true on a writer no owner could reach any more: Subprocess::take_pending_start_writer looks in the (now empty) stdin slot, and the shell has no release site for this ref on Windows at all. The count stays at 1 for the rest of the process.

Fix

  • StaticPipeWriter::start() (src/spawn/static_pipe_writer.rs): after starting the buffered writer, if the writer is already closed (is_done(), which at this point only a failed write can have set), release start()'s ref right there and return Ok with started left unset. The is_done() check is the fixing line. Ok is deliberate: the failure has already been delivered to the owner through on_error/on_close, exactly as an asynchronous failure of the same write is, and returning Err would make Bun.spawn kill the child and throw over an ordinary EPIPE. That release frees the writer for Subprocess and ShellSubprocess, so start() takes this: *mut Self instead of &mut self (the shape Make re-entrant runtime objects &self-only; delete AnyTask #36571 and spawn: release the stdio PipeReader through its pointer, not a &mut receiver #37879 use for the reader's equivalent entry points); the three callers pass the slot's pointer and do not touch the writer afterwards. The POSIX arm behaves as before: the buffered writer's start() there only registers the poll and never reports to the parent, so nothing can close the writer underneath it.
  • SecurityScanSubprocess::finish_spawn (src/install/PackageManager/security_scanner.rs): this owner releases start()'s ref itself right after start() returns. It now claims it through the started token, the same token on_write/on_close/take_pending_start_writer use, so it skips the release when start() has already done it. Without this hunk the change above would make it release one ref too many on the synchronous-failure path (checked: with only the start() hunk applied, the scanner test below hangs bun install after the writer it still holds is freed). The same check covers start() returning Err, where the scanner was already releasing one ref too many (the security_scanner.rs half of install(security-scanner): gate post-start() deref on Ok to avoid over-deref #31032; that path is unreachable on Windows and needs poll registration to fail on POSIX). The two comments in that file that describe the re-entrancy as "the write completes synchronously" are corrected at the same time: neither platform completes the write inside start(); the synchronous failure on Windows is what re-enters.

Test

The failing uv_write cannot be arranged from JS (the child's end of a pipe created microseconds earlier would have to be closed already), so this follows #35150, which had the same problem for the reader: a debug-only BUN_INTERNAL_FAIL_PIPE_WRITER_WRITE makes the buffered writer's stream write fail synchronously (src/io/PipeWriter.rs, under cfg(debug_assertions)), and create()/deinit() lines are added to the existing StaticPipeWriter debug scope so a test can count writers, the way the websocket and tsconfig leak tests count alloc lines. Release builds get no test-only code.

test/js/bun/spawn/spawn-pipe-start-error.test.ts (Windows debug builds, next to the reader's test) drives all three owners, each with and without the injection: a fixture running Bun.spawn, Bun.spawnSync and a shell < ${buf} redirect (the children report how many stdin bytes they received: 0 with the injection, 4096 without, which also shows the injection fired), and bun install with a local scanner (it runs even with nothing to scan, so no registry is involved; with the injection the scanner reports the empty package list it gets). Every case asserts created == freed.

On Windows x64 (debug build), with everything but the two fixing lines applied:

(fail) ... > Bun.spawn, Bun.spawnSync and the shell free the writer (write fails: true)
         expect(received).toEqual(expected)   "created": 3,  - "freed": 3,  + "freed": 0
(pass) ... > Bun.spawn, Bun.spawnSync and the shell free the writer (write fails: false)
(pass) ... > bun install frees the security scanner's JSON writer (write fails: true)    (the scanner was balanced before, its release did not depend on the token)
(pass) ... > bun install frees the security scanner's JSON writer (write fails: false)

With only the start() hunk, the scanner's injected case fails instead (bun install never finishes). With both, 5/5 pass, and the trace of an injected Bun.spawn writer continues the one above with deref 1 -> 0 and deinit().

The new tests need a Windows debug build, so they are skipped on the lanes this PR's CI builds; what runs there is the existing coverage. Also run against the fixed build: on Windows x64, spawn.test.ts, spawnSync.test.ts, spawn-empty-arrayBufferOrBlob.test.ts, spawn-stdin-pipe-fd-leak.test.ts and spawn-streaming-stdin.test.ts (135 pass, 0 fail), bunshell.test.ts + bunshell-instance.test.ts + the shell leak.test.ts (458 pass; the 8 failures are 5 tilde-expansion tests that need HOME set and 3 tests that hit their timeouts when the three files run together and pass when run alone, none of them involving stdin), the two security-scanner suites (46/46) and child_process.test.ts + child-process-stdio.test.js (58 pass, 0 fail). On Linux: the shell suites, the scanner suites and, with BUN_FEATURE_FLAG_DISABLE_MEMFD=1 so that Bun.spawn reaches the writer, spawn.test.ts, all without failures; the source lints pass. cargo check --workspace is clean for the four Linux targets and macOS x64, and the touched crates check clean for macOS arm64, Windows x64/arm64, FreeBSD and both Android targets (Windows x64 was also built natively for the runs above).

Open PRs nearby: #37774 (POSIX error path), #37755 and #37799 edit on_close in the same file and compose with this (only a comment changes there here); #37879 converts the reader's start() to the same raw-pointer shape and touches the adjacent lines of js_bun_spawn_bindings.rs, so whichever of the two lands second needs a trivial rebase.


no test proof · iteration 1 · 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

…inside start()

StaticPipeWriter::start() takes a ref on the writer and records it in
`started` after the buffered writer has started. On Windows starting the
writer issues the uv_write, and when that fails synchronously the writer
closes itself inside the call: on_close runs before `started` is set, so
it releases nothing, and the owner's on_close_io drops create()'s ref and
empties its slot. start() then set `started` on a writer nothing could
reach any more, stranding its ref, so the writer was never freed.

start() now releases its own ref when it finds the writer closed
underneath it and reports Ok, the same outcome as a write that fails
asynchronously. That release frees the writer for Subprocess and
ShellSubprocess, so start() takes a raw pointer instead of `&mut self`.
SecurityScanSubprocess, which releases start()'s ref itself right after
start() returns, now claims it through the `started` token so it does
not release a ref start() already released; this also covers start()
returning Err, where it was releasing one ref too many.

The failing uv_write cannot be arranged from JS, so debug builds get a
fault-injection flag for it, plus create()/deinit() lines in the
StaticPipeWriter debug scope that the test counts for all three owners.
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

Pipe writer lifecycle

Layer / File(s) Summary
Writer failure injection and lifecycle
src/bun_core/env_var.rs, src/io/PipeWriter.rs, src/spawn/static_pipe_writer.rs
Adds a debug Windows write-failure flag. StaticPipeWriter now manages synchronous failures, references, and started state explicitly.
Startup caller ownership updates
src/runtime/api/bun/js_bun_spawn_bindings.rs, src/runtime/shell/subproc.rs, src/install/PackageManager/security_scanner.rs
Updates buffered stdin and security-scanner startup to use the raw-pointer start operation and conditional reference release.
Fault-injection and ownership tests
test/js/bun/spawn/buffer-stdin-owners-fixture.ts, test/js/bun/spawn/spawn-pipe-start-error.test.ts
Tests normal and injected writes for spawn, spawnSync, shell execution, and the security scanner. Tests check output, exit status, and lifecycle counts.

Possibly related PRs

  • oven-sh/bun#37669: Related subprocess pipe lifecycle changes in src/runtime/shell/subproc.rs.

Suggested reviewers: jarred-sumner, dylan-conway

🚥 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 summarizes the primary Windows buffer-stdin writer leak fix caused by synchronous uv_write failure.
Description check ✅ Passed The description explains the problem, fix, scope, and verification results, including platform-specific test limitations.

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

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced and fixed.

Reproduced on Windows x64 with a debug build: with the debug fault injection added in this PR making the buffer-stdin writer's uv_write fail synchronously, the unfixed build leaves every writer allocated (StaticPipeWriter ... create() x3, no deinit(); refcount stuck at 1 after onError), for Bun.spawn, Bun.spawnSync and the shell redirect alike. Fixed build frees all of them (3/3, security scanner 1/1); trace and the full list of suites run against the fixed build are in the description.

The new tests require a Windows debug build and are skipped elsewhere, so on this PR's CI lanes only the existing spawn/shell/scanner coverage exercises the change.

@claude claude Bot left a comment

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.

I reviewed this PR and didn't find any bugs. Because it reworks intrusive-refcount ownership across three StaticPipeWriter owners and changes start() to unsafe fn(*mut Self) — with the covering tests only running on Windows debug builds — a human look would still be worthwhile.

What was reviewed:

  • Refcount balance on the new Windows is_done() path in start() and across all three callers (Subprocess/JsCell, ShellSubprocess, SecurityScanSubprocess) — traced create/start/on_close/deref on both the sync-fail and normal paths.
  • The security-scanner started-token gating, including the pre-existing POSIX Err over-deref it also closes; writer_local's extra ref keeps *writer_ptr live for the token read.
  • The shell caller drops its &stdin borrow before start() (plain field), and the JS-bindings caller relies on JsCell's UnsafeCell shape the same way the adjacent Readable::Pipe start sites already do.
  • Fault injection is cfg(debug_assertions)-gated; buffer_writer_mut still has other callers so it isn't dead.
Extended reasoning...

Overview

This PR fixes a Windows-only leak of StaticPipeWriter (the buffer/blob-stdin writer used by Bun.spawn, Bun.spawnSync, the shell's < ${buf} redirect, and the security scanner's package-list pipe) when its uv_write fails synchronously inside start(). It touches src/spawn/static_pipe_writer.rs (core fix + signature change to unsafe fn start(this: *mut Self)), all three call sites in src/runtime/api/bun/js_bun_spawn_bindings.rs, src/runtime/shell/subproc.rs, and src/install/PackageManager/security_scanner.rs, plus a debug-only fault-injection hook in src/io/PipeWriter.rs, a feature-flag entry in src/bun_core/env_var.rs, and new debug-scope create()/deinit() log lines. Tests are added to test/js/bun/spawn/spawn-pipe-start-error.test.ts (Windows debug only) with a shared fixture.

Security risks

None identified. The fault injection is compiled out of release builds via #[cfg(debug_assertions)], and the env-var flag lives alongside the existing BUN_INTERNAL_FAIL_PIPE_READER_START sibling. No new user-facing surface, no untrusted-input parsing, no auth/crypto paths.

Level of scrutiny

High. This is native-code intrusive refcounting — REVIEW.md's most-blocked category — spread across three separate owners with three different lifetime shapes (JsCell, plain field, RefPtr with a local dupe_ref). The fix also converts start() from &mut self to a raw-pointer unsafe fn, which is the right shape (the callee may free *this) but every caller now has a new safety obligation. The PR description's refcount traces are exemplary and match my own walk-through of every terminal path (Ok/Err × sync-fail/normal × each owner), and the security-scanner hunk closes an adjacent pre-existing over-deref on the POSIX Err arm. Still, this is exactly the class of change where a maintainer's independent trace is worth the time.

Other factors

The new tests are gated to Windows debug builds and are skipped on this PR's CI lanes per the description, so CI here only exercises the change through pre-existing spawn/shell/scanner suites. The description reports manual runs of those suites on Windows x64 and Linux, plus cargo check across targets. The js_bun_spawn_bindings.rs call site holds a &RefPtr from JsCell::get() across the re-entrant start(); this is the file's established pattern (the adjacent Readable::Pipe start/read_all blocks do the same) and JsCell is UnsafeCell-backed to make that structurally sound, but it is worth a maintainer's eye given the shell caller was rewritten to explicitly drop its borrow first. There are also several nearby open PRs (#37774, #37755, #37799, #37879) touching adjacent code in the same file, so merge order will need a light rebase either way.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:05 PM PT - Aug 12th, 2026

@robobun, your commit 6ebbe5a is building: #93416

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 8:05 AM PT - Aug 13th, 2026

@robobun, your commit 2ffd7c5 has some failures in Build #94409 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 37898

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

bun-37898 --bun

Comment thread src/bun_core/env_var.rs
Comment thread src/install/PackageManager/security_scanner.rs
Comment thread src/install/PackageManager/security_scanner.rs
Comment thread src/io/PipeWriter.rs Outdated
Comment thread src/spawn/static_pipe_writer.rs
Comment thread src/spawn/static_pipe_writer.rs
Comment thread src/spawn/static_pipe_writer.rs

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/spawn/static_pipe_writer.rs`:
- Line 210: Add a SAFETY comment immediately before the unsafe call to
writer.start in the surrounding match, explaining that the intrusive reference
acquired above guarantees the dereferenced writer remains valid for this
operation.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1697b9ef-6d06-4815-bd31-e80cc8d3264d

📥 Commits

Reviewing files that changed from the base of the PR and between 74c2457 and 0dc8dd4.

📒 Files selected for processing (8)
  • src/bun_core/env_var.rs
  • src/install/PackageManager/security_scanner.rs
  • src/io/PipeWriter.rs
  • src/runtime/api/bun/js_bun_spawn_bindings.rs
  • src/runtime/shell/subproc.rs
  • src/spawn/static_pipe_writer.rs
  • test/js/bun/spawn/buffer-stdin-owners-fixture.ts
  • test/js/bun/spawn/spawn-pipe-start-error.test.ts

Comment thread src/spawn/static_pipe_writer.rs

@claude claude Bot left a comment

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.

I reviewed this PR and didn't find any bugs. Because it reworks intrusive-refcount lifetime handling in StaticPipeWriter::start() across three owners (with a &mut self*mut Self signature change and a new release site whose test coverage only runs on Windows debug builds), a human look would still be worthwhile. The comment-cop flags on the SAFETY / doc comments also want a human call — they read as ownership-invariant documentation to me, not workaround justification.

Checked: refcount balance on every terminal path of the new Windows start() (Err, sync-close via is_done(), success) for each owner — is_done is set synchronously by BaseWindowsPipeWriter::close() before start_with_current_pipe() returns, and start()'s own ref keeps *this live for the read. Also checked that the security scanner's new token-gated deref fixes a pre-existing over-deref on the POSIX Err path (the errdefer guard would have deref'd a freed writer), and that none of the three updated callers touches the writer after start() returns.

Extended reasoning...

Overview

Fixes a Windows-only leak of StaticPipeWriter (the buffer/Blob-stdin pump) when its uv_write fails synchronously inside start(). The fix reshapes start() to take *mut Self, adds an is_done() check on Windows that releases start()'s own ref when the writer closed underneath it, updates all three callers (js_bun_spawn_bindings.rs, shell/subproc.rs, security_scanner.rs), gates the security scanner's post-start deref on the started token, adds a debug-only BUN_INTERNAL_FAIL_PIPE_WRITER_WRITE fault-injection flag in PipeWriter.rs, and adds create()/deinit() debug-scope lines plus a Windows-debug-only test that counts them across all three owners.

Security risks

None. The env-var fault injection is under cfg(debug_assertions) and only synthesizes an EPIPE; it does not alter release builds.

Level of scrutiny

High. This is intrusive-refcount lifetime code with re-entrant close callbacks — REVIEW.md's most-blocked category. I traced the refcount on every path: on Windows sync-fail, start_with_current_pipe() sets is_done=false then write() → sync error → close() (sets is_done=true, fires on_close → owner's on_close_io drops create()'s ref) → parent_on_error() → return Ok. Back in start(), is_done() is true, so start() releases its own +1 (which is the last ref for Subprocess/ShellSubprocess; the scanner's writer_local keeps it alive there). started stays false, and every other release site (on_close, on_write, take_pending_start_writer, the scanner's post-start block) gates on started, so exactly one site releases start()'s ref on every path. The scanner change also fixes a latent over-deref on the POSIX Err path where the unconditional deref plus writer_local.deref() plus the errdefer guard summed to one too many.

Other factors

The new tests are Windows-debug-only and skipped on this PR's CI lanes, so CI exercises only the existing spawn/shell/scanner suites against the changed callers. The PR description documents manual runs of those suites on Windows and Linux. The &mut self → raw-pointer signature change is a public unsafe API change (matching the sibling reader conversion in #37879). The comment-cop bot flagged six comments; they look like ownership-invariant docs (the start() doc comment, the on_close release-site note) rather than workaround excuses, but that plus the refcount subtlety are enough that a human should sign off.

Comment thread src/io/PipeWriter.rs

@claude claude Bot left a comment

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.

I reviewed this PR and didn't find any bugs. Because it reworks intrusive refcount balance for StaticPipeWriter::start() across three owners with Windows-only re-entrancy, and the new tests are skipped on this PR's CI lanes, a human look would still be worthwhile.

What was reviewed:

  • Traced the start() ref on every path (Ok/Err/is_done) for all three owners; each releases exactly once via the started token.
  • Checked that no caller touches the writer after start() may have freed it; the shell caller correctly ends its stdin borrow before the call.
  • Confirmed the security-scanner hunk now guards its release on started, so start()'s own release on sync failure/Err isn't doubled.
  • The injected_write_error() path is cfg(debug_assertions)-gated and the flag is stripped from child env in the fixture.
Extended reasoning...

Overview

This PR fixes a Windows-only leak of StaticPipeWriter (the object that pumps a Buffer/Blob stdin into a child) when its uv_write fails synchronously inside start(). The fix converts StaticPipeWriter::start from &mut self to unsafe fn(this: *mut Self), adds an is_done() check on Windows after starting the buffered writer to release start()'s own ref when the writer already closed underneath it, and updates all three callers (Subprocess, ShellSubprocess, SecurityScanSubprocess). The security scanner is additionally changed to claim start()'s ref through the started token rather than unconditionally, which also fixes a latent over-release on the pre-existing Err path. Debug-only fault injection (BUN_INTERNAL_FAIL_PIPE_WRITER_WRITE) and create()/deinit() scoped-log lines are added so a Windows-debug-only test can count writers across all owners.

Security risks

None identified. The change is a resource-lifetime fix in the spawn/pipe layer; no user input parsing, auth, crypto, or trust boundaries are touched. The new env flag is compiled only under debug_assertions.

Level of scrutiny

High. This is exactly the memory-safety category the repo's review guidance calls out as most-blocked: intrusive refcount balance where a callback (on_close → owner's on_close_io) fires re-entrantly inside start() and may free the object. The invariant — on every path exactly one site releases start()'s +1, and started records which — has to hold across three owners, two platforms, and both the sync-failure and normal-close paths. The reasoning in the PR description and code comments traces this convincingly, and the bug-hunting pass found nothing, but the correctness argument is subtle enough that a human should confirm it, particularly the interaction with take_pending_start_writer on Subprocess and the on_write/on_close release sites that also read started.

Other factors

  • The new tests require a Windows debug build and are describe.skipIf'd elsewhere; per the description, only the existing spawn/shell/scanner suites exercise this on the PR's CI lanes. So CI green does not directly validate the fixing lines.
  • The js_bun_spawn_bindings.rs caller keeps the if let Writable::Buffer(buffer) pattern-binding live across start(), whereas the shell caller was restructured to end the stdin borrow first. Under strict aliasing this is a pre-existing pattern in the codebase and the raw-pointer form is strictly better than the old &mut self, but it's worth a human glance.
  • There are outstanding automated comment-cop github-actions comments about long comments on several hunks. These look like linter noise on legitimate SAFETY / refcount-invariant comments rather than substantive feedback, but they haven't been resolved.
  • The CodeRabbit clippy finding (missing SAFETY comment) was addressed in 44eb98e.

@claude claude Bot left a comment

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.

I reviewed this PR and didn't find any bugs. Because it reworks intrusive-refcount ownership across three owners (Subprocess, ShellSubprocess, SecurityScanSubprocess) and converts start() to an unsafe fn(*mut Self) whose caller contract now includes "may free *this", a human look at the refcount balance would still be worthwhile — especially since the new tests only run on Windows debug lanes and are skipped on this PR's CI.

What was reviewed:

  • Traced the Windows sync-failure path (start_with_current_pipewriteclose sets is_doneon_close/on_close_io drops owner's ref → start() sees is_done() and releases its own +1) for all three owners; each ends at zero.
  • Checked the security-scanner hunk against the new start(): gating the deref on the started token is required (with only the start() hunk it would double-release on sync failure); writer_local keeps the writer live across the token read.
  • Verified the POSIX arm is behaviour-preserving (only registers the poll; owner's ref survives) and that injected_write_error() is dead in release builds.
Extended reasoning...

Overview

This PR fixes a Windows-only leak of StaticPipeWriter (the buffer/blob-stdin pump for Bun.spawn, the shell's < ${buffer} redirect, and the install security scanner) when uv_write fails synchronously inside start(). The core change converts StaticPipeWriter::start from &mut self to unsafe fn(this: *mut Self) and adds an is_done() check after start_with_current_pipe() so start() releases its own +1 when the writer closed underneath it. All three callers are updated to pass the raw pointer and not touch it afterwards; the security scanner additionally gates its post-start() deref on the started token so it doesn't double-release. Debug-only fault injection (BUN_INTERNAL_FAIL_PIPE_WRITER_WRITE) and create()/deinit() scoped-log lines are added to make the leak testable.

Security risks

None identified. The change is entirely internal refcount bookkeeping on a stdin writer; no user-controlled input reaches new parsing or validation. The env-var flag is cfg(debug_assertions)-gated and the injection helper compiles to None in release.

Level of scrutiny

High. This is exactly the class REVIEW.md's memory-safety section is about: intrusive refcounts that must balance on every terminal path (success, sync failure, async failure, Err from the buffered writer), across three independently-owned callers, with an unsafe fn whose contract now includes "the writer may be freed when this returns". The reasoning is careful and the PR description traces each path, but it's subtle enough (e.g. the security scanner was already balanced before and needed the token gate only because of the start() hunk) that a maintainer who owns this code should confirm it.

Other factors

  • The new tests are skipIf(!isWindows || !isDebug), so this PR's own CI does not run them; verification relies on the author's manual Windows-debug run reported in the description and on the existing spawn/shell/scanner suites for regressions.
  • All prior review-thread items (comment-cop, CodeRabbit's missing SAFETY comment) are resolved and addressed in commit 44eb98e.
  • The js_bun_spawn_bindings.rs caller reads the slot pointer via buffer.as_ptr() before start() (which may overwrite the slot through on_close_io); the shell caller splits this explicitly. Both look sound under NLL + JsCell's UnsafeCell semantics, and neither touches the pointer after the call.
  • The PR notes overlap with #37879 (reader's equivalent raw-pointer conversion) on adjacent lines of js_bun_spawn_bindings.rs; whichever lands second needs a small rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants