Skip to content

Commit

Permalink
timers: make setImmediate() immune to tampering
Browse files Browse the repository at this point in the history
Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

Backport-PR-URL: #19006
PR-URL: #17736
Fixes: #17681
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
bnoordhuis authored and addaleax committed Feb 26, 2018
1 parent 0615edd commit 81da433
Show file tree
Hide file tree
Showing 7 changed files with 41 additions and 30 deletions.
23 changes: 10 additions & 13 deletions lib/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@
'use strict';

const async_wrap = process.binding('async_wrap');
const TimerWrap = process.binding('timer_wrap').Timer;
const {
Timer: TimerWrap,
setImmediateCallback,
} = process.binding('timer_wrap');
const L = require('internal/linkedlist');
const internalUtil = require('internal/util');
const { createPromise, promiseResolve } = process.binding('util');
Expand All @@ -47,12 +50,8 @@ const { kInit, kDestroy, kAsyncIdCounter } = async_wrap.constants;
const async_id_symbol = Symbol('asyncId');
const trigger_async_id_symbol = Symbol('triggerAsyncId');

/* This is an Uint32Array for easier sharing with C++ land. */
const scheduledImmediateCount = process._scheduledImmediateCount;
delete process._scheduledImmediateCount;
/* Kick off setImmediate processing */
const activateImmediateCheck = process._activateImmediateCheck;
delete process._activateImmediateCheck;
const [activateImmediateCheck, scheduledImmediateCountArray] =
setImmediateCallback(processImmediate);

// Timeout values > TIMEOUT_MAX are set to 1.
const TIMEOUT_MAX = 2 ** 31 - 1;
Expand Down Expand Up @@ -706,8 +705,6 @@ function processImmediate() {
}
}

process._immediateCallback = processImmediate;

