test: split FFI call and callback coverage

Isolate each callback abort scenario so timeouts identify the failing
case. Consolidate callback GC coverage in the weakref test and disable
core dumps for intentional aborts on POSIX.

Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Assisted-by: Codex
PR-URL: https://github.com/nodejs/node/pull/66287
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>

test/ffi/ffi-callback-test-common.js

@@ -0,0 +1,45 @@
+'use strict';
+const common = require('../common');
+const assert = require('node:assert');
+const { spawnSync } = require('node:child_process');
+
+function spawnAbortingChild(source) {
+  const args = ['-e', source];
+  if (common.isWindows) {
+    return spawnSync(process.execPath, args, { encoding: 'utf8' });
+  }
+
+  // Avoid writing core files for these intentional aborts.
+  return spawnSync('/bin/sh', [
+    '-c', 'ulimit -c 0 && exec "$@"',
+    'sh', process.execPath, ...args,
+  ], { encoding: 'utf8' });
+}
+
+function assertAborts(source, message) {
+  const { stderr, status, signal } = spawnAbortingChild(source);
+  assert.ok(common.nodeProcessAborted(status, signal),
+            `status: ${status}, signal: ${signal}
+stderr: ${stderr}`);
+  assert.match(stderr, message);
+}
+
+function assertCallbackAborts(callbackBody, message) {
+  assertAborts(
+    `'use strict';
+const ffi = require('node:ffi');
+const { fixtureSymbols, libraryPath } = require(${JSON.stringify(require.resolve('./ffi-test-common'))});
+const { lib, functions } = ffi.dlopen(libraryPath, fixtureSymbols);
+const callback = lib.registerCallback(
+  { arguments: ['i32'], return: 'i32' },
+  () => { ${callbackBody} },
+);
+functions.call_int_callback(callback, 21);`,
+    message,
+  );
+}
+
+module.exports = {
+  assertAborts,
+  assertCallbackAborts,
+};

test/ffi/ffi.status

@@ -4,6 +4,6 @@ prefix ffi
 
 [$system==solaris] # Also applies to SmartOS
 # Bundled libffi callbacks crash on SmartOS.
-test-ffi-calls: SKIP
+test-ffi-callback*: SKIP
 test-ffi-shared-buffer: SKIP
 test-ffi-weakref-calls: SKIP

test/ffi/test-ffi-callback-abort-cross-thread.js

@@ -0,0 +1,28 @@
+'use strict';
+const common = require('../common');
+common.skipIfFFIMissing();
+const { test } = require('node:test');
+const { assertAborts } = require('./ffi-callback-test-common');
+
+test('ffi aborts on cross-thread callback invocation', () => {
+  const workerSource = `
+const { workerData } = require('node:worker_threads');
+const ffi = require('node:ffi');
+const { fixtureSymbols, libraryPath } = require(${JSON.stringify(require.resolve('./ffi-test-common'))});
+const { functions } = ffi.dlopen(libraryPath, fixtureSymbols);
+functions.call_int_callback(workerData, 21);
+`;
+  assertAborts(
+    `'use strict';
+const { Worker } = require('node:worker_threads');
+const ffi = require('node:ffi');
+const { fixtureSymbols, libraryPath } = require(${JSON.stringify(require.resolve('./ffi-test-common'))});
+const { lib } = ffi.dlopen(libraryPath, fixtureSymbols);
+const callback = lib.registerCallback(
+  { arguments: ['i32'], return: 'i32' },
+  (value) => value * 2,
+);
+new Worker(${JSON.stringify(workerSource)}, { eval: true, workerData: callback });`,
+    /Callbacks can only be invoked on the system thread they were created on/,
+  );
+});

test/ffi/test-ffi-callback-abort-fractional-return.js

@@ -0,0 +1,9 @@
+'use strict';
+const common = require('../common');
+common.skipIfFFIMissing();
+const { test } = require('node:test');
+const { assertCallbackAborts } = require('./ffi-callback-test-common');
+
+test('ffi aborts on fractional callback return values', () => {
+  assertCallbackAborts('return 1.5;', /Callback returned invalid value for declared FFI type/);
+});

