ffi: fix crash in refCallback and unrefCallback

Both handlers called fn.ClearWeak() and fn.SetWeak() without checking
whether the persistent handle was still populated. After the callback
function is garbage collected following an earlier unrefCallback()
call, the handle is empty and both V8 methods dereference a null slot.

InvokeCallback() already guarded against this. Add the same check to
both handlers and throw ERR_INVALID_ARG_VALUE, matching the existing
behavior for a pointer that is not in the callback map.

Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Assisted-by: claude:opus-5

Author: trivikr

doc/api/ffi.md

@@ -459,6 +459,10 @@ memory.
 
 Keeps the callback strongly referenced by JavaScript.
 
+Throws `ERR_INVALID_ARG_VALUE` if the callback function has already been
+garbage collected after a previous `library.unrefCallback(pointer)` call, since
+a collected function cannot be referenced again.
+
 ### `library.unrefCallback(pointer)`
 
 * `pointer` {bigint}
@@ -469,6 +473,9 @@ If the callback function is later garbage collected, subsequent native
 invocations become a no-op. Non-void return values are zero-initialized before
 returning to native code.
 
+Throws `ERR_INVALID_ARG_VALUE` if the callback function has already been
+garbage collected.
+
 ## Calling native functions
 
 Argument conversion depends on the declared FFI type.

src/node_ffi.cc

@@ -1125,6 +1125,15 @@ void DynamicLibrary::RefCallback(const FunctionCallbackInfo<Value>& args) {
     return;
   }
 
+  // The callback function may already have been collected after a previous
+  // unrefCallback() call. The persistent handle is empty in that case, and
+  // ClearWeak() on an empty handle dereferences a null slot. There is also no
+  // function left to make strong again.
+  if (existing->second->fn.IsEmpty()) {
+    THROW_ERR_INVALID_ARG_VALUE(env, "Callback not found");
+    return;
+  }
+
   existing->second->fn.ClearWeak();
 }
 
@@ -1156,6 +1165,14 @@ void DynamicLibrary::UnrefCallback(const FunctionCallbackInfo<Value>& args) {
     return;
   }
 
+  // The callback function may already have been collected by a previous
+  // unrefCallback() call. The persistent handle is empty in that case, and
+  // SetWeak() on an empty handle dereferences a null slot.
+  if (existing->second->fn.IsEmpty()) {
+    THROW_ERR_INVALID_ARG_VALUE(env, "Callback not found");
+    return;
+  }
+
   existing->second->fn.SetWeak();
 }
 

test/ffi/test-ffi-weakref-calls.js

@@ -51,6 +51,37 @@ test('ffi refCallback retains callback function', async (t) => {
   lib.unregisterCallback(pointer);
 });
 
+test('callback ref/unref throw after callback function is collected', async (t) => {
+  const { lib } = ffi.dlopen(libraryPath, fixtureSymbols);
+  t.after(() => lib.close());
+
+  let callback = () => 1;
+  const ref = new WeakRef(callback);
+  const pointer = lib.registerCallback(
+    { arguments: ['i32'], return: 'i32' },
+    callback,
+  );
+
+  lib.unrefCallback(pointer);
+  callback = null;
+
+  await gcUntil(
+    'callback ref/unref throw after callback function is collected',
+    () => ref.deref() === undefined,
+  );
+
+  t.assert.throws(() => lib.unrefCallback(pointer), {
+    code: 'ERR_INVALID_ARG_VALUE',
+    message: /Callback not found/,
+  });
+  t.assert.throws(() => lib.refCallback(pointer), {
+    code: 'ERR_INVALID_ARG_VALUE',
+    message: /Callback not found/,
+  });
+
+  lib.unregisterCallback(pointer);
+});
+
 test('callback ref/unref/unregister throw when library is closed', (t) => {
   const { lib } = ffi.dlopen(libraryPath, fixtureSymbols);
   const callback = lib.registerCallback(() => {});