worker: do not crash when JSTransferable lists untransferable value

This can currently be triggered when posting a closing FileHandle.

Refs: https://github.com/nodejs/node/pull/34746#issuecomment-673675333

PR-URL: https://github.com/nodejs/node/pull/34766
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

2 files changed, 45 insertions, 5 deletions

diff --git a/src/node_messaging.cc b/src/node_messaging.cc
index c6152935704..a072176523e 100644
--- a/src/node_messaging.cc
+++ b/src/node_messaging.cc
@@ -343,6 +343,12 @@ class SerializerDelegate : public ValueSerializer::Delegate {
 
  private:
   Maybe<bool> WriteHostObject(BaseObjectPtr<BaseObject> host_object) {
+    BaseObject::TransferMode mode = host_object->GetTransferMode();
+    if (mode == BaseObject::TransferMode::kUntransferable) {
+      ThrowDataCloneError(env_->clone_unsupported_type_str());
+      return Nothing<bool>();
+    }
+
     for (uint32_t i = 0; i < host_objects_.size(); i++) {
       if (host_objects_[i] == host_object) {
         serializer->WriteUint32(i);
@@ -350,11 +356,7 @@ class SerializerDelegate : public ValueSerializer::Delegate {
       }
     }
 
-    BaseObject::TransferMode mode = host_object->GetTransferMode();
-    if (mode == BaseObject::TransferMode::kUntransferable) {
-      ThrowDataCloneError(env_->clone_unsupported_type_str());
-      return Nothing<bool>();
-    } else if (mode == BaseObject::TransferMode::kTransferable) {
+    if (mode == BaseObject::TransferMode::kTransferable) {
       THROW_ERR_MISSING_TRANSFERABLE_IN_TRANSFER_LIST(env_);
       return Nothing<bool>();
     }
diff --git a/test/parallel/test-worker-message-port-jstransferable-nested-untransferable.js b/test/parallel/test-worker-message-port-jstransferable-nested-untransferable.js
new file mode 100644
index 00000000000..621c42be514
--- /dev/null
+++ b/test/parallel/test-worker-message-port-jstransferable-nested-untransferable.js
@@ -0,0 +1,38 @@
+// Flags: --expose-internals --no-warnings
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const {
+  JSTransferable, kTransfer, kTransferList
+} = require('internal/worker/js_transferable');
+const { MessageChannel } = require('worker_threads');
+
+// Transferring a JSTransferable that refers to another, untransferable, value
+// in its transfer list should not crash hard.
+
+class OuterTransferable extends JSTransferable {
+  constructor() {
+    super();
+    // Create a detached MessagePort at this.inner
+    const c = new MessageChannel();
+    this.inner = c.port1;
+    c.port2.postMessage(this.inner, [ this.inner ]);
+  }
+
+  [kTransferList] = common.mustCall(() => {
+    return [ this.inner ];
+  });
+
+  [kTransfer] = common.mustCall(() => {
+    return {
+      data: { inner: this.inner },
+      deserializeInfo: 'does-not:matter'
+    };
+  });
+}
+
+const { port1 } = new MessageChannel();
+const ot = new OuterTransferable();
+assert.throws(() => {
+  port1.postMessage(ot, [ot]);
+}, { name: 'DataCloneError' });