From 598356820459b1c3b32f7647c418278a6430619e Mon Sep 17 00:00:00 2001
From: Santiago Gimeno <santiago.gimeno@gmail.com>
Date: Sun, 25 Apr 2021 18:42:47 +0200
Subject: [PATCH] worker: avoid potential deadlock on NearHeapLimit
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It can happen that the `NearHeapLimit` callback is called while calling
the `oninit()` function on `MessagePort` construction causing a deadlock
when the `Worker::Exit()` method is called, as the `mutex_` was already
held on the `CreateEnvMessagePort()` method. To fix it, just use the
`mutex_` to protect the `child_port_data_` variable and avoid holding it
when creating the `MessagePort`.
Also, return early from `Worker::Run()` if the worker message port
could not be created.

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

PR-URL: https://github.com/nodejs/node/pull/38403
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
---
 src/node_worker.cc                            | 18 +++++++++++----
 src/node_worker.h                             |  2 +-
 .../test-worker-nearheaplimit-deadlock.js     | 22 +++++++++++++++++++
 3 files changed, 37 insertions(+), 5 deletions(-)
 create mode 100644 test/parallel/test-worker-nearheaplimit-deadlock.js

diff --git a/src/node_worker.cc b/src/node_worker.cc
index 3195ddbaf01b87..dcc76ad88f04e2 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -328,7 +328,10 @@ void Worker::Run() {
       Debug(this, "Created Environment for worker with id %llu", thread_id_.id);
       if (is_stopped()) return;
       {
-        CreateEnvMessagePort(env_.get());
+        if (!CreateEnvMessagePort(env_.get())) {
+          return;
+        }
+
         Debug(this, "Created message port for worker %llu", thread_id_.id);
         if (LoadEnvironment(env_.get(), StartExecutionCallback{}).IsEmpty())
           return;
@@ -352,17 +355,24 @@ void Worker::Run() {
   Debug(this, "Worker %llu thread stops", thread_id_.id);
 }
 
-void Worker::CreateEnvMessagePort(Environment* env) {
+bool Worker::CreateEnvMessagePort(Environment* env) {
   HandleScope handle_scope(isolate_);
-  Mutex::ScopedLock lock(mutex_);
+  std::unique_ptr<MessagePortData> data;
+  {
+    Mutex::ScopedLock lock(mutex_);
+    data = std::move(child_port_data_);
+  }
+
   // Set up the message channel for receiving messages in the child.
   MessagePort* child_port = MessagePort::New(env,
                                              env->context(),
-                                             std::move(child_port_data_));
+                                             std::move(data));
   // MessagePort::New() may return nullptr if execution is terminated
   // within it.
   if (child_port != nullptr)
     env->set_message_port(child_port->object(isolate_));
+
+  return child_port;
 }
 
 void Worker::JoinThread() {
diff --git a/src/node_worker.h b/src/node_worker.h
index 2c65f0e1a83bbe..077d2b8390e6f8 100644
--- a/src/node_worker.h
+++ b/src/node_worker.h
@@ -70,7 +70,7 @@ class Worker : public AsyncWrap {
   static void LoopStartTime(const v8::FunctionCallbackInfo<v8::Value>& args);
 
  private:
-  void CreateEnvMessagePort(Environment* env);
+  bool CreateEnvMessagePort(Environment* env);
   static size_t NearHeapLimit(void* data, size_t current_heap_limit,
                               size_t initial_heap_limit);
 
diff --git a/test/parallel/test-worker-nearheaplimit-deadlock.js b/test/parallel/test-worker-nearheaplimit-deadlock.js
new file mode 100644
index 00000000000000..1f38bc074da048
--- /dev/null
+++ b/test/parallel/test-worker-nearheaplimit-deadlock.js
@@ -0,0 +1,22 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const { Worker } = require('worker_threads');
+
+// Do not use isMainThread so that this test itself can be run inside a Worker.
+if (!process.env.HAS_STARTED_WORKER) {
+  process.env.HAS_STARTED_WORKER = 1;
+  const opts = {
+    resourceLimits: {
+      maxYoungGenerationSizeMb: 0,
+      maxOldGenerationSizeMb: 0
+    }
+  };
+
+  const worker = new Worker(__filename, opts);
+  worker.on('error', common.mustCall((err) => {
+    assert.strictEqual(err.code, 'ERR_WORKER_OUT_OF_MEMORY');
+  }));
+} else {
+  setInterval(() => {}, 1);
+}