Foreign Function Interface (FFI) implementation

[bengl]

Here's a first pass at implementing a foreign function interface (FFI). It's roughly based on my attempt at building an FFI library in userland. That implementation uses dyncall, which doesn't support all the platforms that Node.js does, so I've used libffi instead.

As an example, this works on my PR branch.

$ cat test.c 
int test_add(int a, int b) {
  return a + b;
}
$ clang -shared -undefined dynamic_lookup -o libtest.so test.c
$ ./node -p "require('node:ffi').getNativeFunction('./libtest.so', 'test_add', 'int', ['int', 'int'])(7, 5)"
12

Here is the new doc page for this.

Limitations

• Structs aren't supported as return values or arguments (but pointers are, and, of course, pointers to structs).
• Callbacks aren't supported. See Open Questions below.
• Only the documented types are supported, even if a given type is a typedef or otherwise equivalent to a supported type.

Open Questions

• How should libffi be updated/maintained?
• Do callbacks need to be supported in this PR?
  • My thoughts: It's a bigger task, and I think there's plenty of utility without callbacks. I'd prefer to do this separately afterward.
• Should pointers be obfuscated somehow? Right now they aren't. They very easily could be, though, with a Pointer class.
  • My thoughts: I don't particularly think it's necessary, considering there are certainly other ways, both in several npm modules and in this feature, to get at pointers. Also, not having raw values makes it difficult to pack them into structs inside buffers, or do any pointer arithmetic.
• Should Buffers/ArrayBuffers/ArrayBufferViews be acceptable as pointer arguments? This might simplify userland code.
  • My thoughts: Maybe, but in a separate PR.
• Similarly, should Strings be acceptable as char * arguments?
  • My thoughts: Maybe, but in a separate PR.

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/tsc

[benjamingr]

The actual approach looks neat and the changes look like the correct approach to do this.

Still I have to ask - is this common enough to be in core and not a userland module?

[Kruithne]

The actual approach looks neat and the changes look like the correct approach to do this.

Still I have to ask - is this common enough to be in core and not a userland module?

With desktop applications built on Chromium + Node.js frameworks (Electron, nw.js, etc) becoming the common standard, having a means to interact with native libraries without needing to reach for third party modules would be a big improvement.

Additionally, bun has shipped built in FFI support as well.

[bengl]

@benjamingr

Still I have to ask - is this common enough to be in core and not a userland module?

Calling into native code is common enough, given the myriad native modules on npm.

The primary motivation for FFI, at least from my perspective, is the ability to call into native code without the usage of an additional C++ binding layer. This comes up particularly with pre-built dynamic libraries. For example, the official Oracle DB driver requires that a closed-source dynamic library be installed. This means that (currently) an additional C++ addon must be either compiled or prebuilt. With FFI built into core, the close-source dynamic library can be used without a C++ addon.

For another fun example, libm can be used directly without an addon 😃:

$ ./node -p "require('node:ffi').getNativeFunction('/usr/lib/libm.dylib', 'atanh', 'double', ['double'])(Math.tanh(Math.PI))"
3.141592653589798

The real win here is less of a need for native addons.

[Kruithne]

This means that (currently) an additional C++ addon must be either compiled or prebuilt.

This is a common problem integrating applications with Steamworks (the API for Steam). The library cannot be publicly distributed and the popular native addon for it is now unmaintained and broken. With FFI, this wouldn't be an issue for a lot of people.

[aapoalas]

Hello from Deno world! Nice to see FFI in Node as well: At least I've been completely taken by it and have spent significant amounts of time both thinking about and contributing to it on Deno side.

Few things that I would bring up from that experience:

1. Be very careful of giving these keys to users. If at all possible, I would even recommend making FFI always forbidden by default and only allowed when explicitly given permission for, whether or not the user is using the new Node Permissions stuff. The possibility for misuse is just too great and too easy. Examples include opening the win32 DLL by name or by pointer address (this is somewhat static so may be liable to brute force search) which then enables exploiting basically any API one wants, or opening libc for mmap, using that to create writable, executable memory and writing one's own exploit directly into a TypedArray.
2. Regarding pointer types, I recommend using V8's own v8::External. I've implemented support for Fast API to pass those as void pointers to and from native calls, meaning that if eventually you implement a JIT compiling trampoline on the native call side (both Bun and Deno have this) then you can leverage the V8 Fast API and get to ~2-5 nanoseconds baseline call performance. The Externals show up as either null (for null pointers) or as prototype-less and non-extendable objects on the JS side, meaning that they're already essentially opaque. Also note that V8 has some issues regarding zero-length ArrayBuffers: Certain APIs will think that a zero-length buffer is by necessity backed by a null pointer, so using buffers to represent pointers may backfire in some edge cases.
3. Deno has chosen to have two different pointer types: "pointer" and "buffer". The first corresponds (nowadays) to an External object or null, while the latter corresponds to a TypedArray (we've chosen to always use Uint8Array as V8 Fast API requires us to choose something and that's the most reasonable one). I would love it if we could mix and match, but the V8 Fast API does not work like that, at least for the moment. Fast API does offer call type overloading, but it's limited in such a way that this particular overload wouldn't work (I think). But... this sounds like a good idea for a change!
4. Strings as char *: While I think this is "cute" and would be nice, I don't think its necessarily a good idea. Often enough, users would probably want to optimise their string usage, reusing buffers as possible etc. Using a plain JS string would need to do a copy on each call. V8 Fast API does offer the FastOneByteString class or whatever its name was, which would offer a way to do this without copying every time, but the lifetime of said pointer is not really controllable by the FFI API user (whereas a buffer is entirely controllable) and not all JS strings would take the Fast API path (eg. TwoByte strings, roped strings, ...), meaning that the performance benefit would be "choppy".

I implemented FFI callbacks for Deno about a year ago, I think? The runtime backends are of course quite different so in that sense I doubt there's much that I could "teach" you. One thing that does spring to mind about that is, however, again regarding V8 Fast API: The Fast API has a requirement that the calls may not call back into V8 / JavaScript. This means that for FFI API declarations (if you take Fast API into use), you'll need to have some sort of flag to opt out of Fast API. For Deno that's callback: true, and it just makes it so that no Fast API binding is generated for the FFI function binding.

This opt out needs to happen in sometimes surprising places. eg. I've been playing with creating C++ lambdas from JavaScript. When passing a lambda into an API, any copies made of the lambda will callback, so APIs that pass a lambda or that may cause a lambda (created by me) to be passed on the native side need the callback: true flag.

I hope some of this may be useful to you! If you want to know more about how Deno's FFI works, I've written a bunch about it here. Feel free to ping me or send me an email or something if you have any direct questions you would want to ask!

And if I am speaking out of turn, my apologies: I absolutely mean no offense.

[bengl]

@aapoalas Thanks for all the info and ideas! Especially your notes on callbacks, and this set of notes.

Be very careful of giving these keys to users.

Yes, before I take this PR out of Draft mode, I'll be ensuring that it's covered by the permission model. That being said, I don't see this as being any more dangerous (from a security perspective) than being able to load native addons.

Regarding pointer types, I recommend using V8's own v8::External.

One of the goals here is to enable pointer math. I'd prefer not having to require multiple buffers in order to pass multiple pointers into a function. For example, in a test file, I have this:

https://github.com/nodejs/node/blob/5e33eeb008e3b3e5aeb1a9f0aed6d948e1bbe367/test/ffi/types/test.js#L75-L81

I could enable this with a wrapper class, and get a new External when calling add() on the wrapper class or something like that, but I have a strong preference for being able to do this entirely in JavaScript. As before, I don't think this is any more of a security issue than native addon.

The biggest issue with this, right now, is that my implementation stuffs all arguments and a slot for the return value into a Buffer (which is re-used for each call). I don't know how I would do that if pointers were External objects instead of bigints or numbers.

Deno has chosen to have two different pointer types: "pointer" and "buffer".

Hmm, it sounds like this might be because of implementation details. It might not actually apply here?

Strings as char *: While I think this is "cute" and would be nice, I don't think its necessarily a good idea. Often enough, users would probably want to optimise their string usage, reusing buffers as possible etc. Using a plain JS string would need to do a copy on each call.

I don't think it would make sense to only support strings. Pointers (or Buffers, if taking that approach) would stuff need to be supported. Strings could also be cached.

[aapoalas]

@aapoalas Thanks for all the info and ideas! Especially your notes on callbacks, and this set of notes.

No problem, I hope it's helpful!

Be very careful of giving these keys to users.

Yes, before I take this PR out of Draft mode, I'll be ensuring that it's covered by the permission model. That being said, I don't see this as being any more dangerous (from a security perspective) than being able to load native addons.

Yeah, it's not opening more doors really, but it is much easier to do nasty stuff with FFI since it becomes possible to keep most of the work inside JS.

Regarding pointer types, I recommend using V8's own v8::External.

One of the goals here is to enable pointer math. I'd prefer not having to require multiple buffers in order to pass multiple pointers into a function. For example, in a test file, I have this:

Deno only in the most recent minor version update changed from pointer numbers to pointer objects. With that change I added a few APIs: Creating a pointer from a number (dangerous!), offsetting a pointer, and getting the numeric value of a pointer. These of course exist to enable pointer arithmetic when needed.

