From 64549731b6cfe3729cb42896ab1f8229b62ff968 Mon Sep 17 00:00:00 2001
From: Chengzhong Wu <legendecas@gmail.com>
Date: Fri, 5 May 2023 19:22:42 +0800
Subject: [PATCH] src: throw DataCloneError on transfering untransferable
 objects

The HTML StructuredSerializeWithTransfer algorithm defines that when
an untransferable object is in the transfer list, a DataCloneError is
thrown.
An array buffer that is already transferred is also considered as
untransferable.

PR-URL: https://github.com/nodejs/node/pull/47604
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
---
 doc/api/worker_threads.md                     | 44 +++++++++++++++++--
 lib/internal/buffer.js                        | 10 +++++
 lib/internal/modules/esm/worker.js            | 16 +++++--
 lib/worker_threads.js                         |  2 +
 src/env_properties.h                          |  4 +-
 src/node_messaging.cc                         | 16 ++++---
 test/addons/worker-buffer-callback/test.js    |  5 ++-
 .../test_worker_buffer_callback/test.js       |  5 ++-
 .../test-buffer-pool-untransferable.js        |  5 ++-
 .../test-worker-message-port-arraybuffer.js   |  7 +++
 ...est-worker-message-port-transfer-native.js |  2 +-
 ...ge-transfer-port-mark-as-untransferable.js | 34 +++++++++++---
 12 files changed, 126 insertions(+), 24 deletions(-)

diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md
index f3f500c9075058..09b695eb96ea29 100644
--- a/doc/api/worker_threads.md
+++ b/doc/api/worker_threads.md
@@ -130,8 +130,11 @@ added:
   - v12.19.0
 -->
 
+* `object` {any} Any arbitrary JavaScript value.
+
 Mark an object as not transferable. If `object` occurs in the transfer list of
-a [`port.postMessage()`][] call, it is ignored.
+a [`port.postMessage()`][] call, an error is thrown. This is a no-op if
+`object` is a primitive value.
 
 In particular, this makes sense for objects that can be cloned, rather than
 transferred, and which are used by other objects on the sending side.
@@ -150,11 +153,17 @@ const typedArray2 = new Float64Array(pooledBuffer);
 markAsUntransferable(pooledBuffer);
 
 const { port1 } = new MessageChannel();
-port1.postMessage(typedArray1, [ typedArray1.buffer ]);
+try {
+  // This will throw an error, because pooledBuffer is not transferable.
+  port1.postMessage(typedArray1, [ typedArray1.buffer ]);
+} catch (error) {
+  // error.name === 'DataCloneError'
+}
 
 // The following line prints the contents of typedArray1 -- it still owns
-// its memory and has been cloned, not transferred. Without
-// `markAsUntransferable()`, this would print an empty Uint8Array.
+// its memory and has not been transferred. Without
+// `markAsUntransferable()`, this would print an empty Uint8Array and the
+// postMessage call would have succeeded.
 // typedArray2 is intact as well.
 console.log(typedArray1);
 console.log(typedArray2);
@@ -162,6 +171,29 @@ console.log(typedArray2);
 
 There is no equivalent to this API in browsers.
 
+## `worker.isMarkedAsUntransferable(object)`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+* `object` {any} Any JavaScript value.
+* Returns: {boolean}
+
+Check if an object is marked as not transferable with
+[`markAsUntransferable()`][].
+
+```js
+const { markAsUntransferable, isMarkedAsUntransferable } = require('node:worker_threads');
+
+const pooledBuffer = new ArrayBuffer(8);
+markAsUntransferable(pooledBuffer);
+
+isMarkedAsUntransferable(pooledBuffer);  // Returns true.
+```
+
+There is no equivalent to this API in browsers.
+
 ## `worker.moveMessagePortToContext(port, contextifiedSandbox)`
 
 <!-- YAML
@@ -568,6 +600,10 @@ are part of the channel.
 <!-- YAML
 added: v10.5.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/47604
+    description: An error is thrown when an untransferable object is in the
+                 transfer list.
   - version:
       - v15.14.0
       - v14.18.0
diff --git a/lib/internal/buffer.js b/lib/internal/buffer.js
index fbe9de249348b3..a65dd43263714e 100644
--- a/lib/internal/buffer.js
+++ b/lib/internal/buffer.js
@@ -1053,6 +1053,15 @@ function markAsUntransferable(obj) {
   obj[untransferable_object_private_symbol] = true;
 }
 
