Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions src/jsc/FetchHeaders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,16 @@
global: &JSGlobalObject,
value: JSValue,
) -> JsResult<Option<NonNull<FetchHeaders>>> {
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()

Check failure on line 153 in src/jsc/FetchHeaders.rs

View workflow job for this annotation

GitHub Actions / cargo clippy

unsafe block missing a safety comment
}
},
)
}

pub fn put_default(
Expand Down Expand Up @@ -327,9 +334,16 @@
&mut self,
global: &JSGlobalObject,
) -> JsResult<Option<NonNull<FetchHeaders>>> {
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()

Check failure on line 343 in src/jsc/FetchHeaders.rs

View workflow job for this annotation

GitHub Actions / cargo clippy

unsafe block missing a safety comment
}
},
)
}

pub fn deref(&mut self) {
Expand Down
6 changes: 5 additions & 1 deletion src/jsc/JSBigInt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ impl JSBigInt {
}

pub fn to_string(&self, global: &JSGlobalObject) -> JsResult<BunString> {
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(),
)
}
}
13 changes: 10 additions & 3 deletions src/jsc/JSValue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
29 changes: 29 additions & 0 deletions src/jsc/TopExceptionScope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,10 @@ pub fn call_null_is_throw<T>(
/// 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<R>(
global: &JSGlobalObject,
Expand Down Expand Up @@ -733,6 +737,31 @@ pub fn call_check_slow<R>(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<R>(
global: &JSGlobalObject,
f: impl FnOnce() -> R,
free: impl FnOnce(R),
) -> JsResult<R> {
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.
Expand Down
14 changes: 12 additions & 2 deletions src/jsc/URL.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,22 @@
/// If it fails, the tag is marked Dead
#[track_caller]
pub fn href_from_js(value: JSValue, global: &JSGlobalObject) -> JsResult<String> {
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<Option<NonNull<URL>>> {
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) }

Check failure on line 84 in src/jsc/URL.rs

View workflow job for this annotation

GitHub Actions / cargo clippy

unsafe block missing a safety comment
}
},
)
.map(NonNull::new)
}

pub fn from_utf8(input: &[u8]) -> Option<NonNull<URL>> {
Expand Down
13 changes: 13 additions & 0 deletions src/jsc/host_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,19 @@ pub fn from_js_host_call_generic<R>(
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<R>(
global_this: &JSGlobalObject,
f: impl FnOnce() -> R,
free: impl FnOnce(R),
) -> Result<R, JsError> {
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
Expand Down
9 changes: 5 additions & 4 deletions src/jsc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -808,7 +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,
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,
};
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

52 changes: 51 additions & 1 deletion test/js/node/worker_threads/worker_threads.test.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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([]);
Comment thread
robobun marked this conversation as resolved.
},
180_000,
);
Loading