Skip to content

Commit

Permalink
vm: fix race condition with timeout param
Browse files Browse the repository at this point in the history
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: nodejs#13074
Reviewed-By: Anna Henningsen <[email protected]>
  • Loading branch information
laverdet committed Jul 19, 2017
1 parent 53951fb commit 7a175b0
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 73 deletions.
24 changes: 11 additions & 13 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -841,35 +841,33 @@ class ContextifyScript : public BaseObject {
bool timed_out = false;
bool received_signal = false;
if (break_on_sigint && timeout != -1) {
Watchdog wd(env->isolate(), timeout);
SigintWatchdog swd(env->isolate());
Watchdog wd(env->isolate(), timeout, &timed_out);
SigintWatchdog swd(env->isolate(), &received_signal);
result = script->Run();
timed_out = wd.HasTimedOut();
received_signal = swd.HasReceivedSignal();
} else if (break_on_sigint) {
SigintWatchdog swd(env->isolate());
SigintWatchdog swd(env->isolate(), &received_signal);
result = script->Run();
received_signal = swd.HasReceivedSignal();
} else if (timeout != -1) {
Watchdog wd(env->isolate(), timeout);
Watchdog wd(env->isolate(), timeout, &timed_out);
result = script->Run();
timed_out = wd.HasTimedOut();
} else {
result = script->Run();
}

if (try_catch->HasCaught()) {
if (try_catch->HasTerminated())
env->isolate()->CancelTerminateExecution();

if (timed_out || received_signal) {
// It is possible that execution was terminated by another timeout in
// which this timeout is nested, so check whether one of the watchdogs
// from this invocation is responsible for termination.
if (timed_out) {
env->ThrowError("Script execution timed out.");
} else if (received_signal) {
env->ThrowError("Script execution interrupted.");
} else if (display_errors) {
}
env->isolate()->CancelTerminateExecution();
}

if (try_catch->HasCaught()) {
if (!timed_out && !received_signal && display_errors) {
// We should decorate non-termination exceptions
DecorateErrorStack(env, *try_catch);
}
Expand Down
53 changes: 11 additions & 42 deletions src/node_watchdog.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@

namespace node {

Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms) : isolate_(isolate),
timed_out_(false),
destroyed_(false) {
Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms, bool* timed_out)
: isolate_(isolate), timed_out_(timed_out) {

int rc;
loop_ = new uv_loop_t;
CHECK(loop_);
Expand All @@ -33,20 +33,6 @@ Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms) : isolate_(isolate),


Watchdog::~Watchdog() {
Destroy();
}


void Watchdog::Dispose() {
Destroy();
}


void Watchdog::Destroy() {
if (destroyed_) {
return;
}

uv_async_send(&async_);
uv_thread_join(&thread_);

Expand All @@ -59,8 +45,6 @@ void Watchdog::Destroy() {
CHECK_EQ(0, rc);
delete loop_;
loop_ = nullptr;

destroyed_ = true;
}


Expand All @@ -72,7 +56,7 @@ void Watchdog::Run(void* arg) {
uv_run(wd->loop_, UV_RUN_DEFAULT);

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

Expand All @@ -85,45 +69,30 @@ void Watchdog::Async(uv_async_t* async) {

void Watchdog::Timer(uv_timer_t* timer) {
Watchdog* w = ContainerOf(&Watchdog::timer_, timer);
w->timed_out_ = true;
uv_stop(w->loop_);
*w->timed_out_ = true;
w->isolate()->TerminateExecution();
uv_stop(w->loop_);
}


SigintWatchdog::~SigintWatchdog() {
Destroy();
}


void SigintWatchdog::Dispose() {
Destroy();
}


SigintWatchdog::SigintWatchdog(v8::Isolate* isolate)
: isolate_(isolate), received_signal_(false), destroyed_(false) {
SigintWatchdog::SigintWatchdog(
v8::Isolate* isolate, bool* received_signal)
: isolate_(isolate), received_signal_(received_signal) {
// Register this watchdog with the global SIGINT/Ctrl+C listener.
SigintWatchdogHelper::GetInstance()->Register(this);
// Start the helper thread, if that has not already happened.
SigintWatchdogHelper::GetInstance()->Start();
}


void SigintWatchdog::Destroy() {
if (destroyed_) {
return;
}

destroyed_ = true;

SigintWatchdog::~SigintWatchdog() {
SigintWatchdogHelper::GetInstance()->Unregister(this);
SigintWatchdogHelper::GetInstance()->Stop();
}


void SigintWatchdog::HandleSigint() {
received_signal_ = true;
*received_signal_ = true;
isolate_->TerminateExecution();
}

Expand Down
26 changes: 8 additions & 18 deletions src/node_watchdog.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,13 @@ namespace node {

class Watchdog {
public:
explicit Watchdog(v8::Isolate* isolate, uint64_t ms);
explicit Watchdog(v8::Isolate* isolate,
uint64_t ms,
bool* timed_out = nullptr);
~Watchdog();

void Dispose();

v8::Isolate* isolate() { return isolate_; }
bool HasTimedOut() { return timed_out_; }
private:
void Destroy();

private:
static void Run(void* arg);
static void Async(uv_async_t* async);
static void Timer(uv_timer_t* timer);
Expand All @@ -35,27 +32,20 @@ class Watchdog {
uv_loop_t* loop_;
uv_async_t async_;
uv_timer_t timer_;
bool timed_out_;
bool destroyed_;
bool* timed_out_;
};

class SigintWatchdog {
public:
explicit SigintWatchdog(v8::Isolate* isolate);
explicit SigintWatchdog(v8::Isolate* isolate,
bool* received_signal = nullptr);
~SigintWatchdog();

void Dispose();

v8::Isolate* isolate() { return isolate_; }
bool HasReceivedSignal() { return received_signal_; }
void HandleSigint();

private:
void Destroy();

v8::Isolate* isolate_;
bool received_signal_;
bool destroyed_;
bool* received_signal_;
};

class SigintWatchdogHelper {
Expand Down
33 changes: 33 additions & 0 deletions test/pummel/test-vm-race.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
'use strict';
require('../common');
const vm = require('vm');

// We're testing a race condition so we just have to spin this in a loop
// for a little while and see if it breaks. The condition being tested
// is an `isolate->TerminateExecution()` reaching the main JS stack from
// the timeout watchdog.
const sandbox = { timeout: 5 };
const context = vm.createContext(sandbox);
const script = new vm.Script(
'var d = Date.now() + timeout;while (d > Date.now());'
);
const immediate = setImmediate(function() {
throw new Error('Detected vm race condition!');
});

// When this condition was first discovered this test would fail in 50ms
// or so. A better, but still incorrect implementation would fail after
// 100 seconds or so. If you're messing with vm timeouts you might
// consider increasing this timeout to hammer out races.
const giveUp = Date.now() + 5000;
do {
// The loop adjusts the timeout up or down trying to hit the race
try {
script.runInContext(context, { timeout: 5 });
++sandbox.timeout;
} catch (err) {
--sandbox.timeout;
}
} while (Date.now() < giveUp);

clearImmediate(immediate);

0 comments on commit 7a175b0

Please sign in to comment.