+// This simply checks if the object is marked as untransferable and doesn't
+// check whether we are able to transfer it.
+function isMarkedAsUntransferable(obj) {
+  if (obj == null)
+    return false;
+  // Private symbols are not inherited.
+  return obj[untransferable_object_private_symbol] !== undefined;
+}
+
 // A toggle used to access the zero fill setting of the array buffer allocator
 // in C++.
 // |zeroFill| can be undefined when running inside an isolate where we
@@ -1079,6 +1088,7 @@ module.exports = {
   FastBuffer,
   addBufferPrototypeMethods,
   markAsUntransferable,
+  isMarkedAsUntransferable,
   createUnsafeBuffer,
   readUInt16BE,
   readUInt32BE,
diff --git a/lib/internal/modules/esm/worker.js b/lib/internal/modules/esm/worker.js
index 616076c98fb627..9b531c3231b54e 100644
--- a/lib/internal/modules/esm/worker.js
+++ b/lib/internal/modules/esm/worker.js
@@ -30,13 +30,21 @@ const {
   WORKER_TO_MAIN_THREAD_NOTIFICATION,
 } = require('internal/modules/esm/shared_constants');
 const { initializeHooks } = require('internal/modules/esm/utils');
-
+const { isMarkedAsUntransferable } = require('internal/buffer');
 
 function transferArrayBuffer(hasError, source) {
   if (hasError || source == null) return;
-  if (isArrayBuffer(source)) return [source];
-  if (isTypedArray(source)) return [TypedArrayPrototypeGetBuffer(source)];
-  if (isDataView(source)) return [DataViewPrototypeGetBuffer(source)];
+  let arrayBuffer;
+  if (isArrayBuffer(source)) {
+    arrayBuffer = source;
+  } else if (isTypedArray(source)) {
+    arrayBuffer = TypedArrayPrototypeGetBuffer(source);
+  } else if (isDataView(source)) {
+    arrayBuffer = DataViewPrototypeGetBuffer(source);
+  }
+  if (arrayBuffer && !isMarkedAsUntransferable(arrayBuffer)) {
+    return [arrayBuffer];
+  }
 }
 
 function wrapMessage(status, body) {
diff --git a/lib/worker_threads.js b/lib/worker_threads.js
index 155cc11ecdaf43..084fa98908dc57 100644
--- a/lib/worker_threads.js
+++ b/lib/worker_threads.js
@@ -20,6 +20,7 @@ const {
 
 const {
   markAsUntransferable,
+  isMarkedAsUntransferable,
 } = require('internal/buffer');
 
 module.exports = {
@@ -27,6 +28,7 @@ module.exports = {
   MessagePort,
   MessageChannel,
   markAsUntransferable,
+  isMarkedAsUntransferable,
   moveMessagePortToContext,
   receiveMessageOnPort,
   resourceLimits,
diff --git a/src/env_properties.h b/src/env_properties.h
index 320d08d22577c8..3ef589397dbdcc 100644
--- a/src/env_properties.h
+++ b/src/env_properties.h
@@ -67,7 +67,7 @@
   V(change_string, "change")                                                   \
   V(channel_string, "channel")                                                 \
   V(chunks_sent_since_last_write_string, "chunksSentSinceLastWrite")           \
-  V(clone_unsupported_type_str, "Cannot transfer object of unsupported type.") \
+  V(clone_unsupported_type_str, "Cannot clone object of unsupported type.")    \
   V(code_string, "code")                                                       \
   V(commonjs_string, "commonjs")                                               \
   V(config_string, "config")                                                   \
@@ -302,6 +302,8 @@
   V(time_to_first_header_string, "timeToFirstHeader")                          \
   V(tls_ticket_string, "tlsTicket")                                            \
   V(transfer_string, "transfer")                                               \
+  V(transfer_unsupported_type_str,                                             \
+    "Cannot transfer object of unsupported type.")                             \
   V(ttl_string, "ttl")                                                         \
   V(type_string, "type")                                                       \
   V(uid_string, "uid")                                                         \
diff --git a/src/node_messaging.cc b/src/node_messaging.cc
index 2d126d98441bad..d7d5848bfbab9e 100644
--- a/src/node_messaging.cc
+++ b/src/node_messaging.cc
@@ -459,11 +459,14 @@ Maybe<bool> Message::Serialize(Environment* env,
               .To(&untransferable)) {
         return Nothing<bool>();
       }
-      if (untransferable) continue;
+      if (untransferable) {
+        ThrowDataCloneException(context, env->transfer_unsupported_type_str());
+        return Nothing<bool>();
+      }
     }
 
     // Currently, we support ArrayBuffers and BaseObjects for which
-    // GetTransferMode() does not return kUntransferable.
+    // GetTransferMode() returns kTransferable.
     if (entry->IsArrayBuffer()) {
       Local<ArrayBuffer> ab = entry.As<ArrayBuffer>();
       // If we cannot render the ArrayBuffer unusable in this Isolate,
@@ -474,7 +477,10 @@ Maybe<bool> 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.
-      if (!ab->IsDetachable()) continue;
+      if (!ab->IsDetachable() || ab->WasDetached()) {
+        ThrowDataCloneException(context, env->transfer_unsupported_type_str());
+        return Nothing<bool>();
+      }
       if (std::find(array_buffers.begin(), array_buffers.end(), ab) !=
           array_buffers.end()) {
         ThrowDataCloneException(
@@ -524,8 +530,8 @@ Maybe<bool> Message::Serialize(Environment* env,
                 entry.As<Object>()->GetConstructorName()));
         return Nothing<bool>();
       }
-      if (host_object && host_object->GetTransferMode() !=
-              BaseObject::TransferMode::kUntransferable) {
+      if (host_object && host_object->GetTransferMode() ==
+                             BaseObject::TransferMode::kTransferable) {
         delegate.AddHostObject(host_object);
         continue;
       }
diff --git a/test/addons/worker-buffer-callback/test.js b/test/addons/worker-buffer-callback/test.js
index 08625b72384eb1..3194fdcb375965 100644
--- a/test/addons/worker-buffer-callback/test.js
+++ b/test/addons/worker-buffer-callback/test.js
@@ -9,7 +9,10 @@ const { buffer } = require(`./build/${common.buildType}/binding`);
 
 const { port1 } = new MessageChannel();
 const origByteLength = buffer.byteLength;
-port1.postMessage(buffer, [buffer.buffer]);
+assert.throws(() => port1.postMessage(buffer, [buffer.buffer]), {
+  code: 25,
+  name: 'DataCloneError',
+});
 
 assert.strictEqual(buffer.byteLength, origByteLength);
 assert.notStrictEqual(buffer.byteLength, 0);
diff --git a/test/node-api/test_worker_buffer_callback/test.js b/test/node-api/test_worker_buffer_callback/test.js
index 4884a27d39e6a2..04b9249c684eab 100644
--- a/test/node-api/test_worker_buffer_callback/test.js
+++ b/test/node-api/test_worker_buffer_callback/test.js
@@ -9,7 +9,10 @@ const { buffer } = require(`./build/${common.buildType}/binding`);
 
 const { port1 } = new MessageChannel();
 const origByteLength = buffer.byteLength;
-port1.postMessage(buffer, [buffer]);
+assert.throws(() => port1.postMessage(buffer, [buffer]), {
+  code: 25,
+  name: 'DataCloneError',
+});
 
 assert.strictEqual(buffer.byteLength, origByteLength);
 assert.notStrictEqual(buffer.byteLength, 0);
diff --git a/test/parallel/test-buffer-pool-untransferable.js b/test/parallel/test-buffer-pool-untransferable.js
index d39b03435fa5b7..596bb6b6c91422 100644
--- a/test/parallel/test-buffer-pool-untransferable.js
+++ b/test/parallel/test-buffer-pool-untransferable.js
@@ -13,7 +13,10 @@ assert.strictEqual(a.buffer, b.buffer);
 const length = a.length;
 
 const { port1 } = new MessageChannel();
-port1.postMessage(a, [ a.buffer ]);
+assert.throws(() => port1.postMessage(a, [ a.buffer ]), {
+  code: 25,
+  name: 'DataCloneError',
+});
 
 // Verify that the pool ArrayBuffer has not actually been transferred:
 assert.strictEqual(a.buffer, b.buffer);
diff --git a/test/parallel/test-worker-message-port-arraybuffer.js b/test/parallel/test-worker-message-port-arraybuffer.js
index 247efa3fdf143b..a30378674a311d 100644
--- a/test/parallel/test-worker-message-port-arraybuffer.js
+++ b/test/parallel/test-worker-message-port-arraybuffer.js
@@ -12,6 +12,13 @@ const { MessageChannel } = require('worker_threads');
   typedArray[0] = 0x12345678;
 
   port1.postMessage(typedArray, [ arrayBuffer ]);
+  assert.strictEqual(arrayBuffer.byteLength, 0);
+  // Transferring again should throw a DataCloneError.
+  assert.throws(() => port1.postMessage(typedArray, [ arrayBuffer ]), {
+    code: 25,
+    name: 'DataCloneError',
+  });
+
   port2.on('message', common.mustCall((received) => {
     assert.strictEqual(received[0], 0x12345678);
     port2.close(common.mustCall());
diff --git a/test/parallel/test-worker-message-port-transfer-native.js b/test/parallel/test-worker-message-port-transfer-native.js
index eb51b2b01af1a3..6990e48e5cb801 100644
--- a/test/parallel/test-worker-message-port-transfer-native.js
+++ b/test/parallel/test-worker-message-port-transfer-native.js
@@ -31,7 +31,7 @@ const { internalBinding } = require('internal/test/binding');
     port1.postMessage(nativeObject);
   }, {
     name: 'DataCloneError',
-    message: /Cannot transfer object of unsupported type\.$/
+    message: /Cannot clone object of unsupported type\.$/
   });
   port1.close();
 }
diff --git a/test/parallel/test-worker-message-transfer-port-mark-as-untransferable.js b/test/parallel/test-worker-message-transfer-port-mark-as-untransferable.js
index 240928263871a4..94ae4e8c5fd237 100644
--- a/test/parallel/test-worker-message-transfer-port-mark-as-untransferable.js
+++ b/test/parallel/test-worker-message-transfer-port-mark-as-untransferable.js
@@ -1,19 +1,22 @@
 'use strict';
 const common = require('../common');
 const assert = require('assert');
-const { MessageChannel, markAsUntransferable } = require('worker_threads');
+const { MessageChannel, markAsUntransferable, isMarkedAsUntransferable } = require('worker_threads');
 
 {
   const ab = new ArrayBuffer(8);
 
   markAsUntransferable(ab);
+  assert.ok(isMarkedAsUntransferable(ab));
   assert.strictEqual(ab.byteLength, 8);
 
-  const { port1, port2 } = new MessageChannel();
-  port1.postMessage(ab, [ ab ]);
+  const { port1 } = new MessageChannel();
+  assert.throws(() => port1.postMessage(ab, [ ab ]), {
+    code: 25,
+    name: 'DataCloneError',
+  });
 
   assert.strictEqual(ab.byteLength, 8);  // The AB is not detached.
-  port2.once('message', common.mustCall());
 }
 
 {
@@ -21,17 +24,36 @@ const { MessageChannel, markAsUntransferable } = require('worker_threads');
   const channel2 = new MessageChannel();
 
   markAsUntransferable(channel2.port1);
+  assert.ok(isMarkedAsUntransferable(channel2.port1));
 
   assert.throws(() => {
     channel1.port1.postMessage(channel2.port1, [ channel2.port1 ]);
-  }, /was found in message but not listed in transferList/);
+  }, {
+    code: 25,
+    name: 'DataCloneError',
+  });
 
   channel2.port1.postMessage('still works, not closed/transferred');
   channel2.port2.once('message', common.mustCall());
 }
 
 {
-  for (const value of [0, null, false, true, undefined, [], {}]) {
+  for (const value of [0, null, false, true, undefined]) {
     markAsUntransferable(value);  // Has no visible effect.
+    assert.ok(!isMarkedAsUntransferable(value));
   }
+  for (const value of [[], {}]) {
+    markAsUntransferable(value);
+    assert.ok(isMarkedAsUntransferable(value));
+  }
+}
+
+{
+  // Verifies that the mark is not inherited.
+  class Foo {}
+  markAsUntransferable(Foo.prototype);
+  assert.ok(isMarkedAsUntransferable(Foo.prototype));
+
+  const foo = new Foo();
+  assert.ok(!isMarkedAsUntransferable(foo));
 }