From db55c3cfc14775cda810efc58bb8d7c1b7c356c4 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sat, 6 Jul 2019 22:09:52 +0200
Subject: [PATCH] worker: fix passing multiple SharedArrayBuffers at once

V8 has a handle scope below each `GetSharedArrayBufferId()` call,
so using a `v8::Local` that outlives that handle scope to store
references to `SharedArrayBuffer`s is invalid and may cause accidental
de-duplication of passed `SharedArrayBuffer`s.

Use a persistent handle instead to address this issue.

Fixes: https://github.com/nodejs/node/issues/28559

PR-URL: https://github.com/nodejs/node/pull/28582
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 src/node_messaging.cc                           | 10 +++++++---
 ...-message-port-multiple-sharedarraybuffers.js | 17 +++++++++++++++++
 2 files changed, 24 insertions(+), 3 deletions(-)
 create mode 100644 test/parallel/test-worker-message-port-multiple-sharedarraybuffers.js

diff --git a/src/node_messaging.cc b/src/node_messaging.cc
index 7a0f2db8830978..46f06b747e312f 100644
--- a/src/node_messaging.cc
+++ b/src/node_messaging.cc
@@ -19,6 +19,7 @@ using v8::Exception;
 using v8::Function;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
+using v8::Global;
 using v8::HandleScope;
 using v8::Isolate;
 using v8::Just;
@@ -241,8 +242,10 @@ class SerializerDelegate : public ValueSerializer::Delegate {
       Local<SharedArrayBuffer> shared_array_buffer) override {
     uint32_t i;
     for (i = 0; i < seen_shared_array_buffers_.size(); ++i) {
-      if (seen_shared_array_buffers_[i] == shared_array_buffer)
+      if (PersistentToLocal::Strong(seen_shared_array_buffers_[i]) ==
+          shared_array_buffer) {
         return Just(i);
+      }
     }
 
     auto reference = SharedArrayBufferMetadata::ForSharedArrayBuffer(
@@ -252,7 +255,8 @@ class SerializerDelegate : public ValueSerializer::Delegate {
     if (!reference) {
       return Nothing<uint32_t>();
     }
-    seen_shared_array_buffers_.push_back(shared_array_buffer);
+    seen_shared_array_buffers_.emplace_back(
+      Global<SharedArrayBuffer> { isolate, shared_array_buffer });
     msg_->AddSharedArrayBuffer(reference);
     return Just(i);
   }
@@ -289,7 +293,7 @@ class SerializerDelegate : public ValueSerializer::Delegate {
   Environment* env_;
   Local<Context> context_;
   Message* msg_;
-  std::vector<Local<SharedArrayBuffer>> seen_shared_array_buffers_;
+  std::vector<Global<SharedArrayBuffer>> seen_shared_array_buffers_;
   std::vector<MessagePort*> ports_;
 
   friend class worker::Message;
diff --git a/test/parallel/test-worker-message-port-multiple-sharedarraybuffers.js b/test/parallel/test-worker-message-port-multiple-sharedarraybuffers.js
new file mode 100644
index 00000000000000..eb68236e4d6788
--- /dev/null
+++ b/test/parallel/test-worker-message-port-multiple-sharedarraybuffers.js
@@ -0,0 +1,17 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const { MessageChannel } = require('worker_threads');
+
+// Regression test for https://github.com/nodejs/node/issues/28559
+
+const obj = [
+  [ new SharedArrayBuffer(0), new SharedArrayBuffer(1) ],
+  [ new SharedArrayBuffer(2), new SharedArrayBuffer(3) ]
+];
+
+const { port1, port2 } = new MessageChannel();
+port1.once('message', common.mustCall((message) => {
+  assert.deepStrictEqual(message, obj);
+}));
+port2.postMessage(obj);