FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: fix race condition in debug signal on exit · nodejs/node@134a60c · GitHub

/ node Public

Commit 134a60c

Browse files
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

2 files changed

Lines changed: 60 additions & 13 deletions

File tree

‎src/atomic-polyfill.h‎

Lines 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_

‎src/node.cc‎

Lines 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)
Back | FazBrowse Home | New Git URL