worker: fix crash when SharedArrayBuffer outlives creating thread

addaleax · targos · commit d4e397a90047 · 2019-08-20T09:10:16.000+02:00
Keep a reference to the `ArrayBuffer::Allocator` alive for at least as long as a `SharedArrayBuffer` allocated by it lives. Refs: #28788 Fixes: #28777 Fixes: #28773 PR-URL: #29190 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
diff --git 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<ArrayBufferAllocator> 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<ArrayBufferAllocator> array_buffer_allocator_;
   DeleteFnPtr<IsolateData, FreeIsolateData> isolate_data_;
 
   friend class Worker;
diff --git 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<ArrayBufferAllocator> array_buffer_allocator();
 
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void CloneParentEnvVars(
@@ -59,6 +60,7 @@ class Worker : public AsyncWrap {
   std::vector<std::string> argv_;
 
   MultiIsolatePlatform* platform_;
+  std::shared_ptr<ArrayBufferAllocator> 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
@@ -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 <utility>
@@ -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<v8::ArrayBuffer::Allocator> 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<bool> SharedArrayBufferMetadata::AssignToSharedArrayBuffer(
 }
 
 SharedArrayBufferMetadata::SharedArrayBufferMetadata(
-    const SharedArrayBuffer::Contents& contents)
-  : contents_(contents) { }
+    const SharedArrayBuffer::Contents& contents,
+    std::shared_ptr<v8::ArrayBuffer::Allocator> allocator)
+  : contents_(contents), allocator_(allocator) { }
 
 SharedArrayBufferMetadata::~SharedArrayBufferMetadata() {
   contents_.Deleter()(contents_.Data(),
diff --git 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<v8::ArrayBuffer::Allocator>);
 
   // Attach a lifetime tracker object with a reference count to `target`.
   v8::Maybe<bool> AssignToSharedArrayBuffer(
@@ -55,6 +57,7 @@ class SharedArrayBufferMetadata
       v8::Local<v8::SharedArrayBuffer> target);
 
   v8::SharedArrayBuffer::Contents contents_;
+  std::shared_ptr<v8::ArrayBuffer::Allocator> allocator_;
 };
 
 }  // namespace worker
diff --git a/test/parallel/test-worker-arraybuffer-zerofill.js 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
@@ -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]));
+}));