test/ffi/test-ffi-callback-abort-out-of-range-return.js

@@ -0,0 +1,9 @@
+'use strict';
+const common = require('../common');
+common.skipIfFFIMissing();
+const { test } = require('node:test');
+const { assertCallbackAborts } = require('./ffi-callback-test-common');
+
+test('ffi aborts on out-of-range callback return values', () => {
+  assertCallbackAborts('return 2 ** 40;', /Callback returned invalid value for declared FFI type/);
+});

test/ffi/test-ffi-callback-abort-promise.js

@@ -0,0 +1,9 @@
+'use strict';
+const common = require('../common');
+common.skipIfFFIMissing();
+const { test } = require('node:test');
+const { assertCallbackAborts } = require('./ffi-callback-test-common');
+
+test('ffi aborts when a callback returns a promise', () => {
+  assertCallbackAborts('return Promise.resolve(1);', /Callbacks cannot return promises/);
+});

test/ffi/test-ffi-callback-abort-throw.js

@@ -0,0 +1,9 @@
+'use strict';
+const common = require('../common');
+common.skipIfFFIMissing();
+const { test } = require('node:test');
+const { assertCallbackAborts } = require('./ffi-callback-test-common');
+
+test('ffi aborts when a callback throws', () => {
+  assertCallbackAborts('throw new Error("boom");', /Callbacks cannot throw an exception/);
+});

test/ffi/test-ffi-callbacks.js

@@ -0,0 +1,84 @@
+'use strict';
+const common = require('../common');
+common.skipIfFFIMissing();
+const assert = require('node:assert');
+const { test } = require('node:test');
+const ffi = require('node:ffi');
+const { cString, fixtureSymbols, libraryPath } = require('./ffi-test-common');
+
+function getLibrary() {
+  return ffi.dlopen(libraryPath, fixtureSymbols);
+}
+
+test('ffi callbacks can be registered and invoked', () => {
+  const { lib, functions: symbols } = getLibrary();
+  const seen = [];
+  const intCallback = lib.registerCallback(
+    { arguments: ['i32'], return: 'i32' },
+    (value) => value * 2,
+  );
+  const stringCallback = lib.registerCallback(
+    { arguments: ['pointer'], return: 'void' },
+    (ptr) => seen.push(ffi.toString(ptr)),
+  );
+  const binaryCallback = lib.registerCallback(
+    { arguments: ['i32', 'i32'], return: 'i32' },
+    (a, b) => a + b,
+  );
+
+  try {
+    assert.strictEqual(symbols.call_int_callback(intCallback, 21), 42);
+    symbols.call_string_callback(stringCallback, cString('hello callback'));
+    assert.deepStrictEqual(seen, ['hello callback']);
+    assert.strictEqual(symbols.call_binary_int_callback(binaryCallback, 19, 23), 42);
+
+    const nullPointerCallback = lib.registerCallback({ return: 'pointer' }, () => null);
+    const undefinedPointerCallback = lib.registerCallback({ return: 'pointer' }, () => undefined);
+    try {
+      assert.strictEqual(symbols.call_pointer_callback_is_null(nullPointerCallback), 1);
+      assert.strictEqual(symbols.call_pointer_callback_is_null(undefinedPointerCallback), 1);
+    } finally {
+      lib.unregisterCallback(nullPointerCallback);
+      lib.unregisterCallback(undefinedPointerCallback);
+    }
+  } finally {
+    lib.unregisterCallback(intCallback);
+    lib.unregisterCallback(stringCallback);
+    lib.unregisterCallback(binaryCallback);
+    lib.close();
+  }
+});
+
+test('ffi callback ref and unref APIs work', () => {
+  const { lib, functions: symbols } = getLibrary();
+  let called = false;
+  const values = [];
+  const voidCallback = lib.registerCallback(() => {
+    called = true;
+  });
+  const countingCallback = lib.registerCallback(
+    { arguments: ['i32'], return: 'i32' },
+    (value) => {
+      values.push(value);
+      return 0;
+    },
+  );
+
+  try {
+    lib.unrefCallback(voidCallback);
+    lib.refCallback(voidCallback);
+    symbols.call_void_callback(voidCallback);
+    symbols.call_callback_multiple_times(countingCallback, 5);
+
+    assert.strictEqual(called, true);
+    assert.deepStrictEqual(values, [0, 1, 2, 3, 4]);
+
+    lib.unregisterCallback(voidCallback);
+    lib.unregisterCallback(countingCallback);
+
+    assert.throws(() => lib.refCallback(voidCallback), /Callback not found/);
+    assert.throws(() => lib.unregisterCallback(-1n), /The first argument must be a non-negative bigint/);
+  } finally {
+    lib.close();
+  }
+});

