fixup! ffi: add shared-buffer fast path for numeric and pointer signatures

Raise JS coverage on `lib/internal/ffi-shared-buffer.js` from 83.14% to
99.84% so the nodejs/node `coverage-linux-without-intl` workflow's 95%
threshold is met (the workflow was failing at 94.98%).

Two kinds of changes:

* Mark genuinely-unreachable defensive blocks with `/* c8 ignore */`.
  These are `ERR_INTERNAL_ASSERTION` throws that trigger only when the
  native and JS sides disagree on the shared-buffer ABI, plus the
  `nargs === 0` branch in `buildNumericWrapper` which is dead today
  because `IsSBEligibleSignature` rejects 0-arg signatures on the
  native side.

* Add fixtures and tests to exercise every arity of the void-return
  specialization ladder (1, 3, 4, 5, plus the 7+ rest-params fallback;
  2 and 6 were already covered), and to hit the `throwFFIArgCountError`
  branch in each value-return specialization (1..6) and in the
  pointer-dispatch wrapper.

No behavior change in the FFI runtime path itself.

Author: bengl

lib/internal/ffi-shared-buffer.js

@@ -159,9 +159,11 @@ function writeNumericArg(view, info, offset, arg, index) {
     return;
   }
 
+  /* c8 ignore start */
   // Unreachable: caller filters out non-numeric kinds.
   throw new ERR_INTERNAL_ASSERTION(
     `FFI: writeNumericArg reached with unexpected kind="${kind}"`);
+  /* c8 ignore stop */
 }
 
 // Returns true on fast-path success, false when the caller must fall back
