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

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

@@ -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<void*> DynamicLibrary::ResolveSymbol(Environment* env,
                                            const std::string& name) {
@@ -585,6 +594,7 @@ void DynamicLibrary::Close(const FunctionCallbackInfo<Value>& 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<Value>& args) {
@@ -1176,6 +1186,7 @@ void DynamicLibrary::RegisterCallback(const FunctionCallbackInfo<Value>& args) {
 
   auto ret = static_cast<uint64_t>(reinterpret_cast<uintptr_t>(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<Value>& args) {

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<void*> ResolveSymbol(Environment* env, const std::string& name);
   struct PreparedFunction {
     std::shared_ptr<FFIFunction> fn;

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