Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v14.x] node-api: handle no support for external buffers #45616

Closed
wants to merge 1 commit into from
Closed

[v14.x] node-api: handle no support for external buffers #45616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions doc/api/n-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,7 @@ typedef enum {
napi_arraybuffer_expected,
napi_detachable_arraybuffer_expected,
napi_would_deadlock, /* unused */
napi_no_external_buffers_allowed
} napi_status;
```

Expand Down Expand Up @@ -2252,6 +2253,19 @@ napi_create_external_arraybuffer(napi_env env,

Returns `napi_ok` if the API succeeded.

**Some runtimes other than Node.js have dropped support for external buffers**.
On runtimes other than Node.js this method may return
`napi_no_external_buffers_allowed` to indicate that external
buffers are not supported. One such runtime is Electron as
described in this issue
[electron/issues/35801](/~https://github.com/electron/electron/issues/35801).

In order to maintain broadest compatibility with all runtimes
you may define `NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED` in your addon before
includes for the node-api headers. Doing so will hide the 2 functions
that create external buffers. This will ensure a compilation error
occurs if you accidentally use one of these methods.

This API returns a Node-API value corresponding to a JavaScript `ArrayBuffer`.
The underlying byte buffer of the `ArrayBuffer` is externally allocated and
managed. The caller must ensure that the byte buffer remains valid until the
Expand Down Expand Up @@ -2295,6 +2309,19 @@ napi_status napi_create_external_buffer(napi_env env,

Returns `napi_ok` if the API succeeded.

**Some runtimes other than Node.js have dropped support for external buffers**.
On runtimes other than Node.js this method may return
`napi_no_external_buffers_allowed` to indicate that external
buffers are not supported. One such runtime is Electron as
described in this issue
[electron/issues/35801](/~https://github.com/electron/electron/issues/35801).

In order to maintain broadest compatibility with all runtimes
you may define `NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED` in your addon before
includes for the node-api headers. Doing so will hide the 2 functions
that create external buffers. This will ensure a compilation error
occurs if you accidentally use one of these methods.

This API allocates a `node::Buffer` object and initializes it with data
backed by the passed in buffer. While this is still a fully-supported data
structure, in most cases using a `TypedArray` will suffice.
Expand Down
2 changes: 2 additions & 0 deletions src/js_native_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,13 +387,15 @@ NAPI_EXTERN napi_status napi_create_arraybuffer(napi_env env,
size_t byte_length,
void** data,
napi_value* result);
#ifndef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
NAPI_EXTERN napi_status
napi_create_external_arraybuffer(napi_env env,
void* external_data,
size_t byte_length,
napi_finalize finalize_cb,
void* finalize_hint,
napi_value* result);
#endif // NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
NAPI_EXTERN napi_status napi_get_arraybuffer_info(napi_env env,
napi_value arraybuffer,
void** data,
Expand Down
3 changes: 2 additions & 1 deletion src/js_native_api_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ typedef enum {
napi_date_expected,
napi_arraybuffer_expected,
napi_detachable_arraybuffer_expected,
napi_would_deadlock // unused
napi_would_deadlock, // unused
napi_no_external_buffers_allowed
} napi_status;
// Note: when adding a new enum value to `napi_status`, please also update
// * `const int last_status` in the definition of `napi_get_last_error_info()'
Expand Down
3 changes: 2 additions & 1 deletion src/js_native_api_v8.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,6 +744,7 @@ const char* error_messages[] = {nullptr,
"An arraybuffer was expected",
"A detachable arraybuffer was expected",
"Main thread would deadlock",
"External buffers are not allowed",
};

napi_status napi_get_last_error_info(napi_env env,
Expand All @@ -755,7 +756,7 @@ napi_status napi_get_last_error_info(napi_env env,
// message in the `napi_status` enum each time a new error message is added.
// We don't have a napi_status_last as this would result in an ABI
// change each time a message was added.
const int last_status = napi_would_deadlock;
const int last_status = napi_no_external_buffers_allowed;

static_assert(
NAPI_ARRAYSIZE(error_messages) == last_status + 1,
Expand Down
4 changes: 4 additions & 0 deletions src/node_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,6 +928,10 @@ napi_status napi_create_external_buffer(napi_env env,
NAPI_PREAMBLE(env);
CHECK_ARG(env, result);

#if defined(V8_ENABLE_SANDBOX)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't find this definition on v14.x. Maybe we can simply remove this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could make sense, I think we wanted the backport so that people could compile on the oldest LTS and use the value for the error code.

They question is whether it's better to keep it closer to the original PR even if the define will never be set or to remove it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richardlau do you have an opinion from the Releaser perspective? I'm happy either way. We can remove the define or leave it in to make it a "backport" versus a "backport" with modifications whichever is easier/preferrable for getting it landed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhdawson No preference either way. I think it's very unlikely V8_ENABLE_SANDBOX would be introduced to Node.js 14 if it isn't already there given that Node.js 14 is in maintenance and will go End-of-Life in April.

cc @nodejs/lts

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legendecas since @richardlau figures there is no preference from the Releasers perspective I don't don't feel strongly either. I'm happy to leave as it as I don't think it hurts anything but also happy to remove that i you think that's best.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with both ways. Thanks for the clarification.

return napi_set_last_error(env, napi_no_external_buffers_allowed);
#endif

v8::Isolate* isolate = env->isolate;

// The finalizer object will delete itself after invoking the callback.
Expand Down
2 changes: 2 additions & 0 deletions src/node_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,14 @@ NAPI_EXTERN napi_status napi_create_buffer(napi_env env,
size_t length,
void** data,
napi_value* result);
#ifndef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
NAPI_EXTERN napi_status napi_create_external_buffer(napi_env env,
size_t length,
void* data,
napi_finalize finalize_cb,
void* finalize_hint,
napi_value* result);
#endif // NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
NAPI_EXTERN napi_status napi_create_buffer_copy(napi_env env,
size_t length,
const void* data,
Expand Down
5 changes: 5 additions & 0 deletions test/js-native-api/test_general/test_general.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
// we define NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED here to
// validate that it can be used as a form of test itself. It is
// not related to any of the other tests
// defined in the file
#define NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
Expand Down
5 changes: 5 additions & 0 deletions test/node-api/test_general/test_general.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
#define NAPI_EXPERIMENTAL
// we define NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED here to validate that it can
// be used as a form of test itself. It is
// not related to any of the other tests
// defined in the file
#define NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
#include <node_api.h>
#include <stdlib.h>
#include "../../js-native-api/common.h"
Expand Down