Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
36 changes: 30 additions & 6 deletions src/jsc/FetchHeaders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,22 @@ impl FetchHeaders {
global: &JSGlobalObject,
value: JSValue,
) -> JsResult<Option<NonNull<FetchHeaders>>> {
host_fn::from_js_host_call_generic(global, || {
NonNull::new(WebCore__FetchHeaders__createFromJS(global, value))
})
// Own the returned allocation before the post-call trap check so a
// termination request landing between C++'s RETURN_IF_EXCEPTION and
// ours does not drop the raw pointer (no-op) and leak it.
let mut raw: *mut FetchHeaders = core::ptr::null_mut();
let check = host_fn::from_js_host_call_generic(global, || {
raw = WebCore__FetchHeaders__createFromJS(global, value);
});
let headers = NonNull::new(raw);
if let Err(e) = check {
if let Some(mut p) = headers {
// SAFETY: `p` came from `createFromJS` above and has not been deref'd.
unsafe { p.as_mut() }.deref();
}
return Err(e);
}
Ok(headers)
}

pub fn put_default(
Expand Down Expand Up @@ -327,9 +340,20 @@ impl FetchHeaders {
&mut self,
global: &JSGlobalObject,
) -> JsResult<Option<NonNull<FetchHeaders>>> {
host_fn::from_js_host_call_generic(global, || {
NonNull::new(WebCore__FetchHeaders__cloneThis(self, global))
})
// Same trap-window hazard as `create_from_js`; see above.
let mut raw: *mut FetchHeaders = core::ptr::null_mut();
let check = host_fn::from_js_host_call_generic(global, || {
raw = WebCore__FetchHeaders__cloneThis(self, global);
});
let headers = NonNull::new(raw);
if let Err(e) = check {
if let Some(mut p) = headers {
// SAFETY: `p` came from `cloneThis` above and has not been deref'd.
unsafe { p.as_mut() }.deref();
}
return Err(e);
}
Ok(headers)
}

pub fn deref(&mut self) {
Expand Down
13 changes: 12 additions & 1 deletion src/jsc/JSBigInt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,17 @@ 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))
// Own the returned +1 before the post-call trap check so a termination
// request landing between C++'s RETURN_IF_EXCEPTION and ours does not
// drop it as a no-op (BunString is Copy, no Drop).
let mut out = BunString::DEAD;
let check = crate::host_fn::from_js_host_call_generic(global, || {
out = JSC__JSBigInt__toString(self, global);
});
if let Err(e) = check {
out.deref();
return Err(e);
}
Ok(out)
}
}
21 changes: 18 additions & 3 deletions src/jsc/JSValue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2715,9 +2715,24 @@ impl JSValue {
if flags.for_storage {
bits |= 1 << 1;
}
let ext = host_fn::from_js_host_call_generic(global, || {
Bun__serializeJSValue(global, self, bits)
})?;
// Own the returned allocation before the post-call trap check so a
// termination request landing between C++'s exception check and ours
// does not drop the raw handle (no-op) and leak it.
let mut ext = SerializedScriptValueExternal {
bytes: core::ptr::null(),
size: 0,
handle: core::ptr::null_mut(),
};
let check = host_fn::from_js_host_call_generic(global, || {
ext = Bun__serializeJSValue(global, self, bits);
});
if let Err(e) = check {
if !ext.handle.is_null() {
// SAFETY: `ext.handle` was leaked by `Bun__serializeJSValue` above.
unsafe { Bun__SerializedScriptSlice__free(ext.handle) };
}
return Err(e);
}
if ext.bytes.is_null() || ext.handle.is_null() {
return Err(JsError::Thrown);
}
Expand Down
29 changes: 27 additions & 2 deletions src/jsc/URL.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,37 @@ impl URL {
/// 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))
// Own the returned +1 before the post-call trap check so a termination
// request landing between C++'s RETURN_IF_EXCEPTION and ours does not
// drop it as a no-op (String is Copy, no Drop).
let mut out = String::DEAD;
let check = crate::call_check_slow(global, || {
out = URL__getHrefFromJS(value, global);
});
if let Err(e) = check {
out.deref();
return Err(e);
}
Ok(out)
}

#[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)
// Own the returned allocation before the post-call trap check so a
// termination request landing between C++'s RETURN_IF_EXCEPTION and
// ours does not drop the raw pointer (no-op) and leak it.
let mut raw: *mut URL = core::ptr::null_mut();
let check = crate::call_check_slow(global, || {
raw = URL__fromJS(value, global);
});
if let Err(e) = check {
if !raw.is_null() {
// SAFETY: `raw` came from `URL__fromJS` above and has not been freed.
unsafe { URL__deinit(raw) };
}
return Err(e);
}
Ok(NonNull::new(raw))
}

pub fn from_utf8(input: &[u8]) -> Option<NonNull<URL>> {
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.

46 changes: 45 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,47 @@
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());
expect(stdout).toBe("");
}
expect(hits).toEqual([]);

Check warning on line 1799 in test/js/node/worker_threads/worker_threads.test.ts

View check run for this annotation

Claude / Claude Code Review

Leak regression test can pass vacuously — no positive completion marker

This test can pass vacuously: it asserts only that stdout is empty and that no stderr line matches the six FFI symbol names, but never checks `proc.signalCode` or a positive completion marker — so if the worker fixture is later renamed (every Worker fires `error` → stderr gets "Cannot find module", stdout stays `""`) or the process crashes before LSan runs, `hits` stays `[]` and the test still passes. Consider having the parent fixture `console.log("done")` once all `THREADS` chains finish, then
Comment thread
robobun marked this conversation as resolved.
},
180_000,
);
Loading