From 0e92ae64f01140bc527b5a02d7f154dca882ef09 Mon Sep 17 00:00:00 2001 From: Anna Henningsen 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 Reviewed-By: David Carlier Reviewed-By: James M Snell --- 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 d02a10c9740e61..6081a523cd4c52 100644 --- 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 MessagePort::Detach() {