Skip to content

ffi: keep FFI functions non-constructible - #65184

Open
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:ffi-constructible-native-functions
Open

ffi: keep FFI functions non-constructible#65184
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:ffi-constructible-native-functions

Conversation

@trivikr

@trivikr trivikr commented Aug 10, 2026

Copy link
Copy Markdown
Member

Fixes: #65183

Use concise method functions for Fast API and shared-buffer wrappers, and create native fallback functions with ConstructorBehavior::kThrow, so FFI functions remain non-constructible on all invocation paths.


Assisted-by: codex:gpt-5.6-sol

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/ffi

@nodejs-github-bot nodejs-github-bot added ffi Issues and PRs related to experimental Foreign Function Interface support. needs-ci PRs that need a full CI run. labels Aug 10, 2026
@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2026
@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 47.82609% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.33%. Comparing base (6fec5e8) to head (edd2a79).
⚠️ Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/ffi-shared-buffer.js 33.33% 24 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65184      +/-   ##
==========================================
+ Coverage   90.32%   90.33%   +0.01%     
==========================================
  Files         760      760              
  Lines      248523   248527       +4     
  Branches    46896    46887       -9     
==========================================
+ Hits       224469   224507      +38     
+ Misses      15465    15459       -6     
+ Partials     8589     8561      -28     
Files with missing lines Coverage Δ
lib/internal/ffi/fast-api.js 94.80% <100.00%> (+0.02%) ⬆️
lib/internal/ffi-shared-buffer.js 55.57% <33.33%> (+0.16%) ⬆️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@trivikr trivikr removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@trivikr
trivikr marked this pull request as draft August 11, 2026 01:15
@trivikr

trivikr commented Aug 11, 2026

Copy link
Copy Markdown
Member Author

Converted to draft since test-ffi-dynamic-library is failing in https://ci.nodejs.org/job/node-test-commit-aix/64326/

not ok 3 ffi/test-ffi-dynamic-library
  ---
  duration_ms: 800.27300
  severity: fail
  exitcode: 1
  stack: |-
    Test failure: 'FFI functions are not constructible'
    Location: test/ffi/test-ffi-dynamic-library.js:70:1
    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    
    true !== false
    
        at TestContext.<anonymous> (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-power9/test/ffi/test-ffi-dynamic-library.js:77:12)
        at Test.runInAsyncScope (node:async_hooks:227:14)
        at Test.run (node:internal/test_runner/test:1397:25)
        at Test.processPendingSubtests (node:internal/test_runner/test:969:18)
        at Test.postRun (node:internal/test_runner/test:1537:19)
        at Test.run (node:internal/test_runner/test:1462:12)
        at process.processTicksAndRejections (node:internal/process/task_queues:104:5)
        at async Test.processPendingSubtests (node:internal/test_runner/test:969:7) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: true,
      expected: false,
      operator: 'strictEqual',
      diff: 'simple'
    }
    
    (node:31457608) ExperimentalWarning: FFI is an experimental feature and might change at any time
    (Use `node --trace-warnings ...` to show where the warning was created)
  ...

@trivikr
trivikr force-pushed the ffi-constructible-native-functions branch from edd2a79 to 317ff65 Compare August 11, 2026 02:41
@trivikr
trivikr marked this pull request as ready for review August 11, 2026 02:41
@trivikr

trivikr commented Aug 11, 2026

Copy link
Copy Markdown
Member Author

The AIX failure occurred because AIX uses neither of the JavaScript wrapper paths updated in the original commit.

AIX/PowerPC is excluded from the Fast API path because there is no PPC64 big-endian trampoline emitter. The shared-buffer path is also disabled on big-endian platforms. Consequently, AIX falls back to the generic native function created with v8::Function::New().

Function::New() defaults to ConstructorBehavior::kAllow, so the fallback function had its own prototype and remained constructible. Since it had no Fast API or shared-buffer metadata, it was returned without a JavaScript wrapper.

I amended the commit to create the generic fallback and shared-buffer slow-path functions with ConstructorBehavior::kThrow. The concise-method changes still ensure that JavaScript wrappers are non-constructible, while the C++ change covers native fallback paths such as AIX.

@trivikr trivikr changed the title ffi: keep wrapped functions non-constructible ffi: keep FFI functions non-constructible Aug 11, 2026
Use concise method functions for Fast API and shared-buffer wrappers,
and create native fallback functions with ConstructorBehavior::kThrow,
so FFI functions remain non-constructible on all invocation paths.

Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
Assisted-by: codex:gpt-5.6-sol
@trivikr
trivikr force-pushed the ffi-constructible-native-functions branch from 317ff65 to 4c9023c Compare August 11, 2026 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ffi Issues and PRs related to experimental Foreign Function Interface support. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ffi: JavaScript wrappers make non-constructible native functions constructible

2 participants