lib,src,test,doc: add node:ffi module by cjihrig · Pull Request #62072 · nodejs/node

cjihrig · 2026-03-02T14:43:52Z

This is not ready for review yet. Just opening to see if this is something the project is still interested in.

nodejs-github-bot · 2026-03-02T14:43:58Z

Review requested:

@nodejs/gyp
@nodejs/security-wg

anonrig · 2026-03-02T16:00:18Z

Yes

Qard

Looks very cool! I'm glad to see someone has picked this back up. 🙂

I did a quick review and it all looks good. My only comment is there seems to be a bunch of static strings and a private symbol which we might want to define in env_properties.h.

src/node_ffi.cc

Signed-off-by: Paolo Insogna <paolo@cowtech.it>

github-actions · 2026-03-18T23:43:23Z

Failed to start CI

   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/23272564448

nodejs-github-bot · 2026-03-18T23:44:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/71877/

nodejs-github-bot · 2026-03-18T23:54:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/71878/

cjihrig · 2026-03-18T23:59:59Z

@anonrig I cancelled that CI run to save resources. I started https://ci.nodejs.org/job/node-test-pull-request/71877/ manually, and it's currently running.

@cjihrig

given that this is still draft and @cjihrig indicated it's not ready for review, a sign off is not yet appropriate.

mcollina

lgtm

nodejs-github-bot · 2026-03-19T03:10:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/71884/

@cjihrig

given that this is still draft and @cjihrig indicated it's not ready for review, a sign off is not yet appropriate.

jasnell · 2026-03-19T03:41:32Z

@cjihrig ... definitely interested in seeing ffi land. I was hoping @bengl's prior efforts would make progress. Once you feel this is ready to go, I'll be happy to review.

* src,deps,test: various improvements Signed-off-by: Paolo Insogna <paolo@cowtech.it> * deps,test: fixed failures Signed-off-by: Paolo Insogna <paolo@cowtech.it> * deps,test: fixed failures Signed-off-by: Paolo Insogna <paolo@cowtech.it> * deps,test: fixed failures Signed-off-by: Paolo Insogna <paolo@cowtech.it> * deps,test: fixed failures Signed-off-by: Paolo Insogna <paolo@cowtech.it> * doc,lib: fixed failures Signed-off-by: Paolo Insogna <paolo@cowtech.it> * deps: fixed failures Signed-off-by: Paolo Insogna <paolo@cowtech.it> * test: fixed failures Signed-off-by: Paolo Insogna <paolo@cowtech.it> --------- Signed-off-by: Paolo Insogna <paolo@cowtech.it>

* test: fixed build on Windows Signed-off-by: Paolo Insogna <paolo@cowtech.it> * test: fixed build on Windows Signed-off-by: Paolo Insogna <paolo@cowtech.it> * test: fixed build on Windows Signed-off-by: Paolo Insogna <paolo@cowtech.it> * test: fixed build on Windows Signed-off-by: Paolo Insogna <paolo@cowtech.it> * test: removed useless file Signed-off-by: Paolo Insogna <paolo@cowtech.it> --------- Signed-off-by: Paolo Insogna <paolo@cowtech.it>

mcollina

lgtm

mcollina · 2026-04-01T17:27:16Z

@cjihrig @ShogunPanda this now conflicts, can you rebase?

codecov · 2026-04-02T16:06:16Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.18%. Comparing base (a9ac9b1) to head (281152e).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #62072      +/-   ##
==========================================
- Coverage   89.70%   89.18%   -0.53%     
==========================================
  Files         695      702       +7     
  Lines      214213   216018    +1805     
  Branches    41021    41395     +374     
==========================================
+ Hits       192167   192657     +490     
- Misses      14113    15415    +1302     
- Partials     7933     7946      +13

Files with missing lines	Coverage Δ
lib/ffi.js	`95.73% <ø> (ø)`
lib/internal/bootstrap/realm.js	`96.21% <ø> (+<0.01%)`	⬆️
lib/internal/process/permission.js	`100.00% <ø> (ø)`
lib/internal/process/pre_execution.js	`98.63% <ø> (+1.40%)`	⬆️
src/env.cc	`85.53% <ø> (+0.34%)`	⬆️
src/ffi/data.cc	`5.40% <ø> (ø)`
src/ffi/types.cc	`0.00% <ø> (ø)`
src/node_binding.cc	`82.74% <ø> (ø)`
src/node_builtins.cc	`76.09% <ø> (+0.04%)`	⬆️
src/node_ffi.cc	`12.43% <ø> (ø)`
... and 8 more

... and 24 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.

jasnell · 2026-04-02T16:19:35Z

src/ffi/data.cc

