From 44da3b3986f9bb5eb055150456ad99452bd010b5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 13 Nov 2019 19:19:54 +0000 Subject: [PATCH 1/2] src: mark ArrayBuffers with free callbacks as untransferable More precisely, make them untransferable if they were created through *our* APIs, because those do not follow the improved free callback mechanism that V8 uses now. All other ArrayBuffers can be transferred between threads now, the assumption being that they were created in a clean way that follows the V8 API on this. This addresses a TODO comment. Refs: https://github.com/nodejs/node/pull/30339#issuecomment-552225353 --- src/env.h | 1 + src/js_native_api_v8.cc | 2 ++ src/js_native_api_v8.h | 4 +++ src/node_api.cc | 8 +++++ src/node_buffer.cc | 12 +++++--- src/node_messaging.cc | 14 ++++++--- test/addons/worker-buffer-callback/binding.cc | 29 +++++++++++++++++++ .../addons/worker-buffer-callback/binding.gyp | 9 ++++++ test/addons/worker-buffer-callback/test.js | 15 ++++++++++ 9 files changed, 86 insertions(+), 8 deletions(-) create mode 100644 test/addons/worker-buffer-callback/binding.cc create mode 100644 test/addons/worker-buffer-callback/binding.gyp create mode 100644 test/addons/worker-buffer-callback/test.js diff --git a/src/env.h b/src/env.h index 5f5188d49d6052..105dd90d2ca469 100644 --- a/src/env.h +++ b/src/env.h @@ -151,6 +151,7 @@ constexpr size_t kFsStatsBufferLength = // "node:" prefix to avoid name clashes with third-party code. #define PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) \ V(alpn_buffer_private_symbol, "node:alpnBuffer") \ + V(arraybuffer_untransferable_private_symbol, "node:untransferableBuffer") \ V(arrow_message_private_symbol, "node:arrowMessage") \ V(contextify_context_private_symbol, "node:contextify:context") \ V(contextify_global_private_symbol, "node:contextify:global") \ diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index ef1edb92eed6fa..6484afaaac629e 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -2581,6 +2581,8 @@ napi_status napi_create_external_arraybuffer(napi_env env, v8::Isolate* isolate = env->isolate; v8::Local buffer = v8::ArrayBuffer::New(isolate, external_data, byte_length); + v8::Maybe marked = env->mark_arraybuffer_as_untransferable(buffer); + CHECK_MAYBE_NOTHING(env, marked, napi_generic_failure); if (finalize_cb != nullptr) { // Create a self-deleting weak reference that invokes the finalizer diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 2e0a7a1d6add20..534b09851f8c8e 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -39,6 +39,10 @@ struct napi_env__ { inline void Unref() { if ( --refs == 0) delete this; } virtual bool can_call_into_js() const { return true; } + virtual v8::Maybe mark_arraybuffer_as_untransferable( + v8::Local ab) const { + return v8::Just(true); + } template void CallIntoModule(T&& call, U&& handle_exception) { diff --git a/src/node_api.cc b/src/node_api.cc index 95664e9c7ace27..b293272b911c44 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -24,6 +24,14 @@ struct node_napi_env__ : public napi_env__ { bool can_call_into_js() const override { return node_env()->can_call_into_js(); } + + v8::Maybe mark_arraybuffer_as_untransferable( + v8::Local ab) const { + return ab->SetPrivate( + context(), + node_env()->arraybuffer_untransferable_private_symbol(), + v8::True(isolate)); + } }; typedef node_napi_env__* node_napi_env; diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 2dbdd61923f026..3cd047c0e7bf41 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -350,10 +350,8 @@ MaybeLocal New(Isolate* isolate, THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate); return MaybeLocal(); } - Local obj; - if (Buffer::New(env, data, length, callback, hint).ToLocal(&obj)) - return handle_scope.Escape(obj); - return Local(); + return handle_scope.EscapeMaybe( + Buffer::New(env, data, length, callback, hint)); } @@ -371,6 +369,12 @@ MaybeLocal New(Environment* env, } Local ab = ArrayBuffer::New(env->isolate(), data, length); + if (ab->SetPrivate(env->context(), + env->arraybuffer_untransferable_private_symbol(), + True(env->isolate())).IsNothing()) { + callback(data, hint); + return Local(); + } MaybeLocal ui = Buffer::New(env, ab, 0, length); CallbackInfo::New(env->isolate(), ab, callback, data, hint); diff --git a/src/node_messaging.cc b/src/node_messaging.cc index c54958a4cf6e93..1028da69b44961 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -306,11 +306,17 @@ Maybe Message::Serialize(Environment* env, // raw data *and* an Isolate with a non-default ArrayBuffer allocator // is always going to outlive any Workers it creates, and so will its // allocator along with it. - // TODO(addaleax): Eventually remove the IsExternal() condition, - // see https://github.com/nodejs/node/pull/30339#issuecomment-552225353 + if (!ab->IsDetachable()) continue; + // See https://github.com/nodejs/node/pull/30339#issuecomment-552225353 // for details. - if (!ab->IsDetachable() || ab->IsExternal()) - continue; + bool untransferrable; + if (!ab->HasPrivate( + context, + env->arraybuffer_untransferable_private_symbol()) + .To(&untransferrable)) { + return Nothing(); + } + if (untransferrable) continue; if (std::find(array_buffers.begin(), array_buffers.end(), ab) != array_buffers.end()) { ThrowDataCloneException( diff --git a/test/addons/worker-buffer-callback/binding.cc b/test/addons/worker-buffer-callback/binding.cc new file mode 100644 index 00000000000000..a40876ebb523a6 --- /dev/null +++ b/test/addons/worker-buffer-callback/binding.cc @@ -0,0 +1,29 @@ +#include +#include +#include + +using v8::Context; +using v8::Isolate; +using v8::Local; +using v8::Object; +using v8::Value; + +char data[] = "hello"; + +void Initialize(Local exports, + Local module, + Local context) { + Isolate* isolate = context->GetIsolate(); + exports->Set(context, + v8::String::NewFromUtf8( + isolate, "buffer", v8::NewStringType::kNormal) + .ToLocalChecked(), + node::Buffer::New( + isolate, + data, + sizeof(data), + [](char* data, void* hint) {}, + nullptr).ToLocalChecked()).Check(); +} + +NODE_MODULE_CONTEXT_AWARE(NODE_GYP_MODULE_NAME, Initialize) diff --git a/test/addons/worker-buffer-callback/binding.gyp b/test/addons/worker-buffer-callback/binding.gyp new file mode 100644 index 00000000000000..55fbe7050f18e4 --- /dev/null +++ b/test/addons/worker-buffer-callback/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'sources': [ 'binding.cc' ], + 'includes': ['../common.gypi'], + } + ] +} diff --git a/test/addons/worker-buffer-callback/test.js b/test/addons/worker-buffer-callback/test.js new file mode 100644 index 00000000000000..3849390e2b5ec6 --- /dev/null +++ b/test/addons/worker-buffer-callback/test.js @@ -0,0 +1,15 @@ +'use strict'; +const common = require('../../common'); +const assert = require('assert'); +const { MessageChannel } = require('worker_threads'); +const { buffer } = require(`./build/${common.buildType}/binding`); + +// Test that buffers allocated with a free callback through our APIs are not +// transfered. + +const { port1, port2 } = new MessageChannel(); +const origByteLength = buffer.byteLength; +port1.postMessage(buffer, [buffer.buffer]); + +assert.strictEqual(buffer.byteLength, origByteLength); +assert.notStrictEqual(buffer.byteLength, 0); From 47ff8a9414ec64970b49d6fe5e8d685db68f3efd Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 18 Nov 2019 13:20:58 +0100 Subject: [PATCH 2/2] fixup! src: mark ArrayBuffers with free callbacks as untransferable --- test/addons/worker-buffer-callback/test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/addons/worker-buffer-callback/test.js b/test/addons/worker-buffer-callback/test.js index 3849390e2b5ec6..b04984f1576432 100644 --- a/test/addons/worker-buffer-callback/test.js +++ b/test/addons/worker-buffer-callback/test.js @@ -7,7 +7,7 @@ const { buffer } = require(`./build/${common.buildType}/binding`); // Test that buffers allocated with a free callback through our APIs are not // transfered. -const { port1, port2 } = new MessageChannel(); +const { port1 } = new MessageChannel(); const origByteLength = buffer.byteLength; port1.postMessage(buffer, [buffer.buffer]);