sink: detach JSSink controller when assignToStream throws

[robobun]

What

JSSink::assign_to_stream now detaches the freshly created JSReadable*SinkController (nulling its m_sinkPtr) when the C++ stream-pump setup returns an error, before returning to the caller.

Why

The generated ${name}__assignToStream functions create the controller with m_sinkPtr = sinkPtr and then call into GlobalObject::assignToStream → readDirectStream / readStreamIntoSink. If that setup throws (for example a direct ReadableStream whose pull getter throws), the controller is never started, so nothing ever calls end()/close() to null m_sinkPtr. The caller's error path (Writable::init for Bun.spawn) then releases and frees the native sink. When the controller is later swept, its destructor runs ${name}__controllerDetached / ${name}__finalize on freed memory.

ASAN report:

heap-use-after-free on address 0x799feed81c78
READ of size 1
  #0 JSSink<FileSink>::js_controller_detached  Sink.rs:567
  #1 FileSink__controllerDetached              generated_jssink.rs:179
  #2 JSReadableFileSinkController::~JSReadableFileSinkController()

freed by:
  #12 FileSink::deinit                         FileSink.rs:1142
  #16 Writable::pipe_release                   Writable.rs:70
  #17 Writable::init                           Writable.rs:339
  #18 spawn_maybe_sync                         js_bun_spawn_bindings.rs:1379

The fix is at the generic JSSink::assign_to_stream layer so it covers every sink type (FileSink, NetworkSink, FetchRequestBodySink, ...), not just the spawn path.

Repro

const { openSync, closeSync } = require("node:fs");
const fd = openSync("/tmp/out.txt", "w");
let armed = false;
const stream = new ReadableStream({
  type: "direct",
  get pull() { if (armed) throw new Error("pull unavailable"); return () => {}; },
});
armed = true;
try {
  Bun.spawn({ cmd: [process.execPath, "-e", "0"], stdio: [stream, fd, "ignore"] });
} catch {}
closeSync(fd);
Bun.gc(true);   // sweep -> controller dtor -> UAF

Tests

The two existing spawn.test.ts cases that cover the stdin-stream-setup-throws path now force a full GC in the child fixture so the controller destructor runs deterministically under debug+ASAN as well. Previously they were only failing on the release-asan lane (where the whole file has been quarantined as [ASAN] [TIMEOUT]), which is why this went unnoticed.

bun bd test test/js/bun/spawn/spawn.test.ts -t "stdin stream setup fails"

