From 9d60499bfb56b5509e7532ad028f935fdc93ab45 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 13 Nov 2019 19:19:54 +0000 Subject: [PATCH] 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 PR-URL: https://github.com/nodejs/node/pull/30475 Backport-PR-URL: https://github.com/nodejs/node/pull/31355 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: David Carlier --- 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 | 10 +++++++ 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(+), 4 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 1ae707428230e8..8b690c09067135 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 f29adeb8f8db61..b90db8f3e8ce86 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -2614,6 +2614,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 8eed25bc9e6f77..0d5424cccaddf0 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 20b591747de2d0..0f7981cd5605ae 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 0cbc441c3cd34a..e80604e2c06bd8 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -364,10 +364,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)); } @@ -385,6 +383,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 19065fdb7d1be5..b0e7d15a8c40a6 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -329,6 +329,16 @@ Maybe Message::Serialize(Environment* env, !env->isolate_data()->uses_node_allocator()) { continue; } + // See https://github.com/nodejs/node/pull/30339#issuecomment-552225353 + // for details. + 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..b04984f1576432 --- /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 } = new MessageChannel(); +const origByteLength = buffer.byteLength; +port1.postMessage(buffer, [buffer.buffer]); + +assert.strictEqual(buffer.byteLength, origByteLength); +assert.notStrictEqual(buffer.byteLength, 0);