Only today I changed one private repo to use the new API and admittedly assigning pointers to buffers became harder, one might even say tedious. Still, it wasn't a terribly common use case, at least in that code base.

In essence, I'd argue that the performance that Fast API offers is more important :) but that can of course be construed to be "sour grapes" :D

I could enable this with a wrapper class, and get a new External when calling add() on the wrapper class or something like that, but I have a strong preference for being able to do this entirely in JavaScript. As before, I don't think this is any more of a security issue than native addon.

I'm no security expert, but isn't pointer spoofing one relatively common attack? Any number always being a pointer does feel a bit scary to me in that sense :) From an ergonomics point of view it's also a bit unfortunate that there is no difference between a pointer and a number.

Side note: ARM might have pointer cryptography turned on, in which case pointer arithmetics would need to prepare for number OR BigInt values.

The biggest issue with this, right now, is that my implementation stuffs all arguments and a slot for the return value into a Buffer (which is re-used for each call). I don't know how I would do that if pointers were External objects instead of bigints or numbers.

Ah okay, yeah I hadn't realized you're doing this in a totally different way. Deno passes the parameters "traditionally".

Deno has chosen to have two different pointer types: "pointer" and "buffer".

Hmm, it sounds like this might be because of implementation details. It might not actually apply here?

Yeah, since you're just passing a buffer, we're on very different paths :) It's a very intriguing choice. Due to Deno's reliance on V8 Fast API it wouldn't make sense to us, but without that and JIT compiling the native side glue code it definitely makes sense. Nice stuff!

Strings as char *: While I think this is "cute" and would be nice, I don't think its necessarily a good idea. Often enough, users would probably want to optimise their string usage, reusing buffers as possible etc. Using a plain JS string would need to do a copy on each call.

I don't think it would make sense to only support strings. Pointers (or Buffers, if taking that approach) would stuff need to be supported. Strings could also be cached.

I'm not sure how strings could be cached in a reasonable way, since you'd want to avoid leaking memory and thus would need to bind the encoded string's buffer into the lifetime of the string itself, somehow, but strings cannot be used as keys in WeakMaps. So, it would then make sense to just encode a string once and return the buffer to the user, after which they can manage the lifetime. This then just becomes essentially the same as TextDecoder, I think.

[bengl]

@RafaelGSS

We should disable it by default when using the Permission model, right?

Actually, how should this work? Should it just be enabled or disabled, like with worker threads or child processes? Since the filename parameter to dlopen() doesn't necessarily correspond to an actual file on disk (e.g. /usr/lib/libm.dylib is not a real file on MacOS, but it can be loaded and it works), I don't think it quite makes sense to apply the fs permissions here.

[cjihrig]

I think this should be a separate permission (similar to child process and worker) with its own flag. When the permission system is enabled, access to FFI should be restricted unless the --allow-ffi flag is provided.

[RafaelGSS]

I think this should be a separate permission (similar to child process and worker) with its own flag. When the permission system is enabled, access to FFI should be restricted unless the --allow-ffi flag is provided.

Exactly. The safest option is to disable it by default when using --experimental-permission. Happy to help implement it. Feel free to ping me on OpenJS slack.

[bengl]

