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 134a60c

Browse filesBrowse files
bnoordhuisrvagg
bnoordhuis
authored and
rvagg
committed
src: fix race condition in debug signal on exit
Before this commit, sending a SIGUSR1 at program exit could trigger a hard to reproduce race condition where `v8::Debug::DebugBreak(isolate)` got called when the isolate was in the process of being torn down. A similar race condition is in theory possible when sending signals to two threads simultaneously but I haven't been able to reproduce that myself (and I tried, oh how I tried.) This commit fixes the race condition by turning `node_isolate` into a `std::atomic` and using it as an ad hoc synchronization primitive in places where that is necessary. A bare minimum std::atomic polyfill is added for OS X because Apple wouldn't be Apple if things just worked out of the box. PR-URL: #3528 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 8057315 commit 134a60c
Copy full SHA for 134a60c

File tree

Expand file treeCollapse file tree

2 files changed

+60
-13
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+60
-13
lines changed
Open diff view settings
Collapse file

‎src/atomic-polyfill.h‎

Copy file name to clipboard
+18Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
#ifndef SRC_ATOMIC_POLYFILL_H_
2+
#define SRC_ATOMIC_POLYFILL_H_
3+
4+
#include "util.h"
5+
6+
namespace nonstd {
7+
8+
template <typename T>
9+
struct atomic {
10+
atomic() = default;
11+
T exchange(T value) { return __sync_lock_test_and_set(&value_, value); }
12+
T value_ = T();
13+
DISALLOW_COPY_AND_ASSIGN(atomic);
14+
};
15+
16+
} // namespace nonstd
17+
18+
#endif // SRC_ATOMIC_POLYFILL_H_
Collapse file

‎src/node.cc‎

Copy file name to clipboardExpand all lines: src/node.cc
+42-13Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,14 @@ typedef int mode_t;
8686
extern char **environ;
8787
#endif
8888

89+
#ifdef __APPLE__
90+
#include "atomic-polyfill.h" // NOLINT(build/include_order)
91+
namespace node { template <typename T> using atomic = nonstd::atomic<T>; }
92+
#else
93+
#include <atomic>
94+
namespace node { template <typename T> using atomic = std::atomic<T>; }
95+
#endif
96+
8997
namespace node {
9098

9199
using v8::Array;
@@ -153,7 +161,7 @@ static double prog_start_time;
153161
static bool debugger_running;
154162
static uv_async_t dispatch_debug_messages_async;
155163

156-
static Isolate* node_isolate = nullptr;
164+
static node::atomic<Isolate*> node_isolate;
157165
static v8::Platform* default_platform;
158166

159167

@@ -3410,28 +3418,46 @@ static void EnableDebug(Environment* env) {
34103418
}
34113419

34123420

3421+
// Called from an arbitrary thread.
3422+
static void TryStartDebugger() {
3423+
// Call only async signal-safe functions here! Don't retry the exchange,
3424+
// it will deadlock when the thread is interrupted inside a critical section.
3425+
if (auto isolate = node_isolate.exchange(nullptr)) {
3426+
v8::Debug::DebugBreak(isolate);
3427+
uv_async_send(&dispatch_debug_messages_async);
3428+
CHECK_EQ(nullptr, node_isolate.exchange(isolate));
3429+
}
3430+
}
3431+
3432+
34133433
// Called from the main thread.
34143434
static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle) {
3435+
// Synchronize with signal handler, see TryStartDebugger.
3436+
Isolate* isolate;
3437+
do {
3438+
isolate = node_isolate.exchange(nullptr);
3439+
} while (isolate == nullptr);
3440+
34153441
if (debugger_running == false) {
34163442
fprintf(stderr, "Starting debugger agent.\n");
34173443

3418-
HandleScope scope(node_isolate);
3419-
Environment* env = Environment::GetCurrent(node_isolate);
3444+
HandleScope scope(isolate);
3445+
Environment* env = Environment::GetCurrent(isolate);
34203446
Context::Scope context_scope(env->context());
34213447

34223448
StartDebug(env, false);
34233449
EnableDebug(env);
34243450
}
3425-
Isolate::Scope isolate_scope(node_isolate);
3451+
3452+
Isolate::Scope isolate_scope(isolate);
34263453
v8::Debug::ProcessDebugMessages();
3454+
CHECK_EQ(nullptr, node_isolate.exchange(isolate));
34273455
}
34283456

34293457

34303458
#ifdef __POSIX__
34313459
static void EnableDebugSignalHandler(int signo) {
3432-
// Call only async signal-safe functions here!
3433-
v8::Debug::DebugBreak(*static_cast<Isolate* volatile*>(&node_isolate));
3434-
uv_async_send(&dispatch_debug_messages_async);
3460+
TryStartDebugger();
34353461
}
34363462

34373463

@@ -3485,8 +3511,7 @@ static int RegisterDebugSignalHandler() {
34853511

34863512
#ifdef _WIN32
34873513
DWORD WINAPI EnableDebugThreadProc(void* arg) {
3488-
v8::Debug::DebugBreak(*static_cast<Isolate* volatile*>(&node_isolate));
3489-
uv_async_send(&dispatch_debug_messages_async);
3514+
TryStartDebugger();
34903515
return 0;
34913516
}
34923517

@@ -4006,7 +4031,8 @@ static void StartNodeInstance(void* arg) {
40064031
// Fetch a reference to the main isolate, so we have a reference to it
40074032
// even when we need it to access it from another (debugger) thread.
40084033
if (instance_data->is_main())
4009-
node_isolate = isolate;
4034+
CHECK_EQ(nullptr, node_isolate.exchange(isolate));
4035+
40104036
{
40114037
Locker locker(isolate);
40124038
Isolate::Scope isolate_scope(isolate);
@@ -4016,7 +4042,7 @@ static void StartNodeInstance(void* arg) {
40164042
array_buffer_allocator->set_env(env);
40174043
Context::Scope context_scope(context);
40184044

4019-
node_isolate->SetAbortOnUncaughtExceptionCallback(
4045+
isolate->SetAbortOnUncaughtExceptionCallback(
40204046
ShouldAbortOnUncaughtException);
40214047

40224048
// Start debug agent when argv has --debug
@@ -4070,12 +4096,15 @@ static void StartNodeInstance(void* arg) {
40704096
env = nullptr;
40714097
}
40724098

4099+
if (instance_data->is_main()) {
4100+
// Synchronize with signal handler, see TryStartDebugger.
4101+
while (isolate != node_isolate.exchange(nullptr)); // NOLINT
4102+
}
4103+
40734104
CHECK_NE(isolate, nullptr);
40744105
isolate->Dispose();
40754106
isolate = nullptr;
40764107
delete array_buffer_allocator;
4077-
if (instance_data->is_main())
4078-
node_isolate = nullptr;
40794108
}
40804109

40814110
int Start(int argc, char** argv) {

0 commit comments

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