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

RO
2mo ago
+263-58

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 #35297 (the same write failing asynchronously) and #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).

  • Rebase notes: #37774 made on_close release the started token on every platform; the resolution keeps that unconditional release (a write failing synchronously inside start() reaches on_close before the token is set, so start() still releases its own ref). #40478 replaced IntrusiveRc with a RefPtr that releases on Drop; the scanner call site now relies on that Drop on its error path instead of a manual deref, and its token-gated claim replaces main's unconditional post-start release. Re-verified after each rebase: 5/5 on Windows debug, #37774's leak test and the 43 scanner provider tests green on Linux.

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 #36571 and #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 #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 2 · 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