From 7e8c874afced00d5425086d7ec90a854fb65262d Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Sun, 18 Oct 2026 20:46:24 -0700 Subject: [PATCH] ffi: keep DynamicLibrary alive while callbacks are registered Callback pointers returned by registerCallback() are plain bigint addresses, so they do not keep the owning DynamicLibrary reachable. If user code dropped the library without calling close(), the next GC ran ~DynamicLibrary(), which freed every ffi_closure. Native code still holding a callback pointer then jumped into freed trampoline memory and crashed the process, even though close() was never called. Make the library a strong reference while it has registered callbacks, and make it weak again when the last callback is unregistered or the library is closed. Callbacks released with unrefCallback() still count, because their trampolines remain live native memory. Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Assisted-by: claude:opus-5.5 --- doc/api/ffi.md | 6 +++ src/node_ffi.cc | 12 +++++ src/node_ffi.h | 4 ++ test/ffi/test-ffi-callback-gc.js | 77 ++++++++++++++++++++++++++++++++ 4 files changed, 99 insertions(+) create mode 100644 test/ffi/test-ffi-callback-gc.js diff --git a/doc/api/ffi.md b/doc/api/ffi.md index d3c01f6e0529..c0d27dd899d5 100644 --- a/doc/api/ffi.md +++ b/doc/api/ffi.md @@ -482,6 +482,12 @@ When `signature` is omitted, the callback uses a default `void ()` signature. The return value is the callback pointer address as a `bigint`. It can be passed to native functions expecting a callback pointer. +While a library has registered callbacks, it is not garbage collected, even if +no JavaScript references to it remain. This keeps callback pointers held by +native code valid until `library.unregisterCallback(pointer)` or +`library.close()` is called. Unregister callbacks or close the library when +they are no longer needed to avoid leaking it. + ```cjs const { DynamicLibrary, suffix } = require('node:ffi'); diff --git a/src/node_ffi.cc b/src/node_ffi.cc index 7581da4e7c6e..1c36eb348b95 100644 --- a/src/node_ffi.cc +++ b/src/node_ffi.cc @@ -119,6 +119,15 @@ void DynamicLibrary::Close() { function_wrappers_.clear(); callbacks_.clear(); } +void DynamicLibrary::UpdateCallbackRetention() { + // Must not be called from ~DynamicLibrary(): the persistent handle may + // already have been reset by the weak callback. + if (callbacks_.empty()) { + MakeWeak(); + } else { + ClearWeak(); + } +} Maybe DynamicLibrary::ResolveSymbol(Environment* env, const std::string& name) { @@ -585,6 +594,7 @@ void DynamicLibrary::Close(const FunctionCallbackInfo& args) { // Closing a library from one of its active callbacks is unsupported and // dangerous. Callbacks must return before the owning library is closed. lib->Close(); + lib->UpdateCallbackRetention(); } void DynamicLibrary::InvokeFunction(const FunctionCallbackInfo& args) { @@ -1176,6 +1186,7 @@ void DynamicLibrary::RegisterCallback(const FunctionCallbackInfo& args) { auto ret = static_cast(reinterpret_cast(callback->ptr)); lib->callbacks_.emplace(callback->ptr, std::move(callback)); + lib->UpdateCallbackRetention(); args.GetReturnValue().Set(BigInt::NewFromUnsigned(isolate, ret)); } @@ -1214,6 +1225,7 @@ void DynamicLibrary::UnregisterCallback( // output, or corrupt memory. Unregistering a callback while it is currently // executing is also unsupported and dangerous. lib->callbacks_.erase(existing); + lib->UpdateCallbackRetention(); } void DynamicLibrary::RefCallback(const FunctionCallbackInfo& args) { diff --git a/src/node_ffi.h b/src/node_ffi.h index 7758380138fd..9e46c5dd86f5 100644 --- a/src/node_ffi.h +++ b/src/node_ffi.h @@ -148,6 +148,10 @@ class DynamicLibrary : public BaseObject { private: void Close(); + // Keeps the JS object strong while callbacks are registered. Callback + // pointers are plain addresses that cannot root the library, so without + // this a GC could free trampolines that native code still holds. + void UpdateCallbackRetention(); v8::Maybe ResolveSymbol(Environment* env, const std::string& name); struct PreparedFunction { std::shared_ptr fn; diff --git a/test/ffi/test-ffi-callback-gc.js b/test/ffi/test-ffi-callback-gc.js new file mode 100644 index 000000000000..ce9e7eb551e2 --- /dev/null +++ b/test/ffi/test-ffi-callback-gc.js @@ -0,0 +1,77 @@ +// Flags: --expose-gc +'use strict'; +const { skipIfFFIMissing } = require('../common'); +skipIfFFIMissing(); + +const { gcUntil } = require('../common/gc'); +const test = require('node:test'); +const ffi = require('node:ffi'); +const { fixtureSymbols, libraryPath } = require('./ffi-test-common'); + +const signature = { arguments: ['i32'], return: 'i32' }; + +test('registered callbacks keep an unreachable library alive', async (t) => { + // A separate handle provides the native function that invokes the callback. + const { lib: keeper, functions: symbols } = + ffi.dlopen(libraryPath, fixtureSymbols); + t.after(() => keeper.close()); + + let lib = new ffi.DynamicLibrary(libraryPath); + const ref = new WeakRef(lib); + const pointer = lib.registerCallback(signature, (x) => x * 2); + t.assert.strictEqual(symbols.call_int_callback(pointer, 21), 42); + + lib = null; + for (let i = 0; i < 5; i++) { + await gcUntil('library with callbacks stays alive', () => true, 1); + t.assert.strictEqual(typeof ref.deref(), 'object'); + t.assert.strictEqual(symbols.call_int_callback(pointer, 21), 42); + } + + ref.deref().unregisterCallback(pointer); +}); + +test('library becomes collectable after its callbacks are unregistered', async () => { + let lib = new ffi.DynamicLibrary(libraryPath); + const ref = new WeakRef(lib); + const first = lib.registerCallback(signature, (x) => x); + const second = lib.registerCallback(signature, (x) => x); + lib.unregisterCallback(first); + lib.unregisterCallback(second); + lib = null; + + await gcUntil('library is collected after unregisterCallback', + () => ref.deref() === undefined); +}); + +test('library becomes collectable after close()', async () => { + let lib = new ffi.DynamicLibrary(libraryPath); + const ref = new WeakRef(lib); + lib.registerCallback(signature, (x) => x); + lib.close(); + lib = null; + + await gcUntil('library is collected after close()', + () => ref.deref() === undefined); +}); + +test('unrefCallback() does not release the callback trampoline', async (t) => { + const { lib: keeper, functions: symbols } = + ffi.dlopen(libraryPath, fixtureSymbols); + t.after(() => keeper.close()); + + let lib = new ffi.DynamicLibrary(libraryPath); + const ref = new WeakRef(lib); + const pointer = lib.registerCallback(signature, () => 1); + lib.unrefCallback(pointer); + lib = null; + + for (let i = 0; i < 5; i++) { + await gcUntil('library with unref callbacks stays alive', () => true, 1); + t.assert.strictEqual(typeof ref.deref(), 'object'); + } + // The JS function may be gone, but the trampoline must still be callable. + symbols.call_int_callback(pointer, 21); + + ref.deref().unregisterCallback(pointer); +});