FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

ffi: fix crash in refCallback and unrefCallback · nodejs/node@428e9bc · GitHub

/ node Public

Commit 428e9bc

Browse files
trivikr
authored and
aduh95
committed
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 PR-URL: #64881 Fixes: #64880 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Aviv Keller <me@aviv.sh>
1 parent c96d76a commit 428e9bc

3 files changed

Lines changed: 55 additions & 0 deletions

File tree

‎doc/api/ffi.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -460,6 +460,10 @@ memory.
460460

461461
Keeps the callback strongly referenced by JavaScript.
462462

463+
Throws `ERR_INVALID_ARG_VALUE` if the callback function has already been
464+
garbage collected after a previous `library.unrefCallback(pointer)` call, since
465+
a collected function cannot be referenced again.
466+
463467
### `library.unrefCallback(pointer)`
464468

465469
* `pointer` {bigint}
@@ -470,6 +474,9 @@ If the callback function is later garbage collected, subsequent native
470474
invocations become a no-op. Non-void return values are zero-initialized before
471475
returning to native code.
472476

477+
Throws `ERR_INVALID_ARG_VALUE` if the callback function has already been
478+
garbage collected.
479+
473480
## Calling native functions
474481

475482
Argument conversion depends on the declared FFI type.

‎src/node_ffi.cc‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1127,6 +1127,15 @@ void DynamicLibrary::RefCallback(const FunctionCallbackInfo<Value>& args) {
11271127
return;
11281128
}
11291129

1130+
// The callback function may already have been collected after a previous
1131+
// unrefCallback() call. The persistent handle is empty in that case, and
1132+
// ClearWeak() on an empty handle dereferences a null slot. There is also no
1133+
// function left to make strong again.
1134+
if (existing->second->fn.IsEmpty()) {
1135+
THROW_ERR_INVALID_ARG_VALUE(env, "Callback not found");
1136+
return;
1137+
}
1138+
11301139
existing->second->fn.ClearWeak();
11311140
}
11321141

@@ -1158,6 +1167,14 @@ void DynamicLibrary::UnrefCallback(const FunctionCallbackInfo<Value>& args) {
11581167
return;
11591168
}
11601169

1170+
// The callback function may already have been collected by a previous
1171+
// unrefCallback() call. The persistent handle is empty in that case, and
1172+
// SetWeak() on an empty handle dereferences a null slot.
1173+
if (existing->second->fn.IsEmpty()) {
1174+
THROW_ERR_INVALID_ARG_VALUE(env, "Callback not found");
1175+
return;
1176+
}
1177+
11611178
existing->second->fn.SetWeak();
11621179
}
11631180

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,37 @@ test('ffi refCallback retains callback function', async (t) => {
5151
lib.unregisterCallback(pointer);
5252
});
5353

54+
test('callback ref/unref throw after callback function is collected', async (t) => {
55+
const { lib } = ffi.dlopen(libraryPath, fixtureSymbols);
56+
t.after(() => lib.close());
57+
58+
let callback = () => 1;
59+
const ref = new WeakRef(callback);
60+
const pointer = lib.registerCallback(
61+
{ arguments: ['i32'], return: 'i32' },
62+
callback,
63+
);
64+
65+
lib.unrefCallback(pointer);
66+
callback = null;
67+
68+
await gcUntil(
69+
'callback ref/unref throw after callback function is collected',
70+
() => ref.deref() === undefined,
71+
);
72+
73+
t.assert.throws(() => lib.unrefCallback(pointer), {
74+
code: 'ERR_INVALID_ARG_VALUE',
75+
message: /Callback not found/,
76+
});
77+
t.assert.throws(() => lib.refCallback(pointer), {
78+
code: 'ERR_INVALID_ARG_VALUE',
79+
message: /Callback not found/,
80+
});
81+
82+
lib.unregisterCallback(pointer);
83+
});
84+
5485
test('callback ref/unref/unregister throw when library is closed', (t) => {
5586
const { lib } = ffi.dlopen(libraryPath, fixtureSymbols);
5687
const callback = lib.registerCallback(() => {});

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL