diff --git a/src/codegen/generate-jssink.ts b/src/codegen/generate-jssink.ts index 9d1db91b9894..5b5549c827f1 100644 --- a/src/codegen/generate-jssink.ts +++ b/src/codegen/generate-jssink.ts @@ -526,8 +526,15 @@ 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} 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); + } + ${name}__finalize(ptr); + RETURN_IF_EXCEPTION(scope, {}); return JSC::JSValue::encode(JSC::jsUndefined()); } @@ -1134,8 +1141,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 76b8f29ba56a..972aeb5caa16 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 @@ -960,9 +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`). - self.readable_stream.set(readable_stream::Strong::default()); - self.pending.set(streams::WritablePending::default()); + // `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`. 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();