From c66421452686c1f93a708b1edcd4ca8d48037f25 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sun, 22 Mar 2020 21:07:21 +0100
Subject: [PATCH] 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).
PR-URL: https://github.com/nodejs/node/pull/32523
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
src/env.cc | 24 ++++++++++++++++++------
src/env.h | 2 +-
2 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/env.cc b/src/env.cc
index 36f8d42b86dc9b..5098e8ef0d602c 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -435,7 +435,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());
@@ -768,12 +769,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) };
@@ -784,9 +796,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
index 539fbf19f4a496..fb28cd81346cb0 100644
--- a/src/env.h
+++ b/src/env.h
@@ -1423,7 +1423,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);