timers: make setImmediate() immune to tampering

Make setImmediate() immune to `process` global tampering by removing
the dependency on the `process._immediateCallback` property.

PR-URL: https://github.com/nodejs/node/pull/17736
Fixes: https://github.com/nodejs/node/issues/17681
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

lib/timers.js

@@ -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 timerInternals = require('internal/timers');
 const internalUtil = require('internal/util');
@@ -48,12 +51,8 @@ const { kInit, kDestroy, kAsyncIdCounter } = async_wrap.constants;
 const async_id_symbol = timerInternals.async_id_symbol;
 const trigger_async_id_symbol = timerInternals.trigger_async_id_symbol;
 
-/* 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);
 
 // The Timeout class
 const Timeout = timerInternals.Timeout;
@@ -676,8 +675,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) {
@@ -694,7 +691,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]);
@@ -748,9 +745,9 @@ function Immediate(callback, args) {
              this);
   }
 
-  if (scheduledImmediateCount[0] === 0)
+  if (scheduledImmediateCountArray[0] === 0)
     activateImmediateCheck();
-  scheduledImmediateCount[0]++;
+  scheduledImmediateCountArray[0]++;
 
   immediateQueue.append(this);
 }
@@ -796,7 +793,7 @@ exports.clearImmediate = function(immediate) {
   if (!immediate) return;
 
   if (!immediate._destroyed) {
-    scheduledImmediateCount[0]--;
+    scheduledImmediateCountArray[0]--;
     immediate._destroyed = true;
 
     if (async_hook_fields[kDestroy] > 0) {

src/env.cc

@@ -311,7 +311,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();

src/env.h

@@ -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")                                                    \
@@ -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(module_load_list_array, v8::Array)                                        \
   V(pbkdf2_constructor_template, v8::ObjectTemplate)                          \

src/node.cc

@@ -2939,12 +2939,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();
@@ -3168,12 +3162,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(),
@@ -3305,9 +3293,6 @@ void SetupProcessObject(Environment* env,
                              env->as_external()).FromJust());
 
   // define various internal methods
-  env->SetMethod(process,
-                 "_activateImmediateCheck",
-                 ActivateImmediateCheck);
   env->SetMethod(process,
                  "_startProfilerIdleNotifier",
                  StartProfilerIdleNotifier);

src/timer_wrap.cc

@@ -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;
@@ -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

test/common/index.js

@@ -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');

test/parallel/test-timer-immediate.js

@@ -0,0 +1,5 @@
+'use strict';
+const common = require('../common');
+common.globalCheck = false;
+global.process = {};  // Boom!
+setImmediate(common.mustCall());