src: make Environment::interrupt_data_ atomic

Otherwise this potentially leads to race conditions when used in a multi-threaded environment (i.e. when used for its very purpose). Backport-PR-URL: #35241 PR-URL: #32523 Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Sep 23, 2020 · 4513b6a3df1cf42f5f6fbd9f33b5709c89c2c774 · 4513b6a
1 parent 18ecaeb
commit 4513b6a3df1cf42f5f6fbd9f33b5709c89c2c774
Showing with 19 additions and 7 deletions.

+18 −6 src/env.cc

+1 −1 src/env.h
diff --git a/src/env.cc b/src/env.cc
@@ -412,7 +412,8 @@ Environment::Environment(IsolateData* isolate_data,
 }
 
 Environment::~Environment() {
-  if (interrupt_data_ != nullptr) *interrupt_data_ = nullptr;
+  if (Environment** interrupt_data = interrupt_data_.load())
+    *interrupt_data = nullptr;
 
   // FreeEnvironment() should have set this.
   CHECK(is_stopping());
@@ -737,12 +738,23 @@ void Environment::RunAndClearNativeImmediates(bool only_refed) {
 }
 
 void Environment::RequestInterruptFromV8() {
-  if (interrupt_data_ != nullptr) return;  // Already scheduled.
-
   // The Isolate may outlive the Environment, so some logic to handle the
   // situation in which the Environment is destroyed before the handler runs
   // is required.
-  interrupt_data_ = new Environment*(this);
+
+  // We allocate a new pointer to a pointer to this Environment instance, and
+  // try to set it as interrupt_data_. If interrupt_data_ was already set, then
+  // callbacks are already scheduled to run and we can delete our own pointer
+  // and just return. If it was nullptr previously, the Environment** is stored;
+  // ~Environment sets the Environment* contained in it to nullptr, so that
+  // the callback can check whether ~Environment has already run and it is thus
+  // not safe to access the Environment instance itself.
+  Environment** interrupt_data = new Environment*(this);
+  Environment** dummy = nullptr;
+  if (!interrupt_data_.compare_exchange_strong(dummy, interrupt_data)) {
+    delete interrupt_data;
+    return;  // Already scheduled.
+  }
 
   isolate()->RequestInterrupt([](Isolate* isolate, void* data) {
     std::unique_ptr<Environment*> env_ptr { static_cast<Environment**>(data) };
@@ -753,9 +765,9 @@ void Environment::RequestInterruptFromV8() {
       // handled during cleanup.
       return;
     }
-    env->interrupt_data_ = nullptr;
+    env->interrupt_data_.store(nullptr);
     env->RunAndClearInterrupts();
-  }, interrupt_data_);
+  }, interrupt_data);
 }
 
 void Environment::ScheduleTimer(int64_t duration_ms) {

diff --git a/src/env.h b/src/env.h
@@ -1412,7 +1412,7 @@ class Environment : public MemoryRetainer {
 
   void RunAndClearNativeImmediates(bool only_refed = false);
   void RunAndClearInterrupts();
-  Environment** interrupt_data_ = nullptr;
+  std::atomic<Environment**> interrupt_data_ {nullptr};
   void RequestInterruptFromV8();
   static void CheckImmediate(uv_check_t* handle);