From bf7d850edf8dcb1125b6293213ec26e1aa2dc2fd Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Fri, 14 Aug 2020 00:04:18 +0200
Subject: [PATCH] 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
---
 src/node_messaging.cc                         | 12 +++---
 ...rt-jstransferable-nested-untransferable.js | 38 +++++++++++++++++++
 2 files changed, 45 insertions(+), 5 deletions(-)
 create mode 100644 test/parallel/test-worker-message-port-jstransferable-nested-untransferable.js

diff --git a/src/node_messaging.cc b/src/node_messaging.cc
index c615293570475c..a072176523e1e9 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 00000000000000..621c42be5149cd
--- /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' });