worker: fix race condition in node_messaging.cc

`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: #33429 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · May 19, 2020 · 0e92ae6 · 0e92ae6
1 parent 61189d3
commit 0e92ae6
Showing 1 changed file with 3 additions and 5 deletions.
diff --git a/src/node_messaging.cc b/src/node_messaging.cc
@@ -464,8 +464,7 @@ void MessagePortData::Disentangle() {
 }
 
 MessagePort::~MessagePort() {
-  if (data_)
-    data_->owner_ = nullptr;
+  if (data_) Detach();
 }
 
 MessagePort::MessagePort(Environment* env,
@@ -662,10 +661,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() {