diff --git a/src/jsc/FetchHeaders.rs b/src/jsc/FetchHeaders.rs index 2bec550f166b..0726b61e9544 100644 --- a/src/jsc/FetchHeaders.rs +++ b/src/jsc/FetchHeaders.rs @@ -144,9 +144,16 @@ impl FetchHeaders { global: &JSGlobalObject, value: JSValue, ) -> JsResult>> { - host_fn::from_js_host_call_generic(global, || { - NonNull::new(WebCore__FetchHeaders__createFromJS(global, value)) - }) + host_fn::from_js_host_call_owned( + global, + || NonNull::new(WebCore__FetchHeaders__createFromJS(global, value)), + // SAFETY: `p` came from `createFromJS` above and has not been deref'd. + |p| { + if let Some(mut p) = p { + unsafe { p.as_mut() }.deref() + } + }, + ) } pub fn put_default( @@ -327,9 +334,16 @@ impl FetchHeaders { &mut self, global: &JSGlobalObject, ) -> JsResult>> { - host_fn::from_js_host_call_generic(global, || { - NonNull::new(WebCore__FetchHeaders__cloneThis(self, global)) - }) + host_fn::from_js_host_call_owned( + global, + || NonNull::new(WebCore__FetchHeaders__cloneThis(self, global)), + // SAFETY: `p` came from `cloneThis` above and has not been deref'd. + |p| { + if let Some(mut p) = p { + unsafe { p.as_mut() }.deref() + } + }, + ) } pub fn deref(&mut self) { diff --git a/src/jsc/JSBigInt.rs b/src/jsc/JSBigInt.rs index bc614d35ab00..f1d174451a5c 100644 --- a/src/jsc/JSBigInt.rs +++ b/src/jsc/JSBigInt.rs @@ -73,6 +73,10 @@ impl JSBigInt { } pub fn to_string(&self, global: &JSGlobalObject) -> JsResult { - crate::host_fn::from_js_host_call_generic(global, || JSC__JSBigInt__toString(self, global)) + crate::host_fn::from_js_host_call_owned( + global, + || JSC__JSBigInt__toString(self, global), + |s| s.deref(), + ) } } diff --git a/src/jsc/JSValue.rs b/src/jsc/JSValue.rs index 113234ea045b..f3794eb5f0d7 100644 --- a/src/jsc/JSValue.rs +++ b/src/jsc/JSValue.rs @@ -2715,9 +2715,16 @@ impl JSValue { if flags.for_storage { bits |= 1 << 1; } - let ext = host_fn::from_js_host_call_generic(global, || { - Bun__serializeJSValue(global, self, bits) - })?; + let ext = host_fn::from_js_host_call_owned( + global, + || Bun__serializeJSValue(global, self, bits), + |ext| { + if !ext.handle.is_null() { + // SAFETY: `ext.handle` was leaked by `Bun__serializeJSValue` above. + unsafe { Bun__SerializedScriptSlice__free(ext.handle) }; + } + }, + )?; if ext.bytes.is_null() || ext.handle.is_null() { return Err(JsError::Thrown); } diff --git a/src/jsc/TopExceptionScope.rs b/src/jsc/TopExceptionScope.rs index 4720a093b6c7..d32070c6d923 100644 --- a/src/jsc/TopExceptionScope.rs +++ b/src/jsc/TopExceptionScope.rs @@ -697,6 +697,10 @@ pub fn call_null_is_throw( /// so `simulateThrow()` is satisfied and the assertion fires on mismatch. In release /// builds the C++ validation machinery is compiled out: a single /// `Bun__RETURN_IF_EXCEPTION` FFI call after the closure (1 FFI hop instead of 3). +/// +/// On `Err` the closure's return value is **dropped**. If `R` owns an FFI +/// allocation with no `Drop` (raw pointer, `bun_core::String`), use +/// [`call_check_slow_owned`] instead. #[inline] pub fn call_check_slow_at( global: &JSGlobalObject, @@ -733,6 +737,31 @@ pub fn call_check_slow(global: &JSGlobalObject, f: impl FnOnce() -> R) -> JsR call_check_slow_at(global, SourceLocation::from_caller(), f) } +/// [`call_check_slow`] for closures whose return owns an FFI allocation +/// without `Drop`. If the post-call check reports an exception (including a +/// termination trap set between C++'s own `RETURN_IF_EXCEPTION` and ours), +/// `free` is invoked on the closure's return before the error is propagated; +/// without it the `Err` arm drops `R` as a no-op and the allocation leaks. +#[track_caller] +#[inline] +pub fn call_check_slow_owned( + global: &JSGlobalObject, + f: impl FnOnce() -> R, + free: impl FnOnce(R), +) -> JsResult { + let mut slot = None; + let check = call_check_slow_at(global, SourceLocation::from_caller(), || slot = Some(f())); + // `call_check_slow_at` runs the closure exactly once on every path. + let r = slot.expect("call_check_slow_at ran the closure"); + match check { + Ok(()) => Ok(r), + Err(e) => { + free(r); + Err(e) + } + } +} + /// Macro forms of the per-mode wrappers — expand [`src!`](crate::src) at the *call site* so /// the debug-build diagnostic `SourceLocation` is a NUL-terminated literal (zero-cost), /// not a `#[track_caller]` `Location::file()` interned through a process-level HashMap. diff --git a/src/jsc/URL.rs b/src/jsc/URL.rs index bc2fe0fa2cb2..33067acafa71 100644 --- a/src/jsc/URL.rs +++ b/src/jsc/URL.rs @@ -70,12 +70,22 @@ impl URL { /// If it fails, the tag is marked Dead #[track_caller] pub fn href_from_js(value: JSValue, global: &JSGlobalObject) -> JsResult { - crate::call_check_slow(global, || URL__getHrefFromJS(value, global)) + crate::call_check_slow_owned(global, || URL__getHrefFromJS(value, global), |s| s.deref()) } #[track_caller] pub fn from_js(value: JSValue, global: &JSGlobalObject) -> JsResult>> { - crate::call_check_slow(global, || URL__fromJS(value, global)).map(NonNull::new) + crate::call_check_slow_owned( + global, + || URL__fromJS(value, global), + // SAFETY: `p` came from `URL__fromJS` above and has not been freed. + |p| { + if !p.is_null() { + unsafe { URL__deinit(p) } + } + }, + ) + .map(NonNull::new) } pub fn from_utf8(input: &[u8]) -> Option> { diff --git a/src/jsc/host_fn.rs b/src/jsc/host_fn.rs index d65bf77c1760..7e37e857015c 100644 --- a/src/jsc/host_fn.rs +++ b/src/jsc/host_fn.rs @@ -728,6 +728,19 @@ pub fn from_js_host_call_generic( crate::call_check_slow(global_this, f) } +/// [`from_js_host_call_generic`] for closures whose return owns an FFI +/// allocation without `Drop`; `free` runs on the `Err` path. See +/// [`crate::call_check_slow_owned`]. +#[track_caller] +#[inline] +pub fn from_js_host_call_owned( + global_this: &JSGlobalObject, + f: impl FnOnce() -> R, + free: impl FnOnce(R), +) -> Result { + crate::call_check_slow_owned(global_this, f, free) +} + // ───────────────────────── error conversion helpers ───────────────────────── // For when bubbling up errors to functions that require a C ABI boundary diff --git a/src/jsc/lib.rs b/src/jsc/lib.rs index 85c8c8df5f3c..148b819ad129 100644 --- a/src/jsc/lib.rs +++ b/src/jsc/lib.rs @@ -421,9 +421,9 @@ pub use self::exception::Exception; pub use self::js_type::JSType; pub use self::top_exception_scope::{ ExceptionValidationScope, ExceptionValidationScopeGuard, SourceLocation, TopExceptionScope, - TopExceptionScopeGuard, call_check_slow, call_check_slow_at, call_false_is_throw, - call_false_is_throw_at, call_null_is_throw, call_null_is_throw_at, call_zero_is_throw, - call_zero_is_throw_at, + TopExceptionScopeGuard, call_check_slow, call_check_slow_at, call_check_slow_owned, + call_false_is_throw, call_false_is_throw_at, call_null_is_throw, call_null_is_throw_at, + call_zero_is_throw, call_zero_is_throw_at, }; /// Generated FFI wrappers for C++ `[[ZIG_EXPORT(mode)]]` functions. /// Emitted by `src/codegen/cppbind.ts` into @@ -808,8 +808,8 @@ pub use bun_core::mark_binding; pub use self::host_fn::{ JSHostFn, JSHostFnZig, JSHostFnZigWithContext, JSHostFunctionTypeWithContext, - from_js_host_call, from_js_host_call_generic, host_construct_result, host_fn_result, - host_setter_result, to_js_host_call, to_js_host_fn, to_js_host_fn_result, + from_js_host_call, from_js_host_call_generic, from_js_host_call_owned, host_construct_result, + host_fn_result, host_setter_result, to_js_host_call, to_js_host_fn, to_js_host_fn_result, to_js_host_fn_with_context, }; pub use self::host_object::{HostFnEntry, create_host_function_object}; diff --git a/test/js/node/worker_threads/worker-terminate-ffi-alloc-parent-fixture.js b/test/js/node/worker_threads/worker-terminate-ffi-alloc-parent-fixture.js new file mode 100644 index 000000000000..96c7120b25a9 --- /dev/null +++ b/test/js/node/worker_threads/worker-terminate-ffi-alloc-parent-fixture.js @@ -0,0 +1,30 @@ +"use strict"; +// Parent driver for the "terminate() during allocating FFI wrapper" leak test. +// Runs THREADS concurrent chains, each spawning ITERS workers back-to-back and +// terminate()ing each as soon as it reports ready. The worker body spends most +// of its time inside the allocating C++ `fill()` loop, so terminate() lands in +// the trap-window reliably. +const { Worker } = require("worker_threads"); +const path = require("path"); + +const ITERS = Number(process.env.ITERS || 6); +const THREADS = Number(process.env.THREADS || 6); +const body = path.join(__dirname, "worker-terminate-ffi-alloc-worker-fixture.js"); + +let finished = 0; +let failed = false; +function chain(iter) { + const w = new Worker(body); + w.on("message", () => w.terminate()); + w.on("error", err => { + failed = true; + console.error(err); + process.exitCode = 1; + }); + w.on("exit", () => { + if (failed) return; + if (iter < ITERS) chain(iter + 1); + else if (++finished === THREADS) console.log("done"); + }); +} +for (let i = 0; i < THREADS; i++) chain(0); diff --git a/test/js/node/worker_threads/worker-terminate-ffi-alloc-worker-fixture.js b/test/js/node/worker_threads/worker-terminate-ffi-alloc-worker-fixture.js new file mode 100644 index 000000000000..eaf016c2fb96 --- /dev/null +++ b/test/js/node/worker_threads/worker-terminate-ffi-alloc-worker-fixture.js @@ -0,0 +1,22 @@ +"use strict"; +// Worker body for the "terminate() during allocating FFI wrapper" leak test. +// `new Response(body, { headers: {plain object} })` drives the Rust +// `FetchHeaders::create_from_js` wrapper, whose C++ side allocates a +// `WebCore::FetchHeaders` on the heap and then runs `fill()` over every entry. +// The final C++ guard is a bare `throwScope.exception()` (no trap handling), +// so a termination trap set during `fill()` is only observed by Rust's +// post-call check, which (before the fix) dropped the raw pointer and leaked +// the allocation. Many headers keep `fill()` running long enough for the +// parent's terminate() to land inside it; the "go" message goes out first so +// termination arrives while the very first call is in flight. +const { parentPort } = require("worker_threads"); + +const hdrs = {}; +for (let i = 0; i < 200; i++) hdrs["x-h-" + i] = "v" + i; + +parentPort.postMessage("go"); +function go() { + new Response("", { headers: hdrs }); + setImmediate(go); +} +go(); diff --git a/test/js/node/worker_threads/worker_threads.test.ts b/test/js/node/worker_threads/worker_threads.test.ts index 68d6c6f3f103..63cdb2ec3bdc 100644 --- a/test/js/node/worker_threads/worker_threads.test.ts +++ b/test/js/node/worker_threads/worker_threads.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, setDefaultTimeout, test } from "bun:test"; -import { bunEnv, bunExe, isDebug, tmpdirSync } from "harness"; +import { bunEnv, bunExe, isASAN, isDebug, isWindows, tmpdirSync } from "harness"; import { once } from "node:events"; import fs from "node:fs"; import { join, relative, resolve } from "node:path"; @@ -1756,3 +1756,53 @@ test("the SHARE_ENV founding thread's process.env stays live after the swap", as expect(stdout.trim()).toBe("yes,unset"); expect(exitCode).toBe(0); }); + +// Rust FFI wrappers that return an owned C++ allocation through +// call_check_slow / from_js_host_call_generic (URL::from_js, URL::href_from_js, +// FetchHeaders::create_from_js, FetchHeaders::clone_this, JSBigInt::to_string, +// JSValue::serialize) used to drop the raw pointer when a termination trap +// landed between the C++ function's own exception guard and the Rust wrapper's +// post-call trap check. For FetchHeaders::create_from_js the final C++ guard is +// a bare throwScope.exception() (no trap handling), so the window includes the +// whole headers->fill() loop; a headers object with many entries keeps a +// terminate() landing inside it reliable. Malloc=1 routes WebCore::FetchHeaders +// through system malloc so LSan sees it; pre-existing per-thread singletons +// (WebCore::eventNames etc.) also show up under Malloc=1, so the assertion is +// scoped to the FetchHeaders leak stack rather than a blanket LSan check. +test.skipIf(!isASAN || isWindows)( + "terminate() during an allocating FFI wrapper does not leak the allocation", + async () => { + const env = { + ...bunEnv, + BUN_DESTRUCT_VM_ON_EXIT: "1", + Malloc: "1", + ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=1"].filter(Boolean).join(":"), + LSAN_OPTIONS: `print_suppressions=0:suppressions=${join(import.meta.dirname, "../../../leaksan.supp")}`, + }; + // Several independent processes so one random miss does not mask a regression. + // Malloc=1 exposes unrelated per-thread singletons, so match only the stacks + // this fix covers rather than asserting on the whole LSan report. + const targets = + /WebCore__FetchHeaders__createFromJS|WebCore__FetchHeaders__cloneThis|URL__fromJS|URL__getHrefFromJS|JSC__JSBigInt__toString|Bun__serializeJSValue/; + const hits: string[] = []; + for (let i = 0; i < 4; i++) { + await using proc = Bun.spawn({ + cmd: [bunExe(), join(import.meta.dirname, "worker-terminate-ffi-alloc-parent-fixture.js")], + env, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + for (const line of stderr.split("\n")) if (targets.test(line)) hits.push(line.trim()); + // Positive completion marker so a missing fixture or crash-before-LSan + // cannot satisfy the (otherwise purely negative) assertions; exitCode is + // 1 from pre-existing Malloc=1 singletons, so check stdout + signalCode. + expect({ stdout: stdout.trim(), signalCode: proc.signalCode }).toEqual({ + stdout: "done", + signalCode: null, + }); + } + expect(hits).toEqual([]); + }, + 180_000, +);