Skip to content

Commit a1bdd2d

Browse files
trivikraduh95
authored andcommitted
ffi: reject signature conflicts from getFunctions getters
A signature getter could request an earlier symbol with a different signature, making getFunctions() return a wrapper with that signature. Recheck pending functions before caching them, and throw on a mismatch. Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> Assisted-by: claude:opus-5.5 PR-URL: #67194 Fixes: #67192 Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>
1 parent 2360d38 commit a1bdd2d

2 files changed

Lines changed: 69 additions & 3 deletions

File tree

‎src/node_ffi.cc‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -331,9 +331,10 @@ MaybeLocal<Function> DynamicLibrary::CreateFunction(
331331

332332
// Creating a callable emits a trampoline, allocates an FFIFunctionInfo, and
333333
// on the SharedBuffer path allocates an ArrayBuffer, so reuse the one already
334-
// handed out for this symbol. `PrepareFunction()` rejects a request that uses
335-
// a different signature, so a hit always describes the same signature. An
336-
// empty handle means the wrapper was collected; fall through and rebuild.
334+
// handed out for this symbol. `PrepareFunction()` and `GetFunctions()` reject
335+
// a request that uses a different signature, so a hit always describes the
336+
// same signature. An empty handle means the wrapper was collected; fall
337+
// through and rebuild.
337338
auto cached = function_wrappers_.find(name);
338339
if (cached != function_wrappers_.end()) {
339340
if (!cached->second.IsEmpty()) {
@@ -1075,6 +1076,23 @@ void DynamicLibrary::GetFunctions(const FunctionCallbackInfo<Value>& args) {
10751076
});
10761077
}
10771078

1079+
// The signature getters above run user code that may have requested an
1080+
// earlier symbol with a different signature. Check before caching anything
1081+
// so that a rejected call leaves the library unchanged.
1082+
for (const auto& item : pending) {
1083+
auto existing = lib->functions_.find(item.name);
1084+
if (existing != lib->functions_.end() &&
1085+
!SignaturesMatch(
1086+
*existing->second, item.fn->return_type, item.fn->args)) {
1087+
THROW_ERR_INVALID_ARG_VALUE(
1088+
env,
1089+
"Function %s"
1090+
" was already requested with a different signature",
1091+
item.name);
1092+
return;
1093+
}
1094+
}
1095+
10781096
for (const auto& item : pending) {
10791097
if (item.should_cache_symbol) {
10801098
lib->symbols_.emplace(item.name, item.fn->ptr);

‎test/ffi/test-ffi-dynamic-library.js‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,54 @@ test('getFunction caches signatures consistently', () => {
220220
}
221221
});
222222

223+
test('getFunctions rejects a signature changed by a getter', () => {
224+
const lib = new ffi.DynamicLibrary(libraryPath);
225+
const conflicting = { arguments: ['f64', 'f64'], return: 'f64' };
226+
227+
try {
228+
let fn;
229+
assert.throws(() => {
230+
lib.getFunctions({
231+
add_i32: fixtureSymbols.add_i32,
232+
get add_i64() {
233+
fn = lib.getFunction('add_i32', conflicting);
234+
return fixtureSymbols.add_i64;
235+
},
236+
});
237+
}, {
238+
code: 'ERR_INVALID_ARG_VALUE',
239+
message: /Function add_i32 was already requested with a different signature/,
240+
});
241+
242+
// The rejected call must not cache any of its own entries.
243+
assert.deepStrictEqual(Object.keys(lib.functions), ['add_i32']);
244+
assert.strictEqual(lib.getFunction('add_i32', conflicting), fn);
245+
} finally {
246+
lib.close();
247+
}
248+
});
249+
250+
test('getFunctions accepts a matching signature requested by a getter', () => {
251+
const lib = new ffi.DynamicLibrary(libraryPath);
252+
253+
try {
254+
let fn;
255+
const functions = lib.getFunctions({
256+
add_i32: fixtureSymbols.add_i32,
257+
get add_i64() {
258+
fn = lib.getFunction('add_i32', fixtureSymbols.add_i32);
259+
return fixtureSymbols.add_i64;
260+
},
261+
});
262+
263+
assert.strictEqual(functions.add_i32, fn);
264+
assert.strictEqual(functions.add_i32(20, 22), 42);
265+
assert.strictEqual(functions.add_i64(20n, 22n), 42n);
266+
} finally {
267+
lib.close();
268+
}
269+
});
270+
223271
test('resolving the same symbol reuses one function', () => {
224272
const lib = new ffi.DynamicLibrary(libraryPath);
225273
const definitions = { add_i32: fixtureSymbols.add_i32 };

0 commit comments

Comments
 (0)