ffi: fix crash in refCallback and unrefCallback

trivikr · web-flow · commit f43086d1e4d6 · 2026-08-02T20:18:19.000Z
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 PR-URL: #64881 Fixes: #64880 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Aviv Keller <me@aviv.sh>
diff --git a/doc/api/ffi.md b/doc/api/ffi.md
@@ -460,6 +460,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}
@@ -470,6 +474,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.
diff --git a/src/node_ffi.cc b/src/node_ffi.cc
@@ -1127,6 +1127,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();
 }
 
@@ -1158,6 +1167,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();
 }
 
diff --git a/test/ffi/test-ffi-weakref-calls.js b/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(() => {});
