domain: error handler runs outside of its domain

Before this change, domains' error handlers would run with the
corresponding domain as the active domain. This creates the
possibility for domains' error handlers to call themselves recursively
if an event emitter created in the error handler emits an error, or if
the error handler throws an error.

This change sets the active domain to be the domain's parent (or null
if the domain for which the error handler is called has no parent) to
prevent that from happening.

Fixes: https://github.com/nodejs/node/issues/26086
PR-URL: https://github.com/nodejs/node/pull/26211
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>

1 files changed, 51 insertions, 9 deletions

diff --git a/lib/domain.js b/lib/domain.js
index eab0b352bc..b2ffed2741 100644
--- a/lib/domain.js
+++ b/lib/domain.js
@@ -132,7 +132,7 @@ function topLevelDomainCallback(cb, ...args) {
 
 // It's possible to enter one domain while already inside
 // another one. The stack is each entered domain.
-const stack = [];
+let stack = [];
 exports._stack = stack;
 internalBinding('domain').enable(topLevelDomainCallback);
 
@@ -211,6 +211,13 @@ Domain.prototype._errorHandler = function(er) {
     });
     er.domainThrown = true;
   }
+  // Pop all adjacent duplicates of the currently active domain from the stack.
+  // This is done to prevent a domain's error handler to run within the context
+  // of itself, and re-entering itself recursively handler as a result of an
+  // exception thrown in its context.
+  while (exports.active === this) {
+    this.exit();
+  }
 
   // The top-level domain-handler is handled separately.
   //
@@ -221,15 +228,15 @@ Domain.prototype._errorHandler = function(er) {
   // process abort. Using try/catch here would always make V8 think
   // that these exceptions are caught, and thus would prevent it from
   // aborting in these cases.
-  if (stack.length === 1) {
+  if (stack.length === 0) {
     // If there's no error handler, do not emit an 'error' event
     // as this would throw an error, make the process exit, and thus
     // prevent the process 'uncaughtException' event from being emitted
     // if a listener is set.
     if (EventEmitter.listenerCount(this, 'error') > 0) {
-      // Clear the uncaughtExceptionCaptureCallback so that we know that, even
-      // if technically the top-level domain is still active, it would
-      // be ok to abort on an uncaught exception at this point
+      // Clear the uncaughtExceptionCaptureCallback so that we know that, since
+      // the top-level domain is not active anymore, it would be ok to abort on
+      // an uncaught exception at this point
       setUncaughtExceptionCaptureCallback(null);
       try {
         caught = this.emit('error', er);
@@ -253,10 +260,6 @@ Domain.prototype._errorHandler = function(er) {
       // The domain error handler threw!  oh no!
       // See if another domain can catch THIS error,
       // or else crash on the original one.
-      // If the user already exited it, then don't double-exit.
-      if (this === exports.active) {
-        stack.pop();
-      }
       updateExceptionCapture();
       if (stack.length) {
         exports.active = process.domain = stack[stack.length - 1];
@@ -486,7 +489,46 @@ EventEmitter.prototype.emit = function(...args) {
       er.domainThrown = false;
     }
 
+    // Remove the current domain (and its duplicates) from the domains stack and
+    // set the active domain to its parent (if any) so that the domain's error
+    // handler doesn't run in its own context. This prevents any event emitter
+    // created or any exception thrown in that error handler from recursively
+    // executing that error handler.
+    const origDomainsStack = stack.slice();
+    const origActiveDomain = process.domain;
+
+    // Travel the domains stack from top to bottom to find the first domain
+    // instance that is not a duplicate of the current active domain.
+    let idx = stack.length - 1;
+    while (idx > -1 && process.domain === stack[idx]) {
+      --idx;
+    }
+
+    // Change the stack to not contain the current active domain, and only the
+    // domains above it on the stack.
+    if (idx < 0) {
+      stack.length = 0;
+    } else {
+      stack.splice(idx + 1);
+    }
+
+    // Change the current active domain
+    if (stack.length > 0) {
+      exports.active = process.domain = stack[stack.length - 1];
+    } else {
+      exports.active = process.domain = null;
+    }
+
+    updateExceptionCapture();
+
     domain.emit('error', er);
+
+    // Now that the domain's error handler has completed, restore the domains
+    // stack and the active domain to their original values.
+    exports._stack = stack = origDomainsStack;
+    exports.active = process.domain = origActiveDomain;
+    updateExceptionCapture();
+
     return false;
   }