Skip to content

Commit

Permalink
src: make Environment::interrupt_data_ atomic
Browse files Browse the repository at this point in the history
Otherwise this potentially leads to race conditions when used in a
multi-threaded environment (i.e. when used for its very purpose).

PR-URL: #32523
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax authored and BethGriggs committed Apr 14, 2020
1 parent 89df2eb commit 6dffd6b
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 7 deletions.
24 changes: 18 additions & 6 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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) };
Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -1420,7 +1420,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);

Expand Down

0 comments on commit 6dffd6b

Please sign in to comment.