-
Notifications
You must be signed in to change notification settings - Fork 29.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
vm: fix race condition with timeout
param
#13074
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,16 +37,14 @@ namespace node { | |
|
||
class Watchdog { | ||
public: | ||
explicit Watchdog(v8::Isolate* isolate, uint64_t ms); | ||
explicit Watchdog( | ||
v8::Isolate* isolate, | ||
uint64_t ms, | ||
bool* timed_out = NULL); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto for 4-space indents. I think here we’d usually keep the |
||
~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); | ||
|
@@ -56,27 +54,21 @@ 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 = NULL); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (ditto) |
||
~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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
// Copyright Joyent, Inc. and other Node contributors. | ||
// | ||
// Permission is hereby granted, free of charge, to any person obtaining a | ||
// copy of this software and associated documentation files (the | ||
// "Software"), to deal in the Software without restriction, including | ||
// without limitation the rights to use, copy, modify, merge, publish, | ||
// distribute, sublicense, and/or sell copies of the Software, and to permit | ||
// persons to whom the Software is furnished to do so, subject to the | ||
// following conditions: | ||
// | ||
// The above copyright notice and this permission notice shall be included | ||
// in all copies or substantial portions of the Software. | ||
// | ||
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS | ||
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF | ||
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN | ||
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, | ||
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR | ||
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE | ||
// USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can drop the copyright header for newly added files, I think. |
||
|
||
'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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in a previous version of this PR these were
volatile
… any reason why that changed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sorry I meant to leave a note about that. The volatile ended up being unnecessary. The watchdogs take a non-const reference so that's good enough to ensure the optimizer doesn't do anything fishy. Then in the dtor
uv_thread_join
is called which will synchronize the watchdog thread back up with the main thread.