node-api: handle no support for external buffers

[mhdawson]

Refs: electron/electron#35801 Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:

• hide the methods to create external buffers so they can avoid using them if they want the broadest compatibility.
• call the methods that create external buffers at runtime to check if external buffers are supported and either use them or not based on the return code.

Signed-off-by: Michael Dawson mdawson@devrus.com

[nodejs-github-bot]

Review requested:

• @nodejs/node-api

[mhdawson]

I've manually tested the new status code and define by hacking our addon tests

To add a test to our test suite to test the status code we would need a build with pointer compression turned on as well as the pointer cage turned on. We don't build in our regular build/test in our CI so no easy way to add that test that I can think off.

To add a test to our test suite to test the #define we'd need to build an addon that would fail to compile, looking at the addon tests I don't think we do anything like that either. @legendecas I know you added some more complicated tests for some harder cases, any of those offer a possible answer to adding a test in this case?

[MarshallOfSound]

@mhdawson I think this was briefly mentioned in the Electron thread this started in but would it be reasonable to add a method that does this transparently for addon authors.

These changes are good because they (a) prevent crashes and (b) allow addon authors to opt out of external buffers completely. But I think the ideal for an addon author is they use a method like

napi_create_or_copy_external_buffer

which internally maps to

napi_status NAPI_CDECL napi_create_or_copy_external_buffer(napi_env env,
                                                   size_t length,
                                                   void* data,
                                                   napi_finalize finalize_cb,
                                                   void* finalize_hint,
                                                   napi_value* result) {
#if defined(V8_COMPRESS_POINTERS_IN_SHARED_CAGE)
  return napi_create_buffer(env, length, data, result);
#else
  return napi_create_external_buffer(env, length, data, finalize_cb, finalize_hint, result);
#fi
}

Not sure if it's reasonable to drop finalization info in the cage scenario but with that method addon authors could then just use napi_create_or_copy_external_buffer and in node it would be external and as performant as possible and in environments with the shared cage enabled it would transparently copy the contents of data.

There might be other implications I'm missing though 🤔

[Julusian]

What versions of nodejs are we likely to see this in? Will it need to be released as part of node-api v9, as it adds a new value to an enum?

@MarshallOfSound a wrapper like that would be nice, but in my opinion could also live in node-addon-api. It will be easy to implement on top of this change by checking the return value instead of using an ifdef

[legendecas]

External array buffers have JavaScript observable differences with the copying semantics. JavaScript code can directly see the changes in the memory of the external array buffer and modify it without calling into the add-on. Implicitly copying with a function like napi_create_or_copy_external_buffer invalidates this behavior -- the change on the external buffer are no longer observable from the copied array buffer, and changes to the copied array buffer are not reflected in the external buffer.

So I'm -1 on a new function like napi_create_or_copy_external_buffer coded in the node-api. Rather, add-on authors can choose the best course for their own distinct use case, and a helper function is fairly straightforward to be added if they do not care about direct memory access from JavaScript.

[legendecas]

I've manually tested the new status code and define by hacking our addon tests
To add a test to our test suite to test the status code we would need a build with pointer compression turned on as well as the pointer cage turned on. We don't build in our regular build/test in our CI so no easy way to add that test that I can think off.

As Electron is embedding Node.js with pointer compression and the sandboxed pointers, maybe it would be worth setting up a CI job to verify Node.js with pointer compression and the sandboxed pointers (which is enabled by default if pointer compression is enabled) too?

[mhdawson]

@lovell adding a new method would definitely require a new Node-API version, would spend some time as experimental, and that node-API version would need to make it through the backporting process to earlier versions (which is easier for more recent Node.js verions, harder for older ones).

I think the changes in this PR may not require a new Node-API version (@nodejs/node-api keep me honest on that) and therefore might be easier to backport. Having said that, until it was backported to all current LTS versions a crash could occur on some versions so maybe only fixing under a new Node-API version makes more sense.

All that makes me think we might want to keep adding a new method to be in a separate PR. I guess the question is if these changes solve the problem at all on their own or not.

[lovell]

@mhdawson The change proposed in this PR is sufficient to detect the presence of the V8 memory cage at runtime, thank you. 👍

My original thoughts around adding an explicit Napi::Buffer::NewOrCopy() method were that it might be useful albeit syntactic sugar for possible inclusion in the node-addon-api wrapper rather than Node-API itself.

[legendecas]

This line should be moved one line up, above the NAPI_EXTERN napi_status NAPI_CDECL, as it is part of the declaration.

Maybe we should simply add a testing addon that defines NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED and it can correctly compile to verify that the new macro doesn't introduce any flaws?

[mhdawson]

@legendecas thats a good idea, I had manually tested defining it, but I only checked that I got an error, not that would not get an error. I'll plan to add that.

[mhdawson]

@legendecas fixed and added to the two flavors of test_general

[mhdawson]

@lovell

@mhdawson The change proposed in this PR is sufficient to detect the presence of the V8 memory cage at runtime, thank you.

Thanks I'll continue to move this PR forward and then once we get it landed, I'll explore adding the additional method you suggest, it does seem like something that could make sense as an addition in a next step.

[mhdawson]

What versions of nodejs are we likely to see this in? Will it need to be released as part of node-api v9, as it adds a new value to an enum?

We will need to discuss this further. We've never guarded the error codes based on Node-API version or EXPERIMENTAL. In the case of EXPERIMENTAL ones there could be a situation where an EXPERIMENTAL return code was before one that was released and we could never remove that one or hide it as that would mess up the enum ordering.

So in that context, since error codes can be present without a guard in newever versions anyway, it does not add a lot of value to try to force a Node-API version update when adding it. I can see the argument for it needing one since if you compile with a check for it, and depend on it not to crash then maybe it should be. Practically though, its probably better if we don't tie it to a Node-API versions and try to backport quicklyish.

@nodejs/node-api what are your thoughts?

[vmoroz]

@mhdawson, per our discussion in Node-API meeting on 10/28/2022 I am adding a few links to Electron code where they implemented the support for the caged pointers. There were several PRs, but the changes to Node.JS code are mostly in these two patch files (the related PRs can be seen in their history):

• https://github.com/electron/electron/blob/main/patches/node/support_v8_sandboxed_pointers.patch
• https://github.com/electron/electron/blob/main/patches/node/build_ensure_v8_pointer_compression_sandbox_is_enabled_on_64bit.patch

We most probably do not need these changes for the Node-API part in this PR, but we might consider them if we ever want to add the V8 pointer cage mode to Node.JS.

I guess the most important question is to understand if we must use V8_ENABLE_SANDBOX or the V8_COMPRESS_POINTERS_IN_SHARED_CAGE macro. In the patch files they use V8_ENABLE_SANDBOX to modify the Node.JS behavior.

[legendecas]

v8_enable_sandbox requires v8_enable_pointer_compression_shared_cage to be set, as asserted in BUILD.gn.

I found the definition of V8_ENABLE_SANDBOX is used more widely in the codebase, e.g.
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api.cc;l=360?q=V8_ENABLE_SANDBOX.

[mhdawson]

@john-yang looks like we might need to enable V8_ENABLE_SANDBOX instead of V8_COMPRESS_POINTERS_IN_SHARED_CAGE, does that make sense to you?

[john-yan]

I agree that checking V8_ENABLE_SANDBOX might make more sense here based on the error returned. V8_COMPRESS_POINTERS_IN_SHARED_CAGE macro is used to check if pointer cage feature is enabled. It doesn't directly relate to external buffers.

[mhdawson]

@richardlau thanks for the heads up, working on it.

[mhdawson]

@richardlau backport PR - #45616