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