From 5eed20b3b792d4d586803878e4ff8807dae5bd7e Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sat, 16 May 2020 12:03:32 +0200
Subject: [PATCH] worker: fix race condition in node_messaging.cc
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`AddToIncomingQueue()` relies on `owner_` only being modified with
`mutex_` being locked, but in these two places, that didn’t happen.

Modify them to use `Detach()` instead, which has the same effect
as setting `owner_ = nullptr` here, but does it with proper locking.

This race condition probably only shows up in practice when Node.js
is compiled in debug mode, because the compiler eliminates the
duplicate load in `AddToIncomingQueue()` when compiling with
optimizations enabled.

PR-URL: https://github.com/nodejs/node/pull/33429
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 src/node_messaging.cc | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/node_messaging.cc b/src/node_messaging.cc
index fa7132d7b43a3d..16b1a97eec2dfc 100644
--- a/src/node_messaging.cc
+++ b/src/node_messaging.cc
@@ -494,8 +494,7 @@ void MessagePortData::Disentangle() {
 }
 
 MessagePort::~MessagePort() {
-  if (data_)
-    data_->owner_ = nullptr;
+  if (data_) Detach();
 }
 
 MessagePort::MessagePort(Environment* env,
@@ -692,10 +691,9 @@ void MessagePort::OnMessage() {
 void MessagePort::OnClose() {
   Debug(this, "MessagePort::OnClose()");
   if (data_) {
-    data_->owner_ = nullptr;
-    data_->Disentangle();
+    // Detach() returns move(data_).
+    Detach()->Disentangle();
   }
-  data_.reset();
 }
 
 std::unique_ptr<MessagePortData> MessagePort::Detach() {