From eef1a32f075f6cc26444a3d25fcfe179403854b2 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 2 Aug 2026 14:16:37 +0000 Subject: [PATCH 1/3] JSSink: release native backing in prototype close() The generated ${name}__doClose (ArrayBufferSink.close(), FileSink.close(), etc.) nulls m_sinkPtr via detach() and then calls ${name}__close, which only runs end(None). Because m_sinkPtr is null, the later ~JS${name} destructor skips ${name}__finalize, so the wrapper's ownership of the native backing was never released on this path: ArrayBufferSink leaked its Box and buffer outright, and FileSink/NetworkSink leaked the wrapper's +1 intrusive ref. __doClose now fires the destroy callback (so Subprocess can clear its weak stdin back-pointer before the sink can be freed) and then calls __finalize, mirroring the destructor order. FileSink::finalize no longer clears pending/readable_stream: now that it is reachable synchronously from close(), tearing those down would strand a backpressured write() promise or an in-flight ReadableStream stdin. Both are released by deinit() (Box drop) once the keep-alive and assignToStream refs are gone. --- src/codegen/generate-jssink.ts | 12 ++++- src/runtime/webcore/FileSink.rs | 12 ++++- test/js/bun/util/arraybuffersink.test.ts | 58 +++++++++++++++++++++++- test/js/bun/util/filesink.test.ts | 56 ++++++++++++++++++----- 4 files changed, 122 insertions(+), 16 deletions(-) diff --git a/src/codegen/generate-jssink.ts b/src/codegen/generate-jssink.ts index 9d1db91b9894..75fb60351e36 100644 --- a/src/codegen/generate-jssink.ts +++ b/src/codegen/generate-jssink.ts @@ -526,8 +526,18 @@ JSC_DEFINE_HOST_FUNCTION(${name}__doClose, (JSC::JSGlobalObject * lexicalGlobalO } sink->detach(); - RETURN_IF_EXCEPTION(scope, {}); ${name}__close(lexicalGlobalObject, ptr); + // detach() nulled m_sinkPtr, so ~${className} will not run __finalize for + // this ptr. Release the wrapper's ownership here (unconditionally: even if + // __close set an exception) so the native backing is freed rather than + // leaked. Fire the destroy callback first, same as the destructor does: + // Subprocess holds a weak back-pointer that must be cleared before + // __finalize can drop the last ref on the sink. + if (auto destroy = std::exchange(sink->m_onDestroy, 0)) { + Bun__onSinkDestroyed(destroy, ptr); + } + ${name}__finalize(ptr); + RETURN_IF_EXCEPTION(scope, {}); return JSC::JSValue::encode(JSC::jsUndefined()); } diff --git a/src/runtime/webcore/FileSink.rs b/src/runtime/webcore/FileSink.rs index 76b8f29ba56a..bf242e1ebc8a 100644 --- a/src/runtime/webcore/FileSink.rs +++ b/src/runtime/webcore/FileSink.rs @@ -961,8 +961,16 @@ impl FileSink { // `ref_()` there. Callers that allocate via `init`/`create` and then // `to_js()` must `deref()` once to release init's +1 (see // `Blob::get_writer`). - self.readable_stream.set(readable_stream::Strong::default()); - self.pending.set(streams::WritablePending::default()); + // + // `finalize` is reachable from the prototype `.close()` as well as the + // C++ destructor (see `${name}__doClose` in generate-jssink.ts), so it + // must not tear down state that in-flight IO still needs: `pending` + // may hold a backpressured `write()` promise that `run_pending` will + // settle once the writer drains, and `readable_stream` may still be + // driving this sink as a spawn stdin. Both are released by `deinit` + // (Box drop) once `must_be_kept_alive_until_eof` and the + // assignToStream ref are gone. `js_sink_ref` roots the wrapper itself, + // so it is safe (and necessary) to release here. self.js_sink_ref.with_mut(|r| r.deinit()); // SAFETY: `&mut self` carries write provenance over the whole // allocation; this is the last use of `self` in `finalize`. diff --git a/test/js/bun/util/arraybuffersink.test.ts b/test/js/bun/util/arraybuffersink.test.ts index aa9c58e6aafc..40d9e2d55acd 100644 --- a/test/js/bun/util/arraybuffersink.test.ts +++ b/test/js/bun/util/arraybuffersink.test.ts @@ -1,6 +1,7 @@ import { ArrayBufferSink } from "bun"; import { describe, expect, it } from "bun:test"; -import { bunEnv, bunExe, withoutAggressiveGC } from "harness"; +import { bunEnv, bunExe, isASAN, withoutAggressiveGC } from "harness"; +import { join } from "node:path"; describe("ArrayBufferSink", () => { const fixtures = [ @@ -99,4 +100,59 @@ describe("ArrayBufferSink", () => { expect(JSON.parse(stdout)).toEqual({ out: "hello" }); expect({ exitCode, signalCode: proc.signalCode }).toEqual({ exitCode: 0, signalCode: null }); }); + + // The generated ${name}__doClose detached the JS wrapper (nulling m_sinkPtr) + // and then called __close, but never __finalize. The wrapper's destructor + // skips __finalize when m_sinkPtr is null, so every close() leaked the boxed + // ArrayBufferSink plus its Vec buffer. The repro runs off a setImmediate + // so the allocation stack does not fall under the module-loader suppression. + // LSAN symbolization of the leak stacks can take several seconds on its own, + // hence the explicit per-test timeout. + it.skipIf(!isASAN)( + "close() does not leak the native sink (LSAN)", + async () => { + const src = ` + await new Promise(resolve => setImmediate(resolve)); + for (let i = 0; i < 4; i++) { + const s = new Bun.ArrayBufferSink(); + s.start({ stream: true, asUint8Array: true }); + s.write(Buffer.alloc(4096, 0x61).toString()); + s.close(); + } + Bun.gc(true); + console.log("done"); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", src], + env: { + ...bunEnv, + ASAN_OPTIONS: "detect_leaks=1", + LSAN_OPTIONS: `suppressions=${join(import.meta.dirname, "../../../leaksan.supp")}`, + }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout.trim()).toBe("done"); + const summary = /SUMMARY: AddressSanitizer: (\d+) byte\(s\) leaked/.exec(stderr); + const leaked = summary ? Number(summary[1]) : 0; + // Before the fix each iteration leaked the ~48-byte struct and the + // 4 KiB write buffer (>16 KiB total for 4 iterations). + expect({ leaked, exitCode }).toEqual({ leaked: 0, exitCode: 0 }); + }, + 30_000, + ); + + it("close() followed by further calls does not crash", () => { + const s = new ArrayBufferSink(); + s.write("hello"); + s.close(); + // After close() the wrapper is detached; every method that needs the + // native backing throws the "already been closed" error rather than + // dereferencing a freed pointer. + expect(() => s.write("x")).toThrow(/already been closed/); + expect(() => s.flush()).toThrow(/already been closed/); + expect(() => s.end()).toThrow(/already been closed/); + expect(s.close()).toBeUndefined(); + }); }); diff --git a/test/js/bun/util/filesink.test.ts b/test/js/bun/util/filesink.test.ts index 3bc4ba4af2bb..a78a3329e9e1 100644 --- a/test/js/bun/util/filesink.test.ts +++ b/test/js/bun/util/filesink.test.ts @@ -323,12 +323,6 @@ it.skipIf(!isPosix)( // run_pending, so a backpressured write()'s promise was left pending forever // while close() threw. Now close() routes the error to that promise and // returns undefined. -// -// Runs in a subprocess because sink.close() on a Blob-created FileSink -// currently leaks the native FileSink (doClose detaches m_sinkPtr so the -// wrapper's +1 never reaches finalize); running it in-process would abort the -// whole file under detect_leaks=1. That leak is pre-existing on main and -// tracked separately. it.skipIf(!isPosix)( "close() after a backpressured write() with the reader gone rejects the write's promise with EPIPE", async () => { @@ -348,12 +342,7 @@ it.skipIf(!isPosix)( `; await using proc = Bun.spawn({ cmd: [bunExe(), "-e", src], - env: { - ...bunEnv, - // Pre-existing leak in sink.close() (see comment above); don't let the - // child's LSAN abort hide the actual assertion we're testing. - ASAN_OPTIONS: "allow_user_segv_handler=1:disable_coredump=0:detect_leaks=0", - }, + env: bunEnv, stderr: "pipe", }); const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); @@ -524,6 +513,49 @@ it.skipIf(!isPosix)("does not leak native FileSink when a pending write fails (E expect(fileSinkInternals.liveCount()).toBeLessThanOrEqual(baseline + 1); }); +// The generated ${name}__doClose detached m_sinkPtr and then called __close, +// so the wrapper's destructor skipped __finalize and the wrapper's +1 on the +// native FileSink was never released. +it("close() does not leak the native FileSink", async () => { + const dir = tmpdirSync(); + const baseline = fileSinkInternals.liveCount(); + const iterations = 8; + for (let i = 0; i < iterations; i++) { + const writer = Bun.file(join(dir, `close-leak-${i}.txt`)).writer(); + writer.write("hi"); + writer.close(); + } + for (let i = 0; i < 50; i++) { + Bun.gc(true); + if (fileSinkInternals.liveCount() <= baseline) break; + await Bun.sleep(10); + } + expect(fileSinkInternals.liveCount()).toBeLessThanOrEqual(baseline + 1); +}); + +// Now that __doClose runs finalize(), finalize() must not tear down state an +// in-flight write still needs: clearing `pending` here would drop the +// backpressure promise's Strong before on_write can settle it. +it.skipIf(isWindows)("close() while a write() promise is pending still settles it", async () => { + await using child = Bun.spawn({ + cmd: [bunExe(), "-e", "for await (const _ of process.stdin) {}"], + env: bunEnv, + stdin: "pipe", + stdout: "ignore", + stderr: "pipe", + }); + const writer = child.stdin; + // 4 MiB overflows the default pipe capacity on Linux/macOS so write() + // returns a promise. + const p = writer.write(Buffer.alloc(4 * 1024 * 1024, 0x61)); + expect(p).toBeInstanceOf(Promise); + writer.close(); + await expect(p).resolves.toBeGreaterThanOrEqual(0); + const [stderr, exitCode] = await Promise.all([child.stderr.text(), child.exited]); + if (exitCode !== 0) expect(stderr).toBe(""); + expect(exitCode).toBe(0); +}); + it("start() without path/fd on an already-open writer does not crash", async () => { const path = join(tmpdirSync(), "filesink-restart.txt"); const writer = Bun.file(path).writer(); From b704b9fffd2d7baf77a824bb03f0e2e50873fa9a Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 2 Aug 2026 15:10:28 +0000 Subject: [PATCH 2/3] doc: finalize() call sites now include __doClose Update the header comments on FileSink::finalize, ArrayBufferSink::finalize and the generated __finalize thunk to list both call sites (destructor lazy sweep and the prototype close() path) so the governing constraint reads correctly up front. --- src/codegen/generate-jssink.ts | 5 +++-- src/runtime/webcore/ArrayBufferSink.rs | 4 +++- src/runtime/webcore/FileSink.rs | 18 ++++++++---------- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/codegen/generate-jssink.ts b/src/codegen/generate-jssink.ts index 75fb60351e36..ee2a5d5f1e97 100644 --- a/src/codegen/generate-jssink.ts +++ b/src/codegen/generate-jssink.ts @@ -1144,8 +1144,9 @@ pub extern "C" fn ${name}__memoryCost(this: &${name}) -> usize { `; - // ZIG_DECL void ${name}__finalize(void* sinkPtr) — called from JS${name}::~JS${name}. - // C++ caller null-checks `m_sinkPtr` before calling. + // ZIG_DECL void ${name}__finalize(void* sinkPtr) — called from + // JS${name}::~JS${name} and from ${name}__doClose. C++ caller null-checks + // `m_sinkPtr` / `ptr` before calling. symbols.push(`${name}__finalize`); templ += `#[allow(dead_code, unreachable_pub, unused)] #[unsafe(no_mangle)] diff --git a/src/runtime/webcore/ArrayBufferSink.rs b/src/runtime/webcore/ArrayBufferSink.rs index 12c067dd9a97..1eafec641957 100644 --- a/src/runtime/webcore/ArrayBufferSink.rs +++ b/src/runtime/webcore/ArrayBufferSink.rs @@ -73,7 +73,9 @@ impl ArrayBufferSink { // `Box` contract applies only to generate-classes.ts classes. /// # Safety /// `this` must be the m_ctx payload allocated via `heap::alloc` in - /// init/JSSink, called from JSC lazy sweep on the mutator thread. + /// init/JSSink. Called from (a) `~JSArrayBufferSink` during lazy sweep + /// and (b) synchronously from `${name}__doClose` (prototype `.close()`), + /// on the mutator thread in both cases. // Forwards `this` to `destroy` without dereferencing it here; // not_unsafe_ptr_arg_deref is a false positive on this forwarding wrapper. #[allow(clippy::not_unsafe_ptr_arg_deref)] diff --git a/src/runtime/webcore/FileSink.rs b/src/runtime/webcore/FileSink.rs index bf242e1ebc8a..569135c1daed 100644 --- a/src/runtime/webcore/FileSink.rs +++ b/src/runtime/webcore/FileSink.rs @@ -925,8 +925,10 @@ impl FileSink { } pub fn finalize(&mut self) { - // `.classes.ts` finalize — see PORTING.md §JSC. Runs during lazy sweep; - // must not touch live JS cells. + // Called from (a) `~JSFileSink` during lazy sweep, and (b) synchronously + // from `${name}__doClose` (prototype `.close()`). Must satisfy both + // contexts: no touching live JS cells (sweep), and no tearing down + // state that in-flight IO still needs (close). // Shutdown never unwinds the writer: the loop stops ticking, so the // `onWrite`/`onClose`/EOF callbacks that balance these refs can no @@ -962,15 +964,11 @@ impl FileSink { // `to_js()` must `deref()` once to release init's +1 (see // `Blob::get_writer`). // - // `finalize` is reachable from the prototype `.close()` as well as the - // C++ destructor (see `${name}__doClose` in generate-jssink.ts), so it - // must not tear down state that in-flight IO still needs: `pending` - // may hold a backpressured `write()` promise that `run_pending` will - // settle once the writer drains, and `readable_stream` may still be - // driving this sink as a spawn stdin. Both are released by `deinit` - // (Box drop) once `must_be_kept_alive_until_eof` and the + // `pending` (a backpressured `write()` promise) and `readable_stream` + // (spawn-stdin source) are left in place: both are released by + // `deinit` (Box drop) once `must_be_kept_alive_until_eof` and the // assignToStream ref are gone. `js_sink_ref` roots the wrapper itself, - // so it is safe (and necessary) to release here. + // so it is released here. self.js_sink_ref.with_mut(|r| r.deinit()); // SAFETY: `&mut self` carries write provenance over the whole // allocation; this is the last use of `self` in `finalize`. From ebb4bce28b6bf784520daf3bde30ba4e596b7a2a Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 2 Aug 2026 15:12:50 +0000 Subject: [PATCH 3/3] Tighten finalize()/doClose comments --- src/codegen/generate-jssink.ts | 9 +++------ src/runtime/webcore/FileSink.rs | 9 ++------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/src/codegen/generate-jssink.ts b/src/codegen/generate-jssink.ts index ee2a5d5f1e97..5b5549c827f1 100644 --- a/src/codegen/generate-jssink.ts +++ b/src/codegen/generate-jssink.ts @@ -527,12 +527,9 @@ JSC_DEFINE_HOST_FUNCTION(${name}__doClose, (JSC::JSGlobalObject * lexicalGlobalO sink->detach(); ${name}__close(lexicalGlobalObject, ptr); - // detach() nulled m_sinkPtr, so ~${className} will not run __finalize for - // this ptr. Release the wrapper's ownership here (unconditionally: even if - // __close set an exception) so the native backing is freed rather than - // leaked. Fire the destroy callback first, same as the destructor does: - // Subprocess holds a weak back-pointer that must be cleared before - // __finalize can drop the last ref on the sink. + // detach() nulled m_sinkPtr so ~${className} won't finalize ptr; do the + // destructor's teardown (onDestroy first so Subprocess clears its weak + // back-pointer, then __finalize) here instead, even if __close threw. if (auto destroy = std::exchange(sink->m_onDestroy, 0)) { Bun__onSinkDestroyed(destroy, ptr); } diff --git a/src/runtime/webcore/FileSink.rs b/src/runtime/webcore/FileSink.rs index 569135c1daed..972aeb5caa16 100644 --- a/src/runtime/webcore/FileSink.rs +++ b/src/runtime/webcore/FileSink.rs @@ -962,13 +962,8 @@ impl FileSink { // belongs to the wrapper it's about to be stored in, so no extra // `ref_()` there. Callers that allocate via `init`/`create` and then // `to_js()` must `deref()` once to release init's +1 (see - // `Blob::get_writer`). - // - // `pending` (a backpressured `write()` promise) and `readable_stream` - // (spawn-stdin source) are left in place: both are released by - // `deinit` (Box drop) once `must_be_kept_alive_until_eof` and the - // assignToStream ref are gone. `js_sink_ref` roots the wrapper itself, - // so it is released here. + // `Blob::get_writer`). `pending`/`readable_stream` are left for + // `deinit` (Box drop) since in-flight IO may still need them. self.js_sink_ref.with_mut(|r| r.deinit()); // SAFETY: `&mut self` carries write provenance over the whole // allocation; this is the last use of `self` in `finalize`.