From d4e397a90047b6f9f973c4c5e33dae4b59960400 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 18 Aug 2019 00:05:48 +0200 Subject: [PATCH] worker: fix crash when SharedArrayBuffer outlives creating thread Keep a reference to the `ArrayBuffer::Allocator` alive for at least as long as a `SharedArrayBuffer` allocated by it lives. Refs: https://github.com/nodejs/node/pull/28788 Fixes: https://github.com/nodejs/node/issues/28777 Fixes: https://github.com/nodejs/node/issues/28773 PR-URL: https://github.com/nodejs/node/pull/29190 Reviewed-By: James M Snell Reviewed-By: Colin Ihrig --- src/node_worker.cc | 13 +++++--- src/node_worker.h | 2 ++ src/sharedarraybuffer_metadata.cc | 17 ++++++++-- src/sharedarraybuffer_metadata.h | 5 ++- .../test-worker-arraybuffer-zerofill.js | 33 +++++++++++++++++++ ...er-sharedarraybuffer-from-worker-thread.js | 22 +++++++++++++ 6 files changed, 83 insertions(+), 9 deletions(-) create mode 100644 test/parallel/test-worker-arraybuffer-zerofill.js create mode 100644 test/parallel/test-worker-sharedarraybuffer-from-worker-thread.js diff --git a/src/node_worker.cc b/src/node_worker.cc index 8f97f5c3514639..11e44a92757e41 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -59,6 +59,7 @@ Worker::Worker(Environment* env, per_isolate_opts_(per_isolate_opts), exec_argv_(exec_argv), platform_(env->isolate_data()->platform()), + array_buffer_allocator_(ArrayBufferAllocator::Create()), start_profiler_idle_notifier_(env->profiler_idle_notifier_started()), thread_id_(Environment::AllocateThreadId()), env_vars_(env->env_vars()) { @@ -102,17 +103,20 @@ bool Worker::is_stopped() const { return stopped_; } +std::shared_ptr Worker::array_buffer_allocator() { + return array_buffer_allocator_; +} + // This class contains data that is only relevant to the child thread itself, // and only while it is running. // (Eventually, the Environment instance should probably also be moved here.) class WorkerThreadData { public: explicit WorkerThreadData(Worker* w) - : w_(w), - array_buffer_allocator_(ArrayBufferAllocator::Create()) { + : w_(w) { CHECK_EQ(uv_loop_init(&loop_), 0); - Isolate* isolate = NewIsolate(array_buffer_allocator_.get(), &loop_); + Isolate* isolate = NewIsolate(w->array_buffer_allocator_.get(), &loop_); CHECK_NOT_NULL(isolate); { @@ -124,7 +128,7 @@ class WorkerThreadData { isolate_data_.reset(CreateIsolateData(isolate, &loop_, w_->platform_, - array_buffer_allocator_.get())); + w->array_buffer_allocator_.get())); CHECK(isolate_data_); if (w_->per_isolate_opts_) isolate_data_->set_options(std::move(w_->per_isolate_opts_)); @@ -166,7 +170,6 @@ class WorkerThreadData { private: Worker* const w_; uv_loop_t loop_; - std::unique_ptr array_buffer_allocator_; DeleteFnPtr isolate_data_; friend class Worker; diff --git a/src/node_worker.h b/src/node_worker.h index db3c95d2aef965..ffc4f19882cc26 100644 --- a/src/node_worker.h +++ b/src/node_worker.h @@ -41,6 +41,7 @@ class Worker : public AsyncWrap { SET_SELF_SIZE(Worker) bool is_stopped() const; + std::shared_ptr array_buffer_allocator(); static void New(const v8::FunctionCallbackInfo& args); static void CloneParentEnvVars( @@ -59,6 +60,7 @@ class Worker : public AsyncWrap { std::vector argv_; MultiIsolatePlatform* platform_; + std::shared_ptr array_buffer_allocator_; v8::Isolate* isolate_ = nullptr; bool start_profiler_idle_notifier_; uv_thread_t tid_; diff --git a/src/sharedarraybuffer_metadata.cc b/src/sharedarraybuffer_metadata.cc index 9ba604b506788a..b9d86d05ff20f3 100644 --- a/src/sharedarraybuffer_metadata.cc +++ b/src/sharedarraybuffer_metadata.cc @@ -1,7 +1,9 @@ #include "sharedarraybuffer_metadata.h" #include "base_object-inl.h" +#include "memory_tracker-inl.h" #include "node_errors.h" +#include "node_worker.h" #include "util-inl.h" #include @@ -91,8 +93,16 @@ SharedArrayBufferMetadata::ForSharedArrayBuffer( return nullptr; } + // If the SharedArrayBuffer is coming from a Worker, we need to make sure + // that the corresponding ArrayBuffer::Allocator lives at least as long as + // the SharedArrayBuffer itself. + worker::Worker* w = env->worker_context(); + std::shared_ptr allocator = + w != nullptr ? w->array_buffer_allocator() : nullptr; + SharedArrayBuffer::Contents contents = source->Externalize(); - SharedArrayBufferMetadataReference r(new SharedArrayBufferMetadata(contents)); + SharedArrayBufferMetadataReference r( + new SharedArrayBufferMetadata(contents, allocator)); if (r->AssignToSharedArrayBuffer(env, context, source).IsNothing()) return nullptr; return r; @@ -114,8 +124,9 @@ Maybe SharedArrayBufferMetadata::AssignToSharedArrayBuffer( } SharedArrayBufferMetadata::SharedArrayBufferMetadata( - const SharedArrayBuffer::Contents& contents) - : contents_(contents) { } + const SharedArrayBuffer::Contents& contents, + std::shared_ptr allocator) + : contents_(contents), allocator_(allocator) { } SharedArrayBufferMetadata::~SharedArrayBufferMetadata() { contents_.Deleter()(contents_.Data(), diff --git a/src/sharedarraybuffer_metadata.h b/src/sharedarraybuffer_metadata.h index 8c753a89c11a88..8da603978a990b 100644 --- a/src/sharedarraybuffer_metadata.h +++ b/src/sharedarraybuffer_metadata.h @@ -46,7 +46,9 @@ class SharedArrayBufferMetadata SharedArrayBufferMetadata(const SharedArrayBufferMetadata&) = delete; private: - explicit SharedArrayBufferMetadata(const v8::SharedArrayBuffer::Contents&); + SharedArrayBufferMetadata( + const v8::SharedArrayBuffer::Contents&, + std::shared_ptr); // Attach a lifetime tracker object with a reference count to `target`. v8::Maybe AssignToSharedArrayBuffer( @@ -55,6 +57,7 @@ class SharedArrayBufferMetadata v8::Local target); v8::SharedArrayBuffer::Contents contents_; + std::shared_ptr allocator_; }; } // namespace worker diff --git a/test/parallel/test-worker-arraybuffer-zerofill.js b/test/parallel/test-worker-arraybuffer-zerofill.js new file mode 100644 index 00000000000000..3dcf4c006ebcd9 --- /dev/null +++ b/test/parallel/test-worker-arraybuffer-zerofill.js @@ -0,0 +1,33 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const { Worker } = require('worker_threads'); + +// Make sure that allocating uninitialized ArrayBuffers in one thread does not +// affect the zero-initialization in other threads. + +const w = new Worker(` +const { parentPort } = require('worker_threads'); + +function post() { + const uint32array = new Uint32Array(64); + parentPort.postMessage(uint32array.reduce((a, b) => a + b)); +} + +setInterval(post, 0); +`, { eval: true }); + +function allocBuffers() { + Buffer.allocUnsafe(32 * 1024 * 1024); +} + +const interval = setInterval(allocBuffers, 0); + +let messages = 0; +w.on('message', (sum) => { + assert.strictEqual(sum, 0); + if (messages++ === 100) { + clearInterval(interval); + w.terminate(); + } +}); diff --git a/test/parallel/test-worker-sharedarraybuffer-from-worker-thread.js b/test/parallel/test-worker-sharedarraybuffer-from-worker-thread.js new file mode 100644 index 00000000000000..60e8a5d52ab5bf --- /dev/null +++ b/test/parallel/test-worker-sharedarraybuffer-from-worker-thread.js @@ -0,0 +1,22 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const { Worker } = require('worker_threads'); + +// Regression test for https://github.com/nodejs/node/issues/28777 +// Make sure that SharedArrayBuffers created in Worker threads are accessible +// after the creating thread ended. + +const w = new Worker(` +const { parentPort } = require('worker_threads'); +const sharedArrayBuffer = new SharedArrayBuffer(4); +parentPort.postMessage(sharedArrayBuffer); +`, { eval: true }); + +let sharedArrayBuffer; +w.once('message', common.mustCall((message) => sharedArrayBuffer = message)); +w.once('exit', common.mustCall(() => { + const uint8array = new Uint8Array(sharedArrayBuffer); + uint8array[0] = 42; + assert.deepStrictEqual(uint8array, new Uint8Array([42, 0, 0, 0])); +}));