+
+bool ValidateBufferLength(Environment* env, size_t len) {
+  if (len > Buffer::kMaxLength) {
+    env->isolate()->ThrowException(ERR_BUFFER_TOO_LARGE(env->isolate()));


Does THROW_ERR_BUFFER_TOO_LARGE(env) not work here?

jasnell · 2026-04-02T16:20:46Z

src/ffi/data.cc

+    } else if (value->IsNumber()) {
+      uint64_t validated;
+      if (!GetValidatedUnsignedInt(
+              env, value, 9007199254740991ULL, "uint64", &validated)) {


Can this be a constant to avoid the inline magic number?

jasnell · 2026-04-02T16:21:02Z

src/ffi/data.cc

+      return;
+    }
+
+    converted = static_cast<T>(number.ToLocalChecked()->Value());


We should avoid using ToLocalChecked

jasnell · 2026-04-02T16:21:59Z

src/ffi/data.cc

+  THROW_IF_INSUFFICIENT_PERMISSIONS(env, permission::PermissionScope::kFFI, "");
+
+  if (args.Length() < 1 || !args[0]->IsBigInt()) {
+    env->ThrowTypeError("The first argument must be a bigint");


These should be proper ERR_* errors.

jasnell · 2026-04-02T16:26:36Z

src/node_ffi.cc

+  Local<External> data = External::New(isolate, info);
+  Local<Function> ret =
+      Function::New(env->context(), DynamicLibrary::InvokeFunction, data)
+          .ToLocalChecked();


There's a few ToLocalChecked and Check uses that should be cleaned up.

jasnell · 2026-04-02T16:32:07Z

src/ffi/types.cc

+                 Local<Object> object,
+                 Local<String> key,
+                 bool* out) {
+  v8::Maybe<bool> has = object->Has(context, key);


v8::Maybe in the using list above

jasnell · 2026-04-02T16:32:54Z

src/ffi/types.cc

+  Local<String> parameters_key =
+      String::NewFromUtf8Literal(env->isolate(), "parameters");
+  Local<String> arguments_key =
+      String::NewFromUtf8Literal(env->isolate(), "arguments");


These could be added to env-properties.h

jasnell · 2026-04-02T16:33:53Z

src/ffi/types.cc

+
+    *static_cast<double*>(ret) = arg->NumberValue(context).FromJust();
+  } else if (type == &ffi_type_pointer) {
+    if (arg->IsNull() || arg->IsUndefined()) {


IsNullOrUndefined?

jasnell · 2026-04-02T16:37:23Z

src/node_ffi.cc

+      String::NewFromUtf8(isolate, name.c_str(), NewStringType::kNormal)
+          .ToLocalChecked());
+  ret->Set(env->context(),
+           FIXED_ONE_BYTE_STRING(env->isolate(), "pointer"),


env-properties?

jasnell · 2026-04-02T16:37:40Z

src/node_ffi.cc

+      info, DynamicLibrary::CleanupFunctionInfo, WeakCallbackType::kParameter);
+  ret->SetName(
+      String::NewFromUtf8(isolate, name.c_str(), NewStringType::kNormal)
+          .ToLocalChecked());


ToV8Value I think?

Signed-off-by: Paolo Insogna <paolo@cowtech.it>

ShogunPanda · 2026-04-02T18:45:17Z

@jasnell Thanks for the review, tomorrow (CEST) I'll address all of them.
Does the architecture and API look fine to you?

nodejs-github-bot added build Issues and PRs related to build files or the CI. dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. labels Mar 2, 2026

Qard reviewed Mar 2, 2026

View reviewed changes

src/node_ffi.cc Outdated Show resolved Hide resolved

cjihrig force-pushed the ffi branch from 5c2ec05 to 9462e4d Compare March 6, 2026 05:03

lib,src,test,doc: add node:ffi module

631d73a

cjihrig force-pushed the ffi branch from 9462e4d to 631d73a Compare March 10, 2026 03:46

ShogunPanda added 2 commits March 17, 2026 09:14

ffi: Refactor to simpler API.

91d97f7

Signed-off-by: Paolo Insogna <paolo@cowtech.it>

fix: Various improvements.

fe8170e

Signed-off-by: Paolo Insogna <paolo@cowtech.it>

cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2026

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Mar 18, 2026

anonrig previously approved these changes Mar 18, 2026

View reviewed changes

anonrig added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Mar 18, 2026

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2026

mcollina previously approved these changes Mar 19, 2026

View reviewed changes

mcollina added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Mar 19, 2026

cjihrig added 4 commits March 18, 2026 21:05

fix the build

8087c26

fix lint

7bcfd25

remove test fixture readme

5d368b3

remove extra test file

e2023fd

tests

da7cdbf

ShogunPanda added 2 commits March 25, 2026 17:29

cjihrig marked this pull request as ready for review April 1, 2026 14:41

mcollina approved these changes Apr 1, 2026

View reviewed changes

Merge branch 'main' into ffi

331ad93

jasnell reviewed Apr 2, 2026

View reviewed changes

src: formatted files

281152e

Signed-off-by: Paolo Insogna <paolo@cowtech.it>

Uh oh!

Conversation

cjihrig commented Mar 2, 2026

Uh oh!

nodejs-github-bot commented Mar 2, 2026

Uh oh!

anonrig commented Mar 2, 2026

Uh oh!

Qard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Mar 18, 2026

Uh oh!

nodejs-github-bot commented Mar 18, 2026

Uh oh!

nodejs-github-bot commented Mar 18, 2026

Uh oh!

cjihrig commented Mar 18, 2026

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Mar 19, 2026

Uh oh!

jasnell commented Mar 19, 2026

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Apr 1, 2026

Uh oh!

codecov bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShogunPanda commented Apr 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented Apr 2, 2026 •

edited

Loading

jasnell Apr 2, 2026 •

edited

Loading