Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit 4513b6a

Browse filesBrowse files
committed
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>
1 parent 18ecaeb commit 4513b6a
Copy full SHA for 4513b6a

File tree

Expand file treeCollapse file tree

2 files changed

+19
-7
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+19
-7
lines changed
Open diff view settings
Collapse file

‎src/env.cc‎

Copy file name to clipboardExpand all lines: src/env.cc
+18-6Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -412,7 +412,8 @@ Environment::Environment(IsolateData* isolate_data,
412412
}
413413

414414
Environment::~Environment() {
415-
if (interrupt_data_ != nullptr) *interrupt_data_ = nullptr;
415+
if (Environment** interrupt_data = interrupt_data_.load())
416+
*interrupt_data = nullptr;
416417

417418
// FreeEnvironment() should have set this.
418419
CHECK(is_stopping());
@@ -737,12 +738,23 @@ void Environment::RunAndClearNativeImmediates(bool only_refed) {
737738
}
738739

739740
void Environment::RequestInterruptFromV8() {
740-
if (interrupt_data_ != nullptr) return; // Already scheduled.
741-
742741
// The Isolate may outlive the Environment, so some logic to handle the
743742
// situation in which the Environment is destroyed before the handler runs
744743
// is required.
745-
interrupt_data_ = new Environment*(this);
744+
745+
// We allocate a new pointer to a pointer to this Environment instance, and
746+
// try to set it as interrupt_data_. If interrupt_data_ was already set, then
747+
// callbacks are already scheduled to run and we can delete our own pointer
748+
// and just return. If it was nullptr previously, the Environment** is stored;
749+
// ~Environment sets the Environment* contained in it to nullptr, so that
750+
// the callback can check whether ~Environment has already run and it is thus
751+
// not safe to access the Environment instance itself.
752+
Environment** interrupt_data = new Environment*(this);
753+
Environment** dummy = nullptr;
754+
if (!interrupt_data_.compare_exchange_strong(dummy, interrupt_data)) {
755+
delete interrupt_data;
756+
return; // Already scheduled.
757+
}
746758

747759
isolate()->RequestInterrupt([](Isolate* isolate, void* data) {
748760
std::unique_ptr<Environment*> env_ptr { static_cast<Environment**>(data) };
@@ -753,9 +765,9 @@ void Environment::RequestInterruptFromV8() {
753765
// handled during cleanup.
754766
return;
755767
}
756-
env->interrupt_data_ = nullptr;
768+
env->interrupt_data_.store(nullptr);
757769
env->RunAndClearInterrupts();
758-
}, interrupt_data_);
770+
}, interrupt_data);
759771
}
760772

761773
void Environment::ScheduleTimer(int64_t duration_ms) {
Collapse file

‎src/env.h‎

Copy file name to clipboardExpand all lines: src/env.h
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1412,7 +1412,7 @@ class Environment : public MemoryRetainer {
14121412

14131413
void RunAndClearNativeImmediates(bool only_refed = false);
14141414
void RunAndClearInterrupts();
1415-
Environment** interrupt_data_ = nullptr;
1415+
std::atomic<Environment**> interrupt_data_ {nullptr};
14161416
void RequestInterruptFromV8();
14171417
static void CheckImmediate(uv_check_t* handle);
14181418

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.