Skip to content

Commit 6b25c75

Browse files
laverdetjasnell
authored andcommitted
vm: fix race condition with timeout param
This fixes a race condition in the watchdog timer used for vm timeouts. The condition would terminate the main stack's execution instead of the code running under the sandbox. PR-URL: #13074 Reviewed-By: Anna Henningsen <[email protected]>
1 parent 191bb5a commit 6b25c75

File tree

4 files changed

+63
-73
lines changed

4 files changed

+63
-73
lines changed

src/node_contextify.cc

+11-13
Original file line numberDiff line numberDiff line change
@@ -958,35 +958,33 @@ class ContextifyScript : public BaseObject {
958958
bool timed_out = false;
959959
bool received_signal = false;
960960
if (break_on_sigint && timeout != -1) {
961-
Watchdog wd(env->isolate(), timeout);
962-
SigintWatchdog swd(env->isolate());
961+
Watchdog wd(env->isolate(), timeout, &timed_out);
962+
SigintWatchdog swd(env->isolate(), &received_signal);
963963
result = script->Run();
964-
timed_out = wd.HasTimedOut();
965-
received_signal = swd.HasReceivedSignal();
966964
} else if (break_on_sigint) {
967-
SigintWatchdog swd(env->isolate());
965+
SigintWatchdog swd(env->isolate(), &received_signal);
968966
result = script->Run();
969-
received_signal = swd.HasReceivedSignal();
970967
} else if (timeout != -1) {
971-
Watchdog wd(env->isolate(), timeout);
968+
Watchdog wd(env->isolate(), timeout, &timed_out);
972969
result = script->Run();
973-
timed_out = wd.HasTimedOut();
974970
} else {
975971
result = script->Run();
976972
}
977973

978-
if (try_catch->HasCaught()) {
979-
if (try_catch->HasTerminated())
980-
env->isolate()->CancelTerminateExecution();
981-
974+
if (timed_out || received_signal) {
982975
// It is possible that execution was terminated by another timeout in
983976
// which this timeout is nested, so check whether one of the watchdogs
984977
// from this invocation is responsible for termination.
985978
if (timed_out) {
986979
env->ThrowError("Script execution timed out.");
987980
} else if (received_signal) {
988981
env->ThrowError("Script execution interrupted.");
989-
} else if (display_errors) {
982+
}
983+
env->isolate()->CancelTerminateExecution();
984+
}
985+
986+
if (try_catch->HasCaught()) {
987+
if (!timed_out && !received_signal && display_errors) {
990988
// We should decorate non-termination exceptions
991989
DecorateErrorStack(env, *try_catch);
992990
}

src/node_watchdog.cc

+11-42
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,9 @@
2727

2828
namespace node {
2929

30-
Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms) : isolate_(isolate),
31-
timed_out_(false),
32-
destroyed_(false) {
30+
Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms, bool* timed_out)
31+
: isolate_(isolate), timed_out_(timed_out) {
32+
3333
int rc;
3434
loop_ = new uv_loop_t;
3535
CHECK(loop_);
@@ -54,20 +54,6 @@ Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms) : isolate_(isolate),
5454

5555

5656
Watchdog::~Watchdog() {
57-
Destroy();
58-
}
59-
60-
61-
void Watchdog::Dispose() {
62-
Destroy();
63-
}
64-
65-
66-
void Watchdog::Destroy() {
67-
if (destroyed_) {
68-
return;
69-
}
70-
7157
uv_async_send(&async_);
7258
uv_thread_join(&thread_);
7359

@@ -80,8 +66,6 @@ void Watchdog::Destroy() {
8066
CHECK_EQ(0, rc);
8167
delete loop_;
8268
loop_ = nullptr;
83-
84-
destroyed_ = true;
8569
}
8670

8771

@@ -93,7 +77,7 @@ void Watchdog::Run(void* arg) {
9377
uv_run(wd->loop_, UV_RUN_DEFAULT);
9478

9579
// Loop ref count reaches zero when both handles are closed.
96-
// Close the timer handle on this side and let Destroy() close async_
80+
// Close the timer handle on this side and let ~Watchdog() close async_
9781
uv_close(reinterpret_cast<uv_handle_t*>(&wd->timer_), nullptr);
9882
}
9983

@@ -106,45 +90,30 @@ void Watchdog::Async(uv_async_t* async) {
10690

10791
void Watchdog::Timer(uv_timer_t* timer) {
10892
Watchdog* w = ContainerOf(&Watchdog::timer_, timer);
109-
w->timed_out_ = true;
110-
uv_stop(w->loop_);
93+
*w->timed_out_ = true;
11194
w->isolate()->TerminateExecution();
95+
uv_stop(w->loop_);
11296
}
11397

11498

115-
SigintWatchdog::~SigintWatchdog() {
116-
Destroy();
117-
}
118-
119-
120-
void SigintWatchdog::Dispose() {
121-
Destroy();
122-
}
123-
124-
125-
SigintWatchdog::SigintWatchdog(v8::Isolate* isolate)
126-
: isolate_(isolate), received_signal_(false), destroyed_(false) {
99+
SigintWatchdog::SigintWatchdog(
100+
v8::Isolate* isolate, bool* received_signal)
101+
: isolate_(isolate), received_signal_(received_signal) {
127102
// Register this watchdog with the global SIGINT/Ctrl+C listener.
128103
SigintWatchdogHelper::GetInstance()->Register(this);
129104
// Start the helper thread, if that has not already happened.
130105
SigintWatchdogHelper::GetInstance()->Start();
131106
}
132107

133108

134-
void SigintWatchdog::Destroy() {
135-
if (destroyed_) {
136-
return;
137-
}
138-
139-
destroyed_ = true;
140-
109+
SigintWatchdog::~SigintWatchdog() {
141110
SigintWatchdogHelper::GetInstance()->Unregister(this);
142111
SigintWatchdogHelper::GetInstance()->Stop();
143112
}
144113

145114

146115
void SigintWatchdog::HandleSigint() {
147-
received_signal_ = true;
116+
*received_signal_ = true;
148117
isolate_->TerminateExecution();
149118
}
150119

src/node_watchdog.h

+8-18
Original file line numberDiff line numberDiff line change
@@ -37,16 +37,13 @@ namespace node {
3737

3838
class Watchdog {
3939
public:
40-
explicit Watchdog(v8::Isolate* isolate, uint64_t ms);
40+
explicit Watchdog(v8::Isolate* isolate,
41+
uint64_t ms,
42+
bool* timed_out = nullptr);
4143
~Watchdog();
42-
43-
void Dispose();
44-
4544
v8::Isolate* isolate() { return isolate_; }
46-
bool HasTimedOut() { return timed_out_; }
47-
private:
48-
void Destroy();
4945

46+
private:
5047
static void Run(void* arg);
5148
static void Async(uv_async_t* async);
5249
static void Timer(uv_timer_t* timer);
@@ -56,27 +53,20 @@ class Watchdog {
5653
uv_loop_t* loop_;
5754
uv_async_t async_;
5855
uv_timer_t timer_;
59-
bool timed_out_;
60-
bool destroyed_;
56+
bool* timed_out_;
6157
};
6258

6359
class SigintWatchdog {
6460
public:
65-
explicit SigintWatchdog(v8::Isolate* isolate);
61+
explicit SigintWatchdog(v8::Isolate* isolate,
62+
bool* received_signal = nullptr);
6663
~SigintWatchdog();
67-
68-
void Dispose();
69-
7064
v8::Isolate* isolate() { return isolate_; }
71-
bool HasReceivedSignal() { return received_signal_; }
7265
void HandleSigint();
7366

7467
private:
75-
void Destroy();
76-
7768
v8::Isolate* isolate_;
78-
bool received_signal_;
79-
bool destroyed_;
69+
bool* received_signal_;
8070
};
8171

8272
class SigintWatchdogHelper {

test/pummel/test-vm-race.js

+33
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
'use strict';
2+
require('../common');
3+
const vm = require('vm');
4+
5+
// We're testing a race condition so we just have to spin this in a loop
6+
// for a little while and see if it breaks. The condition being tested
7+
// is an `isolate->TerminateExecution()` reaching the main JS stack from
8+
// the timeout watchdog.
9+
const sandbox = { timeout: 5 };
10+
const context = vm.createContext(sandbox);
11+
const script = new vm.Script(
12+
'var d = Date.now() + timeout;while (d > Date.now());'
13+
);
14+
const immediate = setImmediate(function() {
15+
throw new Error('Detected vm race condition!');
16+
});
17+
18+
// When this condition was first discovered this test would fail in 50ms
19+
// or so. A better, but still incorrect implementation would fail after
20+
// 100 seconds or so. If you're messing with vm timeouts you might
21+
// consider increasing this timeout to hammer out races.
22+
const giveUp = Date.now() + 5000;
23+
do {
24+
// The loop adjusts the timeout up or down trying to hit the race
25+
try {
26+
script.runInContext(context, { timeout: 5 });
27+
++sandbox.timeout;
28+
} catch (err) {
29+
--sandbox.timeout;
30+
}
31+
} while (Date.now() < giveUp);
32+
33+
clearImmediate(immediate);

0 commit comments

Comments
 (0)