From e29b2c0ed130fcee48671400877f0175230e1636 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 25 Oct 2022 17:39:41 -0400 Subject: [PATCH 01/11] node-api: handle no support for external buffers Refs: https://github.com/electron/electron/issues/35801 Refs: https://github.com/nodejs/abi-stable-node/issues/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 --- doc/api/n-api.md | 13 +++++++++++++ src/js_native_api.h | 2 ++ src/js_native_api_types.h | 3 ++- src/node_api.cc | 4 ++++ src/node_api.h | 2 ++ 5 files changed, 23 insertions(+), 1 deletion(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 3a2b9bceb2ab64..b450717f4d5232 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -2362,6 +2362,19 @@ This API allocates a JavaScript value with external data attached to it. This is used to pass external data through JavaScript code, so it can be retrieved later by native code using [`napi_get_value_external`][]. +**Some runtimes other than Node.ja hasve 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 methods +that create external buffers. This will ensure a compilation error +occurs if you accidentally use one of these methods. + The API adds a `napi_finalize` callback which will be called when the JavaScript object just created is ready for garbage collection. It is similar to `napi_wrap()` except that: diff --git a/src/js_native_api.h b/src/js_native_api.h index 220d140d4bfe9a..9c99813f4fc88e 100644 --- a/src/js_native_api.h +++ b/src/js_native_api.h @@ -402,12 +402,14 @@ NAPI_EXTERN napi_status NAPI_CDECL napi_create_arraybuffer(napi_env env, void** data, napi_value* result); NAPI_EXTERN napi_status NAPI_CDECL +#ifndef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED 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, diff --git a/src/js_native_api_types.h b/src/js_native_api_types.h index 376930ba4a3220..517614950a90f7 100644 --- a/src/js_native_api_types.h +++ b/src/js_native_api_types.h @@ -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()' diff --git a/src/node_api.cc b/src/node_api.cc index 48b94a7c12873c..792322634d2c2b 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -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_COMPRESS_POINTERS_IN_SHARED_CAGE) + 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. diff --git a/src/node_api.h b/src/node_api.h index 3dc17f31f68778..8e6af2cf284e85 100644 --- a/src/node_api.h +++ b/src/node_api.h @@ -153,6 +153,7 @@ 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, @@ -160,6 +161,7 @@ napi_create_external_buffer(napi_env env, 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, From 9d67a665a530422993fce6002283821f2d0a1fc1 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Wed, 26 Oct 2022 15:39:45 -0400 Subject: [PATCH 02/11] Update doc/api/n-api.md Co-authored-by: Chengzhong Wu --- doc/api/n-api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index b450717f4d5232..25095fc4f78c42 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -2362,7 +2362,7 @@ This API allocates a JavaScript value with external data attached to it. This is used to pass external data through JavaScript code, so it can be retrieved later by native code using [`napi_get_value_external`][]. -**Some runtimes other than Node.ja hasve dropped support for external buffers**. +**Some runtimes other than Node.ja 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 From ac56ab00a78badf8207700083d4912ab3819edee Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Wed, 26 Oct 2022 15:47:12 -0400 Subject: [PATCH 03/11] Update n-api.md --- doc/api/n-api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 25095fc4f78c42..7b688472fa1e5f 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -2371,7 +2371,7 @@ described in this issue 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 methods +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. From 5ba9934db59c884f201dc1f4ec97ca26ff68b8d7 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Thu, 27 Oct 2022 16:12:55 -0400 Subject: [PATCH 04/11] Update js_native_api.h --- src/js_native_api.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/js_native_api.h b/src/js_native_api.h index 9c99813f4fc88e..d11a2c5a18cf16 100644 --- a/src/js_native_api.h +++ b/src/js_native_api.h @@ -401,8 +401,8 @@ NAPI_EXTERN napi_status NAPI_CDECL napi_create_arraybuffer(napi_env env, size_t byte_length, void** data, napi_value* result); -NAPI_EXTERN napi_status NAPI_CDECL #ifndef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED +NAPI_EXTERN napi_status NAPI_CDECL napi_create_external_arraybuffer(napi_env env, void* external_data, size_t byte_length, From ebefb2b26945b8ffaa241260b769236ec55c9398 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Fri, 28 Oct 2022 14:20:59 -0400 Subject: [PATCH 05/11] squash: address review comments Signed-off-by: Michael Dawson --- src/js_native_api_v8.cc | 3 ++- test/js-native-api/test_general/test_general.c | 5 +++++ test/node-api/test_general/test_general.c | 5 +++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 58567c5e44a9e7..9000175d9d869a 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -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( @@ -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"); diff --git a/test/js-native-api/test_general/test_general.c b/test/js-native-api/test_general/test_general.c index 7b755ce9a9f202..840a2522d57091 100644 --- a/test/js-native-api/test_general/test_general.c +++ b/test/js-native-api/test_general/test_general.c @@ -1,3 +1,8 @@ +// we define this 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 #include #include diff --git a/test/node-api/test_general/test_general.c b/test/node-api/test_general/test_general.c index d430e2df4f3520..d160376868d2f2 100644 --- a/test/node-api/test_general/test_general.c +++ b/test/node-api/test_general/test_general.c @@ -1,4 +1,9 @@ #define NAPI_EXPERIMENTAL +// we define this 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 #include #include "../../js-native-api/common.h" From a8038ac364fccdfc315bd73d50ffc4b38671aa2f Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Mon, 31 Oct 2022 14:20:22 -0400 Subject: [PATCH 06/11] Update doc/api/n-api.md Co-authored-by: Robert Nagy --- doc/api/n-api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 7b688472fa1e5f..b8c03ee043736e 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -2362,7 +2362,7 @@ This API allocates a JavaScript value with external data attached to it. This is used to pass external data through JavaScript code, so it can be retrieved later by native code using [`napi_get_value_external`][]. -**Some runtimes other than Node.ja have dropped support for external buffers**. +**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 From e66382b82dcd3d0f6403cd509df41a28deb03f5d Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 1 Nov 2022 10:03:37 -0400 Subject: [PATCH 07/11] squash: switch define used to check for cage Signed-off-by: Michael Dawson --- src/node_api.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_api.cc b/src/node_api.cc index 792322634d2c2b..15c224dc92768b 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -950,7 +950,7 @@ napi_status NAPI_CDECL napi_create_external_buffer(napi_env env, NAPI_PREAMBLE(env); CHECK_ARG(env, result); -#if defined(V8_COMPRESS_POINTERS_IN_SHARED_CAGE) +#if defined(V8_ENABLE_SANDBOX) return napi_set_last_error(env, napi_no_external_buffers_allowed); #endif From 15a43d6092810bed2546fc07bedba3e3edcb471f Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 1 Nov 2022 11:48:35 -0400 Subject: [PATCH 08/11] address review comments --- doc/api/n-api.md | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index b8c03ee043736e..0afc747d13f5e6 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -2362,19 +2362,6 @@ This API allocates a JavaScript value with external data attached to it. This is used to pass external data through JavaScript code, so it can be retrieved later by native code using [`napi_get_value_external`][]. -**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. - The API adds a `napi_finalize` callback which will be called when the JavaScript object just created is ready for garbage collection. It is similar to `napi_wrap()` except that: @@ -2416,6 +2403,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 @@ -2460,6 +2460,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. From d3f0252b940a7973c4b471613247fcb9e5dab0ec Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 1 Nov 2022 11:49:04 -0400 Subject: [PATCH 09/11] address review comments --- test/node-api/test_general/test_general.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/node-api/test_general/test_general.c b/test/node-api/test_general/test_general.c index d160376868d2f2..b8d837d5e45650 100644 --- a/test/node-api/test_general/test_general.c +++ b/test/node-api/test_general/test_general.c @@ -1,5 +1,5 @@ #define NAPI_EXPERIMENTAL -// we define this here to validate that it can +// 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 From 874cac1aa9a63185d2af5d9856a808f56bd00c32 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Wed, 2 Nov 2022 09:49:39 -0400 Subject: [PATCH 10/11] squash: address review comments --- test/js-native-api/test_general/test_general.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/js-native-api/test_general/test_general.c b/test/js-native-api/test_general/test_general.c index 840a2522d57091..b474ab442cb763 100644 --- a/test/js-native-api/test_general/test_general.c +++ b/test/js-native-api/test_general/test_general.c @@ -1,5 +1,5 @@ -// we define this here to validate that it can -// be used as a form of test itself. It is +// 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 From 981f9aed29b42e4c863f662f1eed8a3f67f72d11 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Fri, 4 Nov 2022 13:08:54 -0400 Subject: [PATCH 11/11] squash: address comments Signed-off-by: Michael Dawson --- doc/api/n-api.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 0afc747d13f5e6..b0b89de1c75bf6 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -579,6 +579,7 @@ typedef enum { napi_arraybuffer_expected, napi_detachable_arraybuffer_expected, napi_would_deadlock, /* unused */ + napi_no_external_buffers_allowed } napi_status; ```