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.

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

node-api: handle no support for external buffers #45181

Closed
wants to merge 11 commits into from
27 changes: 27 additions & 0 deletions doc/api/n-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,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 @@ -2403,6 +2404,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 @@ -2447,6 +2461,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
Original file line number Diff line number Diff line change
Expand Up @@ -401,13 +401,15 @@ NAPI_EXTERN napi_status NAPI_CDECL napi_create_arraybuffer(napi_env env,
size_t byte_length,
void** data,
napi_value* result);
#ifndef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
Copy link
Member

@legendecas legendecas Oct 27, 2022

Choose a reason for hiding this comment

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

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?

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

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 fixed and added to the two flavors of test_general

NAPI_EXTERN napi_status NAPI_CDECL
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_CDECL napi_get_arraybuffer_info(
napi_env env, napi_value arraybuffer, void** data, size_t* byte_length);
NAPI_EXTERN napi_status NAPI_CDECL napi_is_typedarray(napi_env env,
Expand Down
3 changes: 2 additions & 1 deletion src/js_native_api_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,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()'
mhdawson marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
3 changes: 2 additions & 1 deletion src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,7 @@ static const char* error_messages[] = {
"An arraybuffer was expected",
"A detachable arraybuffer was expected",
"Main thread would deadlock",
"External buffers are not allowed",
};

napi_status NAPI_CDECL napi_get_last_error_info(
Expand All @@ -757,7 +758,7 @@ napi_status NAPI_CDECL napi_get_last_error_info(
// 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,
"Count of error messages must match count of error values");
Expand Down
4 changes: 4 additions & 0 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -950,6 +950,10 @@ napi_status NAPI_CDECL napi_create_external_buffer(napi_env env,
NAPI_PREAMBLE(env);
CHECK_ARG(env, result);

#if defined(V8_ENABLE_SANDBOX)
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
Original file line number Diff line number Diff line change
Expand Up @@ -153,13 +153,15 @@ NAPI_EXTERN napi_status NAPI_CDECL 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_CDECL
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_CDECL 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
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
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