From 6344ae11b745966125a9d481f48339c0ef621a52 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. Hold the owning library through a BaseObjectPtr in each FFICallback, so it stays strong until its last callback is unregistered or the library is closed. Callbacks released with unrefCallback() still count, because their trampolines remain live native memory. Since this forms a cycle, release the callbacks from an Environment cleanup hook so the library is still freed at teardown. 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 | 14 ++++- src/node_ffi.h | 6 ++- test/ffi/test-ffi-callback-gc.js | 90 ++++++++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 2 deletions(-) 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..be81342cb8e9 100644 --- a/src/node_ffi.cc +++ b/src/node_ffi.cc @@ -62,12 +62,24 @@ void FFIFunctionInfo::MemoryInfo(MemoryTracker* tracker) const { DynamicLibrary::DynamicLibrary(Environment* env, Local object) : BaseObject(env, object) { MakeWeak(); + env->AddCleanupHook(CleanupHook, this); } DynamicLibrary::~DynamicLibrary() { + env()->RemoveCleanupHook(CleanupHook, this); this->Close(); } +void DynamicLibrary::CleanupHook(void* arg) { + DynamicLibrary* lib = static_cast(arg); + // Registered callbacks hold strong references to the library, which would + // otherwise keep it alive past Environment teardown. Release them only + // after Close() returns, since dropping the last reference may delete lib. + decltype(lib->callbacks_) callbacks; + callbacks.swap(lib->callbacks_); + lib->Close(); +} + bool DynamicLibrary::is_closed() const { return static_cast(lib_.handle) == nullptr; } @@ -1100,7 +1112,7 @@ void DynamicLibrary::RegisterCallback(const FunctionCallbackInfo& args) { } auto callback = std::unique_ptr( - new FFICallback{.owner = lib, + new FFICallback{.owner = BaseObjectPtr(lib), .env = env, .thread_id = std::this_thread::get_id(), .fn = Global(isolate, fn), diff --git a/src/node_ffi.h b/src/node_ffi.h index 7758380138fd..4b5df70124d8 100644 --- a/src/node_ffi.h +++ b/src/node_ffi.h @@ -83,7 +83,10 @@ class FFIFunctionInfo final : public BaseObject { }; struct FFICallback { - DynamicLibrary* owner; + // Callback pointers are plain addresses that cannot root the library, so + // each registered callback keeps its owner alive until it is unregistered + // or the library is closed. + BaseObjectPtr owner; Environment* env; std::thread::id thread_id; v8::Global fn; @@ -161,6 +164,7 @@ class DynamicLibrary : public BaseObject { Environment* env, const std::string& name, const std::shared_ptr& fn); + static void CleanupHook(void* arg); static void CleanupFunctionInfo( const v8::WeakCallbackInfo& data); bool is_closed() const; diff --git a/test/ffi/test-ffi-callback-gc.js b/test/ffi/test-ffi-callback-gc.js new file mode 100644 index 000000000000..40ee6625ce67 --- /dev/null +++ b/test/ffi/test-ffi-callback-gc.js @@ -0,0 +1,90 @@ +// 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); +}); + +test('libraries with registered callbacks do not block teardown', async (t) => { + const { Worker } = require('node:worker_threads'); + const { once } = require('node:events'); + const worker = new Worker(` + const ffi = require('node:ffi'); + const lib = new ffi.DynamicLibrary(${JSON.stringify(libraryPath)}); + lib.registerCallback(${JSON.stringify(signature)}, (x) => x); + globalThis.lib = lib; + `, { eval: true }); + const [code] = await once(worker, 'exit'); + t.assert.strictEqual(code, 0); +});