@RafaelGSS In the most recent rev, I have it behaving exactly as the worker and child process permissions do (that is, it's only disabled if an fs permission is enabled). It's easy enough to change it so that it's disabled even when only --experimental-permission is provided. That's what, you're suggesting, right?

[RafaelGSS]

@bengl I've just seen the patch. All good.

[bnoordhuis]

More of a practical matter: I suggest removing unused big files from libffi (like the ChangeLog files) to avoid bloating the git tree and the source tarballs.

[gireeshpunathil]

related art (from Java): https://openjdk.org/projects/panama/

[bengl]

A big remaining question is what to do about maintaining and building libffi. Compiling it requires three header files that are generated by its ./configure script. Attempting to build the headers as if cross compiling (as this PR does today does in the updater script) doesn't work, since incorrect assumptions are made about the target system if building the headers elsewhere. Therefore, no matter what approach is taken, ./configure must be run in deps/libffi for each platform Node.js support, at some point. As far as I can tell, we have the following options.

---

Option 1

As is done in ffi-napi, update PRs can include instructions for building the headers. The PR author asks for volunteers to come in and add the headers for their respective platforms.

While this avoids calling the ./configure at Node.js' build time, it increases the people-overhead of updating libffi.

Option 2

In either libffi.gyp or the top-level ./configure, or somewhere else, the ./configure for deps/ffi can be called (or Windows equivalent), so that it's always part of the build process for Node.js.

While this almost completely automates the process of updating libffi, it introduces the concept of running the ./configure script for a dependency at build time, which, AFAIK, is not something that's already done today anywhere else in Node.js.

Option 3

A job can be created in https://ci.nodejs.org that runs the ./configure script on all supported platforms, and adds the headers to a PR. This is basically option 1 but with automated/robot volunteers.

While this doesn't suffer the drawbacks of the previous two options, it does require some help from the build team to set it all up in the first place, and probably wouldn't help for this, the initial PR.

---

My preference is for option 2, because it seems likely that if volunteers need to be rounded up for each libffi update (or indeed, even this PR), then PRs are likely to stall. In addition, the more of the process that can be automated the better. I'd consider option 3 to be my second preference here, but that still requires a bunch of additional setup, and, again, probably won't be in place for this PR.

What are your thoughts? Also, please feel free to correct my assumptions on any of these 😄.

[bnoordhuis]

In either libffi.gyp or the top-level ./configure, or somewhere else, the ./configure for deps/ffi can be called (or Windows equivalent), so that it's always part of the build process for Node.js.

That adds a dependency on autotools (autoconf, automake, m4, etc.) and that's just... no. Just say no.

[RafaelGSS]

In either libffi.gyp or the top-level ./configure, or somewhere else, the ./configure for deps/ffi can be called (or Windows equivalent), so that it's always part of the build process for Node.js.

That adds a dependency on autotools (autoconf, automake, m4, etc.) and that's just... no. Just say no.

Would you mind elaborating on that? I talked with @bengl yesterday and I don't see a really strong objection/downside to this.

[targos]

I think that would at least make the PR semver-major because of the new build requirements.

[richardlau]

How often is the config likely to change? I think we have a similar thing with c-ares, and those are just checked in and, AFAIK, haven't been updated in a while.

I am not keen on starting down the path where we need additional build tools to build dependencies -- it will introduce an extra barrier to contribution (you now need to install extra things) and the situation re. tool availability and version of those tools is not consistent across all of the platforms we support. For example, we do not require perl (required to generate OpenSSL's config).

As for option 3, I'd prefer to avoid giving the test servers the ability to push code to github.

[bengl]

How often is the config likely to change? I think we have a similar thing with c-ares, and those are just checked in and, AFAIK, haven't been updated in a while.

The best example we have is probably this commit on ffi-napi. There are a lot of changes, but it's also a big version jump, IIUC. I had a look at the updating instructions for c-ares, and updating the config directory doesn't seem to be mentioned. I guess the assumption for c-ares is that the generated file never change (or at least never need to)? Based on the ffi-napi commit, I'm not sure that applies here.

the situation re. tool availability and version of those tools is not consistent across all of the platforms we support

libffi is supported on all platforms Node.js is, AFAICT, so I doubt that would be a problem, modulo installing those tools. That being said, I certainly understand what everyone is saying about adding extra build dependencies (including that it would be semver-major), so I's say option 2 is ruled out at this point.

As for option 3, I'd prefer to avoid giving the test servers the ability to push code to github.

These could be build artifacts instead?

[atgreen]

Hi -- I'm the author of libffi, and I'm very happy to see node picking this up.

I have yet to have a chance to review this patch in detail, but I do want to proactively warn you about something that many new libffi users stumble on. It has to do with small return values. From the libffi manual:

rvalue is a pointer to a chunk of memory that will hold the result of
the function call. This must be large enough to hold the result, no
smaller than the system register size (generally 32 or 64 bits), and
must be suitably aligned; it is the caller’s responsibility to ensure
this. If cif declares that the function returns void (using
ffi_type_void), then rvalue is ignored.

In most situations, ‘libffi’ will handle promotion according to the
ABI. However, for historical reasons, there is a special case with
return values that must be handled by your code. In particular, for
integral (not struct) types that are narrower than the system register
size, the return value will be widened by ‘libffi’. ‘libffi’ provides
a type, ffi_arg, that can be used as the return type. For example, if
the CIF was defined with a return type of char, ‘libffi’ will try to
store a full ffi_arg into the return value.

There are lots of examples in the testsuite.

Also, are you sure your code handles double return types correctly? I am not a javascript expert, let alone a node internals expert, but does this code assume return values are no larger than the size of a pointer (which could potentially be < sizeof(double))?

[nodejs-github-bot]

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

[mcollina]

I don't see why this should be a semver major change. Backporting to v20 would be a great win.

[atgreen]

Looks like this didn't make 21. Let me know if there's anything I can help with from the libffi side. I expect to make a new release in the next few weeks.

[github-actions[bot]]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[github-actions[bot]]

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

[Pomax]

@aduh95 @GeoffreyBooth @jasnell did this fall off the radar? A bot closing this PR seems like the wrong thing to happen here.