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

Author: trivikr

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');
 

src/node_ffi.cc

@@ -62,12 +62,24 @@ void FFIFunctionInfo::MemoryInfo(MemoryTracker* tracker) const {
 DynamicLibrary::DynamicLibrary(Environment* env, Local<Object> 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<DynamicLibrary*>(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<void*>(lib_.handle) == nullptr;
 }
@@ -1100,7 +1112,7 @@ void DynamicLibrary::RegisterCallback(const FunctionCallbackInfo<Value>& args) {
   }
 
   auto callback = std::unique_ptr<FFICallback>(
-      new FFICallback{.owner = lib,
+      new FFICallback{.owner = BaseObjectPtr<DynamicLibrary>(lib),
                       .env = env,
                       .thread_id = std::this_thread::get_id(),
                       .fn = Global<Function>(isolate, fn),

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<DynamicLibrary> owner;
   Environment* env;
   std::thread::id thread_id;
   v8::Global<v8::Function> fn;
@@ -161,6 +164,7 @@ class DynamicLibrary : public BaseObject {
       Environment* env,
       const std::string& name,
       const std::shared_ptr<FFIFunction>& fn);
+  static void CleanupHook(void* arg);
   static void CleanupFunctionInfo(
       const v8::WeakCallbackInfo<FFIFunctionInfo>& data);
   bool is_closed() const;

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);
+});