worker: do not crash when JSTransferable lists untransferable value

This can currently be triggered when posting a closing FileHandle. Refs: #34746 (comment) PR-URL: #34766 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax · Aug 13, 2020 · Aug 17, 2020 · 2aa2331c3aa14eab27cf75b70c0941f8908f8fdb
commit 2aa2331c3aa14eab27cf75b70c0941f8908f8fdb
diff --git a/src/node_messaging.cc b/src/node_messaging.cc
@@ -342,18 +342,20 @@ 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);
         return Just(true);
       }
     }
 
-    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) {
       // TODO(addaleax): This message code is too specific. Fix that in a
       // semver-major follow-up.
       THROW_ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST(env_);

diff --git a/test/parallel/test-worker-message-port-jstransferable-nested-untransferable.js 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' });