test/ffi/test-ffi-calls.js

@@ -1,10 +1,8 @@
-// Flags: --expose-gc --allow-natives-syntax
+// Flags: --allow-natives-syntax
 'use strict';
 const common = require('../common');
 common.skipIfFFIMissing();
-const { gcUntil } = require('../common/gc');
 const assert = require('node:assert');
-const { spawnSync } = require('node:child_process');
 const { test } = require('node:test');
 const ffi = require('node:ffi');
 const { cString, fixtureSymbols, libraryPath } = require('./ffi-test-common');
@@ -193,79 +191,6 @@ test('ffi global state helpers work', () => {
   }
 });
 
-test('ffi callbacks can be registered and invoked', () => {
-  const { lib, functions: symbols } = getLibrary();
-  const seen = [];
-  const intCallback = lib.registerCallback(
-    { arguments: ['i32'], return: 'i32' },
-    (value) => value * 2,
-  );
-  const stringCallback = lib.registerCallback(
-    { arguments: ['pointer'], return: 'void' },
-    (ptr) => seen.push(ffi.toString(ptr)),
-  );
-  const binaryCallback = lib.registerCallback(
-    { arguments: ['i32', 'i32'], return: 'i32' },
-    (a, b) => a + b,
-  );
-
-  try {
-    assert.strictEqual(symbols.call_int_callback(intCallback, 21), 42);
-    symbols.call_string_callback(stringCallback, cString('hello callback'));
-    assert.deepStrictEqual(seen, ['hello callback']);
-    assert.strictEqual(symbols.call_binary_int_callback(binaryCallback, 19, 23), 42);
-
-    const nullPointerCallback = lib.registerCallback({ return: 'pointer' }, () => null);
-    const undefinedPointerCallback = lib.registerCallback({ return: 'pointer' }, () => undefined);
-    try {
-      assert.strictEqual(symbols.call_pointer_callback_is_null(nullPointerCallback), 1);
-      assert.strictEqual(symbols.call_pointer_callback_is_null(undefinedPointerCallback), 1);
-    } finally {
-      lib.unregisterCallback(nullPointerCallback);
-      lib.unregisterCallback(undefinedPointerCallback);
-    }
-  } finally {
-    lib.unregisterCallback(intCallback);
-    lib.unregisterCallback(stringCallback);
-    lib.unregisterCallback(binaryCallback);
-    lib.close();
-  }
-});
-
-test('ffi callback ref and unref APIs work', () => {
-  const { lib, functions: symbols } = getLibrary();
-  let called = false;
-  const values = [];
-  const voidCallback = lib.registerCallback(() => {
-    called = true;
-  });
-  const countingCallback = lib.registerCallback(
-    { arguments: ['i32'], return: 'i32' },
-    (value) => {
-      values.push(value);
-      return 0;
-    },
-  );
-
-  try {
-    lib.unrefCallback(voidCallback);
-    lib.refCallback(voidCallback);
-    symbols.call_void_callback(voidCallback);
-    symbols.call_callback_multiple_times(countingCallback, 5);
-
-    assert.strictEqual(called, true);
-    assert.deepStrictEqual(values, [0, 1, 2, 3, 4]);
-
-    lib.unregisterCallback(voidCallback);
-    lib.unregisterCallback(countingCallback);
-
-    assert.throws(() => lib.refCallback(voidCallback), /Callback not found/);
-    assert.throws(() => lib.unregisterCallback(-1n), /The first argument must be a non-negative bigint/);
-  } finally {
-    lib.close();
-  }
-});
-
 test('ffi validates invalid arguments', () => {
   const { lib, functions: symbols } = getLibrary();
   try {
@@ -310,172 +235,3 @@ test('ffi division helpers behave as expected', () => {
     lib.close();
   }
 });
-
-function assertInvalidCallbackReturnAborts(returnExpression) {
-  const { stderr, status, signal } = spawnSync(process.execPath, [
-    '-e',
-    `'use strict';
-const ffi = require('node:ffi');
-const { fixtureSymbols, libraryPath } = require(${JSON.stringify(require.resolve('./ffi-test-common'))});
-const { lib, functions } = ffi.dlopen(libraryPath, fixtureSymbols);
-const callback = lib.registerCallback(
-  { arguments: ['i32'], return: 'i32' },
-  () => (${returnExpression}),
-);
-functions.call_int_callback(callback, 21);`,
-  ], {
-    encoding: 'utf8',
-  });
-
-  assert.ok(common.nodeProcessAborted(status, signal),
-            `status: ${status}, signal: ${signal}
-stderr: ${stderr}`);
-  assert.match(stderr, /Callback returned invalid value for declared FFI type/);
-}
-
-function assertInvalidCallbackBehaviorAborts(callbackBody, message) {
-  const { stderr, status, signal } = spawnSync(process.execPath, [
-    '-e',
-    `'use strict';
-const ffi = require('node:ffi');
-const { fixtureSymbols, libraryPath } = require(${JSON.stringify(require.resolve('./ffi-test-common'))});
-const { lib, functions } = ffi.dlopen(libraryPath, fixtureSymbols);
-const callback = lib.registerCallback(
-  { arguments: ['i32'], return: 'i32' },
-  () => { ${callbackBody} },
-);
-functions.call_int_callback(callback, 21);`,
-  ], {
-    encoding: 'utf8',
-  });
-
-  assert.ok(common.nodeProcessAborted(status, signal),
-            `status: ${status}, signal: ${signal}
-stderr: ${stderr}`);
-  assert.ok(message.test(stderr), stderr);
-}
-
-function assertCrossThreadCallbackAbort() {
-  const workerSource = `
-const { workerData } = require('node:worker_threads');
-const ffi = require('node:ffi');
-const { fixtureSymbols, libraryPath } = require(${JSON.stringify(require.resolve('./ffi-test-common'))});
-const { functions } = ffi.dlopen(libraryPath, fixtureSymbols);
-functions.call_int_callback(workerData, 21);
-`;
-  const { stderr, status, signal } = spawnSync(process.execPath, [
-    '-e',
-    `'use strict';
-const { Worker } = require('node:worker_threads');
-const ffi = require('node:ffi');
-const { fixtureSymbols, libraryPath } = require(${JSON.stringify(require.resolve('./ffi-test-common'))});
-const { lib } = ffi.dlopen(libraryPath, fixtureSymbols);
-const callback = lib.registerCallback(
-  { arguments: ['i32'], return: 'i32' },
-  (value) => value * 2,
-);
-new Worker(${JSON.stringify(workerSource)}, { eval: true, workerData: callback });`,
-  ], {
-    encoding: 'utf8',
-  });
-
-  assert.ok(common.nodeProcessAborted(status, signal),
-            `status: ${status}, signal: ${signal}
-stderr: ${stderr}`);
-  assert.match(stderr, /Callbacks can only be invoked on the system thread they were created on/);
-}
-
-test('ffi aborts on invalid callback return values', () => {
-  assertInvalidCallbackReturnAborts('1.5');
-  assertInvalidCallbackReturnAborts('2 ** 40');
-});
-
-test('ffi aborts on invalid callback behavior', () => {
-  assertInvalidCallbackBehaviorAborts('throw new Error("boom");', /Callbacks cannot throw an exception/);
-  assertInvalidCallbackBehaviorAborts('return Promise.resolve(1);', /Callbacks cannot return promises/);
-});
-
-test('ffi aborts on cross-thread callback invocation', () => {
-  assertCrossThreadCallbackAbort();
-});
-
-test('ffi unrefCallback releases callback function', async () => {
-  const { lib, functions: symbols } = getLibrary();
-  try {
-    let callback = () => 1;
-    const ref = new WeakRef(callback);
-    const pointer = lib.registerCallback(
-      { arguments: ['i32'], return: 'i32' },
-      callback,
-    );
-
-    lib.unrefCallback(pointer);
-    callback = null;
-
-    await gcUntil('ffi unrefCallback releases callback function', () => {
-      return ref.deref() === undefined;
-    });
-
-    assert.strictEqual(symbols.call_int_callback(pointer, 21), 0);
-    lib.unregisterCallback(pointer);
-  } finally {
-    lib.close();
-  }
-});
-
-test('ffi unrefCallback zero-fills narrow callback return', async () => {
-  const { lib, functions: symbols } = getLibrary();
-  try {
-    let callback = () => 1;
-    const ref = new WeakRef(callback);
-    const pointer = lib.registerCallback(
-      { arguments: ['i8'], return: 'i8' },
-      callback,
-    );
-
-    lib.unrefCallback(pointer);
-    callback = null;
-
-    await gcUntil('ffi unrefCallback zero-fills narrow callback return', () => {
-      return ref.deref() === undefined;
-    });
-
-    assert.strictEqual(symbols.call_int8_callback(pointer, 21), 0);
-    lib.unregisterCallback(pointer);
-  } finally {
-    lib.close();
-  }
-});
-
-test('ffi refCallback retains callback function', async () => {
-  const { lib } = getLibrary();
-  try {
-    let callback = () => 1;
-    const ref = new WeakRef(callback);
-    const pointer = lib.registerCallback({ return: 'i32' }, callback);
-
-    lib.unrefCallback(pointer);
-    lib.refCallback(pointer);
-    callback = null;
-
-    for (let i = 0; i < 5; i++) {
-      await gcUntil('ffi refCallback retains callback function', () => true, 1);
-      assert.strictEqual(typeof ref.deref(), 'function');
-    }
-
-    lib.unregisterCallback(pointer);
-  } finally {
-    lib.close();
-  }
-});
-
-test('closing a library invalidates callbacks', () => {
-  const { lib } = getLibrary();
-  const callback = lib.registerCallback(() => {});
-
-  lib.close();
-
-  assert.throws(() => lib.unregisterCallback(callback), /Library is closed/);
-  assert.throws(() => lib.refCallback(callback), /Library is closed/);
-  assert.throws(() => lib.unrefCallback(callback), /Library is closed/);
-});

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

@@ -31,6 +31,28 @@ test('ffi unrefCallback releases callback function', async (t) => {
   lib.unregisterCallback(pointer);
 });
 
+test('ffi unrefCallback zero-fills narrow callback return', async (t) => {
+  const { lib, functions: symbols } = ffi.dlopen(libraryPath, fixtureSymbols);
+  t.after(() => lib.close());
+
+  let callback = () => 1;
+  const ref = new WeakRef(callback);
+  const pointer = lib.registerCallback(
+    { arguments: ['i8'], return: 'i8' },
+    callback,
+  );
+
+  lib.unrefCallback(pointer);
+  callback = null;
+
+  await gcUntil('ffi unrefCallback zero-fills narrow callback return', () => {
+    return ref.deref() === undefined;
+  });
+
+  t.assert.strictEqual(symbols.call_int8_callback(pointer, 21), 0);
+  lib.unregisterCallback(pointer);
+});
+
 test('ffi refCallback retains callback function', async (t) => {
   const { lib } = ffi.dlopen(libraryPath, fixtureSymbols);
   t.after(() => lib.close());