@@ -218,20 +220,24 @@ function wrapWithSharedBuffer(rawFn, parameters, resultType) {
   if (resultType === undefined) resultType = rawFn[kSbResult];
   // `CreateFunction` always attaches these for SB-eligible functions.
   // Missing here means the native side and this wrapper are out of sync.
+  /* c8 ignore start */
   if (parameters === undefined || resultType === undefined) {
     throw new ERR_INTERNAL_ASSERTION(
       'FFI: shared-buffer raw function is missing kSbParams or kSbResult');
   }
+  /* c8 ignore stop */
 
   const slowInvoke = rawFn[kSbInvokeSlow];
   const view = new DataView(buffer);
   let retGetter = null;
   if (resultType !== 'void') {
     const retInfo = sbTypeInfo[resultType];
+    /* c8 ignore start */
     if (retInfo === undefined) {
       throw new ERR_INTERNAL_ASSERTION(
         `FFI: shared-buffer type table missing entry for result type "${resultType}"`);
     }
+    /* c8 ignore stop */
     retGetter = retInfo.get;
   }
 
@@ -241,10 +247,12 @@ function wrapWithSharedBuffer(rawFn, parameters, resultType) {
   let anyPointer = false;
   for (let i = 0; i < nargs; i++) {
     const info = sbTypeInfo[parameters[i]];
+    /* c8 ignore start */
     if (info === undefined) {
       throw new ERR_INTERNAL_ASSERTION(
         `FFI: shared-buffer type table missing entry for parameter type "${parameters[i]}"`);
     }
+    /* c8 ignore stop */
     // Push the `sbTypeInfo` entry directly (entries with the same `kind`
     // share a shape, keeping `writeNumericArg`'s call sites
     // low-polymorphism) and store offsets in a parallel array to avoid
@@ -259,11 +267,13 @@ function wrapWithSharedBuffer(rawFn, parameters, resultType) {
     // Pointer signatures need a per-arg runtime type check and fall back
     // to the native slow-path invoker for non-BigInt pointer arguments,
     // so arity specialization wouldn't buy much here.
+    /* c8 ignore start */
     if (slowInvoke === undefined) {
       throw new ERR_INTERNAL_ASSERTION(
         'FFI: shared-buffer raw function with pointer arguments is ' +
         'missing kSbInvokeSlow');
     }
+    /* c8 ignore stop */
     wrapper = function(...args) {
       if (args.length !== nargs) {
         throwFFIArgCountError(nargs, args.length);
@@ -298,6 +308,11 @@ function wrapWithSharedBuffer(rawFn, parameters, resultType) {
 // but it's the result of lots of perf-hunting.
 function buildNumericWrapper(
   rawFn, view, argInfos, argOffsets, nargs, retGetter) {
+  // `IsSBEligibleSignature` on the native side rejects 0-arg signatures,
+  // so this branch is unreachable today. It's kept as defense-in-depth
+  // for when that filter changes or for programmatic callers that hand a
+  // 0-arg signature through `wrapWithSharedBuffer` directly.
+  /* c8 ignore start */
   if (nargs === 0) {
     if (retGetter === null) {
       return function() {
@@ -315,6 +330,7 @@ function buildNumericWrapper(
       return retGetter(view, 0, true);
     };
   }
+  /* c8 ignore stop */
   if (nargs === 1) {
     const i0 = argInfos[0];
     const o0 = argOffsets[0];
@@ -586,13 +602,15 @@ DynamicLibrary.prototype.getFunctions = function getFunctions(definitions) {
   // uninitialized buffer.
   const functionsDescriptor =
     ObjectGetOwnPropertyDescriptor(DynamicLibrary.prototype, 'functions');
+  /* c8 ignore start */
   if (functionsDescriptor === undefined || !functionsDescriptor.get) {
     // Missing getter means the native and JS sides are out of sync; silently
     // skipping the patch would expose the fast-path-against-uninitialized-buffer
     // footgun this whole block exists to prevent.
     throw new ERR_INTERNAL_ASSERTION(
       'FFI: DynamicLibrary.prototype.functions accessor not found or has no getter');
   }
+  /* c8 ignore stop */
   const origGetter = functionsDescriptor.get;
   ObjectDefineProperty(DynamicLibrary.prototype, 'functions', {
     __proto__: null,

src/node_builtins.cc

@@ -139,10 +139,10 @@ BuiltinLoader::BuiltinCategories BuiltinLoader::GetBuiltinCategories() const {
 #ifndef OPENSSL_NO_QUIC
         "internal/quic/quic", "internal/quic/symbols", "internal/quic/stats",
         "internal/quic/state",
-#endif                  // !OPENSSL_NO_QUIC
+#endif  // !OPENSSL_NO_QUIC
 #if !HAVE_FFI
         "internal/ffi-shared-buffer",
-#endif  // !HAVE_FFI
+#endif                  // !HAVE_FFI
         "ffi",          // Experimental.
         "quic",         // Experimental.
         "sqlite",       // Experimental.

test/ffi/fixture_library/ffi_test_library.c

@@ -233,11 +233,39 @@ FFI_EXPORT void store_sum_2_i32(int32_t a, int32_t b) {
   global_scratch = a + b;
 }
 
+FFI_EXPORT void store_i32(int32_t a) {
+  global_scratch = a;
+}
+
+FFI_EXPORT void store_sum_3_i32(int32_t a, int32_t b, int32_t c) {
+  global_scratch = a + b + c;
+}
+
+FFI_EXPORT void store_sum_4_i32(int32_t a, int32_t b, int32_t c, int32_t d) {
+  global_scratch = a + b + c + d;
+}
+
+FFI_EXPORT void store_sum_5_i32(
+    int32_t a, int32_t b, int32_t c, int32_t d, int32_t e) {
+  global_scratch = a + b + c + d + e;
+}
+
 FFI_EXPORT void store_sum_6_i32(
     int32_t a, int32_t b, int32_t c, int32_t d, int32_t e, int32_t f) {
   global_scratch = a + b + c + d + e + f;
 }
 
+FFI_EXPORT void store_sum_8_i32(int32_t a,
+                                int32_t b,
+                                int32_t c,
+                                int32_t d,
+                                int32_t e,
+                                int32_t f,
+                                int32_t g,
+                                int32_t h) {
+  global_scratch = a + b + c + d + e + f + g + h;
+}
+
 FFI_EXPORT int32_t get_scratch(void) {
   return global_scratch;
 }

test/ffi/test-ffi-shared-buffer.js

@@ -484,34 +484,179 @@ test('void-return 0-arg wrapper branch', () => {
   }
 });
 
-test('void-return wrapper at arity 2 and 6 observes side effects', () => {
+test('void-return wrapper at every specialized arity observes side effects', () => {
   // The arity ladder has a separate void-return closure for each arity.
   // A wiring bug in a mid-arity void specialization would not be caught
-  // by the 0-arg void test above, so exercise the side effects directly.
+  // by the 0-arg void test above, so exercise the side effects directly
+  // at every arity the ladder specializes (1..6) plus the 7+ rest-params
+  // fallback.
   const { lib, functions } = ffi.dlopen(libraryPath, {
+    store_i32: { result: 'void', parameters: ['i32'] },
     store_sum_2_i32: { result: 'void', parameters: ['i32', 'i32'] },
+    store_sum_3_i32: { result: 'void', parameters: ['i32', 'i32', 'i32'] },
+    store_sum_4_i32: {
+      result: 'void',
+      parameters: ['i32', 'i32', 'i32', 'i32'],
+    },
+    store_sum_5_i32: {
+      result: 'void',
+      parameters: ['i32', 'i32', 'i32', 'i32', 'i32'],
+    },
     store_sum_6_i32: {
       result: 'void',
       parameters: ['i32', 'i32', 'i32', 'i32', 'i32', 'i32'],
     },
+    store_sum_8_i32: {
+      result: 'void',
+      parameters: ['i32', 'i32', 'i32', 'i32', 'i32', 'i32', 'i32', 'i32'],
+    },
     get_scratch: { result: 'i32', parameters: [] },
   });
   try {
+    // Powers-of-two summands detect a dropped or duplicated slot at each
+    // arity.
+    assert.strictEqual(functions.store_i32(7), undefined);
+    assert.strictEqual(functions.get_scratch(), 7);
+
     assert.strictEqual(functions.store_sum_2_i32(10, 32), undefined);
     assert.strictEqual(functions.get_scratch(), 42);
 
-    // Powers-of-two summands detect a dropped or duplicated slot.
+    assert.strictEqual(functions.store_sum_3_i32(1, 2, 4), undefined);
+    assert.strictEqual(functions.get_scratch(), 7);
+
+    assert.strictEqual(functions.store_sum_4_i32(1, 2, 4, 8), undefined);
+    assert.strictEqual(functions.get_scratch(), 15);
+
+    assert.strictEqual(functions.store_sum_5_i32(1, 2, 4, 8, 16), undefined);
+    assert.strictEqual(functions.get_scratch(), 31);
+
     assert.strictEqual(
       functions.store_sum_6_i32(1, 2, 4, 8, 16, 32), undefined);
     assert.strictEqual(functions.get_scratch(), 63);
 
-    // Validation still runs on the void-return branch.
+    // 7+ args takes the generic rest-params void branch rather than a
+    // per-arity specialization.
+    assert.strictEqual(
+      functions.store_sum_8_i32(1, 2, 4, 8, 16, 32, 64, 128), undefined);
+    assert.strictEqual(functions.get_scratch(), 255);
+
+    // Validation still runs on every void-return branch, including the
+    // rest-params fallback.
+    assert.throws(
+      () => functions.store_i32(1.5),
+      { code: 'ERR_INVALID_ARG_VALUE' });
     assert.throws(
       () => functions.store_sum_2_i32(1.5, 2),
       { code: 'ERR_INVALID_ARG_VALUE' });
+    assert.throws(
+      () => functions.store_sum_3_i32(1, 1.5, 3),
+      { code: 'ERR_INVALID_ARG_VALUE' });
+    assert.throws(
+      () => functions.store_sum_4_i32(1, 2, 1.5, 4),
+      { code: 'ERR_INVALID_ARG_VALUE' });
+    assert.throws(
+      () => functions.store_sum_5_i32(1, 2, 3, 1.5, 5),
+      { code: 'ERR_INVALID_ARG_VALUE' });
     assert.throws(
       () => functions.store_sum_6_i32(1, 2, 3, 4, 5),
       { code: 'ERR_INVALID_ARG_VALUE' });
+    assert.throws(
+      () => functions.store_sum_8_i32(1, 2, 3, 4, 5, 6, 7, 1.5),
+      { code: 'ERR_INVALID_ARG_VALUE' });
+
+    // Wrong arity hits the `throwFFIArgCountError` branch inside each
+    // specialization (1..6 and the 7+ rest-params fallback).
+    for (const [name, expected, badArgs] of [
+      ['store_i32', 1, []],
+      ['store_sum_2_i32', 2, [1]],
+      ['store_sum_3_i32', 3, [1, 2]],
+      ['store_sum_4_i32', 4, [1, 2, 3]],
+      ['store_sum_5_i32', 5, [1, 2, 3, 4]],
+      ['store_sum_6_i32', 6, [1, 2, 3, 4, 5]],
+      ['store_sum_8_i32', 8, [1, 2, 3, 4, 5, 6, 7]],
+    ]) {
+      assert.throws(
+        () => functions[name](...badArgs),
+        {
+          code: 'ERR_INVALID_ARG_VALUE',
+          message: new RegExp(`expected ${expected}, got ${badArgs.length}`),
+        });
+    }
+  } finally {
+    lib.close();
+  }
+});
+
+test('value-return wrapper arity mismatch hits every specialized branch', () => {
+  // `sum_7_i32` already exercises the 7+ rest-params branch elsewhere;
+  // this test targets the per-arity `throwFFIArgCountError` call in the
+  // value-return closures for arities 1..6 so each specialization's
+  // argument-count guard runs at least once.
+  const { lib, functions } = ffi.dlopen(libraryPath, {
+    logical_not: { result: 'i32', parameters: ['i32'] },
+    add_i32: { result: 'i32', parameters: ['i32', 'i32'] },
+    sum_3_i32: { result: 'i32', parameters: ['i32', 'i32', 'i32'] },
+    sum_4_i32: { result: 'i32', parameters: ['i32', 'i32', 'i32', 'i32'] },
+    sum_five_i32: {
+      result: 'i32',
+      parameters: ['i32', 'i32', 'i32', 'i32', 'i32'],
+    },
+    sum_6_i32: {
+      result: 'i32',
+      parameters: ['i32', 'i32', 'i32', 'i32', 'i32', 'i32'],
+    },
+  });
+  try {
+    for (const [name, expected, badArgs] of [
+      ['logical_not', 1, []],
+      ['add_i32', 2, [1]],
+      ['sum_3_i32', 3, [1, 2]],
+      ['sum_4_i32', 4, [1, 2, 3]],
+      ['sum_five_i32', 5, [1, 2, 3, 4]],
+      ['sum_6_i32', 6, [1, 2, 3, 4, 5]],
+    ]) {
+      assert.throws(
+        () => functions[name](...badArgs),
+        {
+          code: 'ERR_INVALID_ARG_VALUE',
+          message: new RegExp(`expected ${expected}, got ${badArgs.length}`),
+        });
+    }
+
+    // Sanity-check that a correct call still returns a value at each
+    // arity — a bug that swallowed the return on the value-return path
+    // would be caught here.
+    assert.strictEqual(functions.logical_not(0), 1);
+    assert.strictEqual(functions.add_i32(1, 2), 3);
+    assert.strictEqual(functions.sum_3_i32(1, 2, 4), 7);
+    assert.strictEqual(functions.sum_4_i32(1, 2, 4, 8), 15);
+    assert.strictEqual(functions.sum_five_i32(1, 2, 4, 8, 16), 31);
+    assert.strictEqual(functions.sum_6_i32(1, 2, 4, 8, 16, 32), 63);
+  } finally {
+    lib.close();
+  }
+});
+
+test('pointer-dispatch wrapper rejects wrong-arity calls', () => {
+  // Pointer signatures share a single rest-params wrapper rather than the
+  // per-arity ladder, but it still has its own `throwFFIArgCountError`
+  // branch that needs to be exercised.
+  const { lib, functions } = ffi.dlopen(libraryPath, {
+    identity_pointer: { result: 'pointer', parameters: ['pointer'] },
+  });
+  try {
+    assert.throws(
+      () => functions.identity_pointer(),
+      {
+        code: 'ERR_INVALID_ARG_VALUE',
+        message: /expected 1, got 0/,
+      });
+    assert.throws(
+      () => functions.identity_pointer(0n, 0n),
+      {
+        code: 'ERR_INVALID_ARG_VALUE',
+        message: /expected 1, got 2/,
+      });
   } finally {
     lib.close();
   }