Skip to content
Closed
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
22 changes: 14 additions & 8 deletions src/boringssl/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,30 +94,36 @@ pub unsafe fn ssl_ctx_setup(ctx: *mut boring::SSL_CTX) {
// into the process, including pthreads locks. Failing to meet these constraints
// may result in deadlocks, crashes, or memory corruption.

// Routed through `default_alloc` (mimalloc, or libc under `cfg(bun_asan)`)
// rather than `mimalloc` directly. The BoringSSL build already drops
// `BORINGSSL_REQUIRE_MEMORY_HOOKS` under ASAN so Mach-O/COFF fall back to
// libc, but on ELF the weak hook symbols still resolve to these definitions;
// hard-coding mimalloc here put every `OPENSSL_malloc` allocation outside
// LeakSanitizer's view.
Comment thread
robobun marked this conversation as resolved.
Outdated
#[unsafe(no_mangle)]
pub(crate) extern "C" fn OPENSSL_memory_alloc(size: usize) -> *mut c_void {
bun_alloc::mimalloc::mi_malloc(size)
bun_alloc::default_alloc::malloc(size)
}

// BoringSSL always expects memory to be zero'd
/// # Safety
/// `ptr` must be non-null and have been returned by `OPENSSL_memory_alloc`
/// (i.e. `mi_malloc`); BoringSSL guarantees both for this hook.
/// `ptr` must be non-null and returned by `OPENSSL_memory_alloc`; BoringSSL
/// guarantees both for this hook.
Comment thread
robobun marked this conversation as resolved.
#[unsafe(no_mangle)]
pub(crate) unsafe extern "C" fn OPENSSL_memory_free(ptr: *mut c_void) {
// SAFETY: BoringSSL guarantees ptr is non-null and was returned by
// OPENSSL_memory_alloc above (i.e. mi_malloc).
// OPENSSL_memory_alloc above.
unsafe {
let len = bun_alloc::usable_size(ptr.cast());
let len = bun_alloc::default_alloc::usable_size(ptr);
ptr::write_bytes(ptr.cast::<u8>(), 0, len);
bun_alloc::mimalloc::mi_free(ptr);
bun_alloc::default_alloc::free(ptr);
}
}

#[unsafe(no_mangle)]
pub(crate) extern "C" fn OPENSSL_memory_get_size(ptr: *const c_void) -> usize {
// ptr was returned by mi_malloc (or is null, which usable_size handles).
bun_alloc::usable_size(ptr.cast())
// SAFETY: ptr was returned by OPENSSL_memory_alloc (null-safe).
unsafe { bun_alloc::default_alloc::usable_size(ptr) }
}

pub use bun_sys::posix::INET6_ADDRSTRLEN;
Expand Down
3 changes: 1 addition & 2 deletions src/bun_alloc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,8 +271,7 @@ pub mod default_alloc {
/// # Safety
/// `ptr` must be null or a live allocation from the default allocator.
#[inline]
#[cfg(any(debug_assertions, bun_asan))]
pub(crate) unsafe fn usable_size(ptr: *const c_void) -> usize {
pub unsafe fn usable_size(ptr: *const c_void) -> usize {
Comment thread
claude[bot] marked this conversation as resolved.
if ptr.is_null() {
return 0;
}
Expand Down
8 changes: 8 additions & 0 deletions src/jsc/bindings/node/crypto/CryptoGenKeyPair.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
if (scope.exception()) [[unlikely]] {
JSValue exceptionValue = scope.exception();
(void)scope.tryClearException();
m_keyObj = {};
exceptionCallback(exceptionValue);
return;
}
Expand All @@ -56,10 +57,17 @@
if (scope.exception()) [[unlikely]] {
JSValue exceptionValue = scope.exception();
(void)scope.tryClearException();
m_keyObj = {};
exceptionCallback(exceptionValue);
return;
}

// The exported JS wrappers hold their own RefPtr<KeyObjectData>; drop the
// ctx's ref before re-entering JS. The ctx is freed (and with it this ref)
// only after the callback returns, so a callback that never returns
// (process.exit) would otherwise strand the EVP_PKEY past VM teardown.
Comment thread
robobun marked this conversation as resolved.
Outdated
m_keyObj = {};

Check warning on line 69 in src/jsc/bindings/node/crypto/CryptoGenKeyPair.cpp

View check run for this annotation

Claude / Claude Code Review

Sibling crypto job ctxs (SignJobCtx, DhJobCtx) still hold KeyObjectData refs across the callback

Two sibling job contexts share the exact pattern this PR fixes but are left untouched: `SignJobCtx::runFromJS` (CryptoSignJob.cpp:222, `m_keyData`) and `DhJobCtx::runFromJS` (CryptoDhJob.cpp:47, `m_privateKey`/`m_publicKey`) both invoke `Bun__EventLoop__runCallback*` while their `RefPtr<KeyObjectData>` is still set, and this PR's `OPENSSL_memory_alloc` → `default_alloc` change is what makes those siblings newly LSan-visible on ELF. The same one-line reset (`m_keyData = nullptr;` / `m_privateKey
Comment thread
robobun marked this conversation as resolved.
Outdated
Comment thread
claude[bot] marked this conversation as resolved.
Outdated

Bun__EventLoop__runCallback3(
lexicalGlobalObject,
JSValue::encode(callback),
Expand Down
39 changes: 38 additions & 1 deletion test/js/node/crypto/crypto.key-objects.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {
verify,
} from "crypto";
import fs from "fs";
import { isWindows } from "harness";
import { bunEnv, bunExe, isASAN, isWindows } from "harness";
import { createContext, runInContext, runInThisContext, Script } from "node:vm";
import path from "path";

Expand Down Expand Up @@ -1800,3 +1800,40 @@ test("ECDSA should work", async () => {
function randomProp() {
return "prop" + crypto.randomUUID().replace(/-/g, "");
}

// The generateKeyPair callback re-enters JS while the job ctx still owns a
// RefPtr<KeyObjectData>; process.exit() inside the callback never unwinds so
// that ref is never released and the EVP_PKEY survives past VM teardown. With
// OPENSSL_malloc routed through the default allocator (libc under ASAN) LSan
// reports it. The fix drops the ctx's ref before invoking the callback so the
// JS key objects become the only owners and lastChanceToFinalize frees them.
test.skipIf(!isASAN)(
"generateKeyPair: process.exit() in the callback does not strand the EVP_PKEY past VM teardown",
async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`require("crypto").generateKeyPair("rsa", { modulusLength: 512 }, (err, pub, priv) => {
if (err) { console.error(err); process.exit(2); }
if (pub.type !== "public" || priv.type !== "private") process.exit(3);
process.exit(0);
});`,
],
env: {
...bunEnv,
BUN_DESTRUCT_VM_ON_EXIT: "1",
ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=1"].filter(Boolean).join(":"),
LSAN_OPTIONS: `print_suppressions=0:suppressions=${path.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(stderr).not.toContain("LeakSanitizer");
expect({ stdout, stderr, exitCode }).toEqual({ stdout: "", stderr: "", exitCode: 0 });
},
// LSan symbolizes the leak stack through llvm-symbolizer before the child
// can exit, which is several seconds against the debug binary.
30_000,
);
Loading