Skip to content
Closed
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
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 @@ -270,9 +270,8 @@

/// # 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 {

Check warning on line 274 in src/bun_alloc/lib.rs

View check run for this annotation

Claude / Claude Code Review

Crate-root bun_alloc::usable_size is now dead code

The crate-root `bun_alloc::usable_size` (lib.rs:707) is now dead — this PR rewires its only two callers (`OPENSSL_memory_free` / `OPENSSL_memory_get_size`) to `default_alloc::usable_size`, and it's `pub` so the dead-code lint won't flag it. Per REVIEW.md ("Delete dead code in the same PR that makes it dead … helpers whose last caller you rewired … Public items escape dead-code lints — grep for callers manually"), delete it here.
Comment thread
claude[bot] marked this conversation as resolved.
if ptr.is_null() {
return 0;
}
Expand Down
6 changes: 6 additions & 0 deletions src/jsc/bindings/node/crypto/CryptoDhJob.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ void DhJobCtx::runFromJS(JSGlobalObject* lexicalGlobalObject, JSValue callback)
VM& vm = lexicalGlobalObject->vm();
ThrowScope scope = DECLARE_THROW_SCOPE(vm);

// runTask has already produced m_result; drop the key refs before
// re-entering JS so a callback that never returns (process.exit) cannot
// strand the EVP_PKEYs past VM teardown. See KeyPairJobCtx::runFromJS.
Comment thread
robobun marked this conversation as resolved.
Outdated
m_privateKey = nullptr;
m_publicKey = nullptr;

if (!m_result) {
// Same message as the synchronous path so callers observe identical errors either way.
JSObject* err = createError(lexicalGlobalObject, ErrorCode::ERR_CRYPTO_OPERATION_FAILED, "diffieHellman operation failed"_s);
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

More OPENSSL_malloc'd ctx fields (and two sibling job contexts) remain stranded across the callback

The class this PR fixes — "drop every ctx-owned `OPENSSL_malloc` leaf before re-entering JS, since the TZone/FastMalloc ctx isn't LSan-scanned and `process.exit()` strands whatever it points to" — still has instances in and around the functions this PR modified: `KeyPairJobCtx::m_privateKeyEncoding.passphrase` (a `DataPointer` from `OPENSSL_malloc`, `exportPrivate` takes the config by `const&` so does not consume it), `SignJobCtx::m_signResult` / `DhJobCtx::m_result` (both `ByteSource::allocated
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
5 changes: 5 additions & 0 deletions src/jsc/bindings/node/crypto/CryptoSignJob.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,11 @@ void SignJobCtx::runFromJS(JSGlobalObject* lexicalGlobalObject, JSValue callback
auto& vm = lexicalGlobalObject->vm();
auto scope = DECLARE_THROW_SCOPE(vm);

// runTask has already produced m_signResult / m_verifyResult; drop the key
// ref before re-entering JS so a callback that never returns (process.exit)
// cannot strand the EVP_PKEY past VM teardown. See KeyPairJobCtx::runFromJS.
Comment thread
robobun marked this conversation as resolved.
Outdated
m_keyData = nullptr;

switch (m_mode) {
case Mode::Sign: {
if (!m_signResult) {
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