// An optimization so that the try/finally only de-optimizes (since at least v8
// 4.7) what is in this smaller function.
function tryOnImmediate(immediate, oldTail) {
Expand All @@ -724,7 +721,7 @@ function tryOnImmediate(immediate, oldTail) {

if (!immediate._destroyed) {
immediate._destroyed = true;
scheduledImmediateCount[0]--;
scheduledImmediateCountArray[0]--;

if (async_hook_fields[kDestroy] > 0) {
emitDestroy(immediate[async_id_symbol]);
Expand Down Expand Up @@ -778,9 +775,9 @@ function Immediate(callback, args) {
this);
}

if (scheduledImmediateCount[0] === 0)
if (scheduledImmediateCountArray[0] === 0)
activateImmediateCheck();
scheduledImmediateCount[0]++;
scheduledImmediateCountArray[0]++;

immediateQueue.append(this);
}
Expand Down Expand Up @@ -826,7 +823,7 @@ exports.clearImmediate = function(immediate) {
if (!immediate) return;

if (!immediate._destroyed) {
scheduledImmediateCount[0]--;
scheduledImmediateCountArray[0]--;
immediate._destroyed = true;

if (async_hook_fields[kDestroy] > 0) {
Expand Down
2 changes: 1 addition & 1 deletion src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ void Environment::CheckImmediate(uv_check_t* handle) {

MakeCallback(env->isolate(),
env->process_object(),
env->immediate_callback_string(),
env->immediate_callback_function(),
0,
nullptr,
{0, 0}).ToLocalChecked();
Expand Down
2 changes: 1 addition & 1 deletion src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,6 @@ class ModuleWrap;
V(homedir_string, "homedir") \
V(hostmaster_string, "hostmaster") \
V(ignore_string, "ignore") \
V(immediate_callback_string, "_immediateCallback") \
V(infoaccess_string, "infoAccess") \
V(inherit_string, "inherit") \
V(input_string, "input") \
Expand Down Expand Up @@ -289,6 +288,7 @@ class ModuleWrap;
V(http2ping_constructor_template, v8::ObjectTemplate) \
V(http2stream_constructor_template, v8::ObjectTemplate) \
V(http2settings_constructor_template, v8::ObjectTemplate) \
V(immediate_callback_function, v8::Function) \
V(inspector_console_api_object, v8::Object) \
V(pbkdf2_constructor_template, v8::ObjectTemplate) \
V(pipe_constructor_template, v8::FunctionTemplate) \
Expand Down
15 changes: 0 additions & 15 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3169,12 +3169,6 @@ static void DebugEnd(const FunctionCallbackInfo<Value>& args);

namespace {

void ActivateImmediateCheck(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
env->ActivateImmediateCheck();
}


void StartProfilerIdleNotifier(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
env->StartProfilerIdleNotifier();
Expand Down Expand Up @@ -3399,12 +3393,6 @@ void SetupProcessObject(Environment* env,
FIXED_ONE_BYTE_STRING(env->isolate(), "ppid"),
GetParentProcessId).FromJust());

auto scheduled_immediate_count =
FIXED_ONE_BYTE_STRING(env->isolate(), "_scheduledImmediateCount");
CHECK(process->Set(env->context(),
scheduled_immediate_count,
env->scheduled_immediate_count().GetJSArray()).FromJust());

auto should_abort_on_uncaught_toggle =
FIXED_ONE_BYTE_STRING(env->isolate(), "_shouldAbortOnUncaughtToggle");
CHECK(process->Set(env->context(),
Expand Down Expand Up @@ -3536,9 +3524,6 @@ void SetupProcessObject(Environment* env,
env->as_external()).FromJust());

// define various internal methods
env->SetMethod(process,
"_activateImmediateCheck",
ActivateImmediateCheck);
env->SetMethod(process,
"_startProfilerIdleNotifier",
StartProfilerIdleNotifier);
Expand Down
23 changes: 23 additions & 0 deletions src/timer_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@
namespace node {
namespace {

using v8::Array;
using v8::Context;
using v8::Function;
using v8::FunctionCallbackInfo;
using v8::FunctionTemplate;
using v8::HandleScope;
Expand Down Expand Up @@ -67,11 +69,32 @@ class TimerWrap : public HandleWrap {
env->SetProtoMethod(constructor, "stop", Stop);

target->Set(timerString, constructor->GetFunction());

target->Set(env->context(),
FIXED_ONE_BYTE_STRING(env->isolate(), "setImmediateCallback"),
env->NewFunctionTemplate(SetImmediateCallback)
->GetFunction(env->context()).ToLocalChecked()).FromJust();
}

size_t self_size() const override { return sizeof(*this); }

private:
static void SetImmediateCallback(const FunctionCallbackInfo<Value>& args) {
CHECK(args[0]->IsFunction());
auto env = Environment::GetCurrent(args);
env->set_immediate_callback_function(args[0].As<Function>());
auto activate_cb = [] (const FunctionCallbackInfo<Value>& args) {
Environment::GetCurrent(args)->ActivateImmediateCheck();
};
auto activate_function =
env->NewFunctionTemplate(activate_cb)->GetFunction(env->context())
.ToLocalChecked();
auto result = Array::New(env->isolate(), 2);
result->Set(0, activate_function);
result->Set(1, env->scheduled_immediate_count().GetJSArray());
args.GetReturnValue().Set(result);
}

static void New(const FunctionCallbackInfo<Value>& args) {
// This constructor should not be exposed to public javascript.
// Therefore we assert that we are not trying to call this as a
Expand Down
1 change: 1 addition & 0 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

/* eslint-disable required-modules, crypto-check */
'use strict';
const process = global.process; // Some tests tamper with the process global.
const path = require('path');
const fs = require('fs');
const assert = require('assert');
Expand Down
5 changes: 5 additions & 0 deletions test/parallel/test-timer-immediate.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
'use strict';
const common = require('../common');
common.globalCheck = false;
global.process = {}; // Boom!
setImmediate(common.mustCall());

0 comments on commit 81da433

Please sign in to comment.