fails on main (ASAN heap-use-after-free in the child's stderr) and passes with this change. spawn-stdin-readable-stream-edge-cases.test.ts and body-stream.test.ts continue to pass.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 97900a9c-bbb7-4767-858e-549139bce5c9

📥 Commits

Reviewing files that changed from the base of the PR and between fdbaf06 and 35a4dea.

📒 Files selected for processing (2)

• src/runtime/webcore/Sink.rs
• test/js/bun/spawn/spawn.test.ts

---

Walkthrough

JSSink stream assignment now clears source state and detaches the controller after failure. Spawn tests trigger garbage collection in two stdin failure fixtures and verify empty stderr in one fixture.

Changes

JSSink cleanup and regression coverage

Layer / File(s)	Summary
Handle controller assignment failures src/runtime/webcore/Sink.rs	JSSink::assign_to_stream clears the source and detaches the controller pointer when assignment fails.
Exercise stdin setup failures test/js/bun/spawn/spawn.test.ts	The stdin failure fixtures trigger garbage collection. One fixture asserts that stderr is empty.

Possibly related PRs

• oven-sh/bun#36087: Introduces the JSSink-based stream-assignment machinery hardened by this change.
• oven-sh/bun#36703: Updates readable-stream sink and controller cleanup during failure paths.
• oven-sh/bun#36779: Addresses stream controller and source lifetime cleanup during assignment failure.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary fix: detaching the JSSink controller when assignToStream throws.
Description check	✅ Passed	The description explains the fix, cause, impact, reproduction case, and verification steps, although it uses different headings than the template.
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.

---

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

[robobun]

Reproduced under debug+ASAN with the repro in the PR body (the Bun.gc(true) makes the controller destructor run deterministically). Verified:

• bun bd test test/js/bun/spawn/spawn.test.ts -t "stdin stream setup fails": 2 fail on main (ASAN heap-use-after-free in child stderr), 2 pass with this change
• bun bd test test/js/bun/spawn/spawn-stdin-readable-stream-edge-cases.test.ts: 12 pass
• bun bd test test/js/web/fetch/body-stream.test.ts: 9086 pass
• cargo check -p bun_runtime --target x86_64-pc-windows-msvc: clean

Merged. One note from self-review: the two amended tests live in spawn.test.ts, which is currently [ ASAN ] [ TIMEOUT ] quarantined in test/expectations.txt, so the ASAN lane never runs them in CI. Left a note on #36780 (the active expectations.txt prune) so that PR can decide whether to un-quarantine the file or move those two tests into spawn-stdin-readable-stream-edge-cases.test.ts for ASAN coverage.

[claude]

No issues found — the detach-on-error path mirrors the existing JSSink::detach pattern and controller->detach() is idempotent, so callers whose error handlers already tear down the sink aren't affected. Deferring to a human because this is a memory-safety change in the shared assign_to_stream path hit by every sink type (FileSink, NetworkSink, HTTPServerWritable, FetchRequestBodySink, ArrayBufferSink).

What was reviewed:

• Traced the error return through ${name}__assignToStream → GlobalObject::assignToStream: failures come back as an encoded JSC::Exception cell, which JSValue::to_error() recognizes (bindings.cpp:4742), so non-Error throws are also caught.
• Verified JSSinkController__detachPtr → detach() uses std::exchange(m_sinkPtr, nullptr) and the controller's m_onClose/m_onDestroy are both unset at this point (created with 0, never start()ed), so no user JS re-enters during the new detach.
• Checked the other assign_to_stream callers (s3/client.rs, Blob.rs, Writable.rs) — their error branches match on the same .to_error() and any subsequent detach is a no-op against m_sinkPtr == nullptr / SourceHandle::None.

Extended reasoning...

Overview

The PR adds an early-return branch to the generic JSSink<T>::assign_to_stream (src/runtime/webcore/Sink.rs) that fires when the C++ ${name}__assignToStream created a controller (bits != 0) but the stream-pump setup returned an error. In that case it clears the sink's SourceHandle and calls JSSinkController__detachPtr on the just-created controller so its m_sinkPtr is nulled before the caller frees the native sink. Two existing spawn tests are hardened with Bun.gc(true) and an expect(stderr).toBe("") so the ASAN heap-use-after-free surfaces deterministically in debug+ASAN.

Security risks

None. This is a use-after-free fix; no auth/crypto/permissions/parsing surface is touched.

Level of scrutiny

High. Sink.rs is the shared JSSink glue that every sink type (FileSink, NetworkSink, the three HTTPServerWritable variants, FetchRequestBodySink, ArrayBufferSink) routes through, and the fix sits at the JSC/GC boundary where the destructor runs ${name}__finalize(m_sinkPtr). Per the repo's review guidance, memory-safety changes in shared native paths warrant a human look even when the mechanism reads correctly.

Other factors

• The new code is a near-verbatim copy of the existing JSSink::detach JSController arm (same call_check_slow + controller_abi::detach_ptr sequence), so it follows an established pattern.
• controller->detach() (generate-jssink.ts:657) uses std::exchange and null-guards every effect: at this point m_onDestroy == 0 (passed as 0 in ${controller}::create), m_onClose/m_weakReadableStream are unset (start() was never reached), and ${name}__controllerDetached sees SourceHandle::None (cleared just before) so it no-ops. No user JS runs, no double-free.
• to_error() matches JSC::Exception cells (bindings.cpp:4741-4745), which is exactly what GlobalObject::assignToStream returns on failure (ZigGlobalObject.cpp:3029-3032), so the guard is not sensitive to what value was thrown.
• The other error-path callers (s3/client.rs:1092, Writable.rs:228/336, Blob.rs) branch on the same .to_error(); the new detach makes their subsequent teardown a safe no-op rather than changing observable behavior.
• The test change adds Bun.gc(true) inside the child fixture and asserts empty stderr, converting a release-ASAN-only quarantine failure into a deterministic debug+ASAN regression test. The PR body reports it fails on main and passes with the fix.

I did not find anything wrong; deferring only because of the criticality of the shared code path.