From c3c35d9d28554ade4d8d638be31732c11de0ab7f Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Tue, 19 Feb 2019 14:29:31 -0800 Subject: [PATCH 1/3] 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 --- lib/domain.js | 60 ++++++- .../test-domain-emit-error-handler-stack.js | 161 ++++++++++++++++++ .../test-domain-thrown-error-handler-stack.js | 46 +++++ ...in-top-level-error-handler-clears-stack.js | 13 +- ...st-timers-reset-process-domain-on-throw.js | 7 +- 5 files changed, 268 insertions(+), 19 deletions(-) create mode 100644 test/parallel/test-domain-emit-error-handler-stack.js create mode 100644 test/parallel/test-domain-thrown-error-handler-stack.js diff --git a/lib/domain.js b/lib/domain.js index 031350746cc5b8..74e1f50a246b8f 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -125,7 +125,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); @@ -199,6 +199,13 @@ Domain.prototype._errorHandler = function(er) { er.domain = this; er.domainThrown = true; } + // Pop all adjacent duplicates of the currently acrtive 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. // @@ -209,15 +216,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); @@ -241,10 +248,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]; @@ -449,7 +452,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; } diff --git a/test/parallel/test-domain-emit-error-handler-stack.js b/test/parallel/test-domain-emit-error-handler-stack.js new file mode 100644 index 00000000000000..61b1fe8462cc8f --- /dev/null +++ b/test/parallel/test-domain-emit-error-handler-stack.js @@ -0,0 +1,161 @@ +'use strict'; + +const assert = require('assert'); +const common = require('../common'); +const domain = require('domain'); +const EventEmitter = require('events'); + +/* + * Make sure that the domains stack and the active domain is setup properly when + * a domain's error handler is called due to an error event being emitted. + * More specifically, we want to test that: + * - the active domain in the domain's error handler is *not* that domain, *but* + * the active domain is a any direct parent domain at the time the error was + * emitted. + * - the domains stack in the domain's error handler does *not* include that + * domain, *but* it includes all parents of that domain when the error was + * emitted. + */ +const d1 = domain.create(); +const d2 = domain.create(); +const d3 = domain.create(); + +function checkExpectedDomains(err) { + // First, check that domains stack and active domain is as expected when the + // event handler is called synchronously via ee.emit('error'). + if (domain._stack.length !== err.expectedStackLength) { + console.error('expected domains stack length of %d, but instead is %d', + err.expectedStackLength, domain._stack.length); + process.exit(1); + } + + if (process.domain !== err.expectedActiveDomain) { + console.error('expected active domain to be %j, but instead is %j', + err.expectedActiveDomain, process.domain); + process.exit(1); + } + + // Then make sure that the domains stack and active domain is setup as + // expected when executing a callback scheduled via nextTick from the error + // handler. + process.nextTick(() => { + const expectedStackLengthInNextTickCb = + err.expectedStackLength > 0 ? 1 : 0; + if (domain._stack.length !== expectedStackLengthInNextTickCb) { + console.error('expected stack length in nextTick cb to be %d, ' + + 'but instead is %d', expectedStackLengthInNextTickCb, + domain._stack.length); + process.exit(1); + } + + const expectedActiveDomainInNextTickCb = + expectedStackLengthInNextTickCb === 0 ? undefined : + err.expectedActiveDomain; + if (process.domain !== expectedActiveDomainInNextTickCb) { + console.error('expected active domain in nextTick cb to be %j, ' + + 'but instead is %j', expectedActiveDomainInNextTickCb, + process.domain); + process.exit(1); + } + }); +} + +d1.on('error', common.mustCall((err) => { + checkExpectedDomains(err); +}, 2)); + +d2.on('error', common.mustCall((err) => { + checkExpectedDomains(err); +}, 2)); + +d3.on('error', common.mustCall((err) => { + checkExpectedDomains(err); +}, 1)); + +d1.run(() => { + const ee = new EventEmitter(); + assert.strictEqual(process.domain, d1); + assert.strictEqual(domain._stack.length, 1); + + const err = new Error('oops'); + err.expectedStackLength = 0; + err.expectedActiveDomain = null; + ee.emit('error', err); + + assert.strictEqual(process.domain, d1); + assert.strictEqual(domain._stack.length, 1); +}); + +d1.run(() => { + d1.run(() => { + const ee = new EventEmitter(); + + assert.strictEqual(process.domain, d1); + assert.strictEqual(domain._stack.length, 2); + + const err = new Error('oops'); + err.expectedStackLength = 0; + err.expectedActiveDomain = null; + ee.emit('error', err); + + assert.strictEqual(process.domain, d1); + assert.strictEqual(domain._stack.length, 2); + }); +}); + +d1.run(() => { + d2.run(() => { + const ee = new EventEmitter(); + + assert.strictEqual(process.domain, d2); + assert.strictEqual(domain._stack.length, 2); + + const err = new Error('oops'); + err.expectedStackLength = 1; + err.expectedActiveDomain = d1; + ee.emit('error', err); + + assert.strictEqual(process.domain, d2); + assert.strictEqual(domain._stack.length, 2); + }); +}); + +d1.run(() => { + d2.run(() => { + d2.run(() => { + const ee = new EventEmitter(); + + assert.strictEqual(process.domain, d2); + assert.strictEqual(domain._stack.length, 3); + + const err = new Error('oops'); + err.expectedStackLength = 1; + err.expectedActiveDomain = d1; + ee.emit('error', err); + + assert.strictEqual(process.domain, d2); + assert.strictEqual(domain._stack.length, 3); + }); + }); +}); + +d3.run(() => { + d1.run(() => { + d3.run(() => { + d3.run(() => { + const ee = new EventEmitter(); + + assert.strictEqual(process.domain, d3); + assert.strictEqual(domain._stack.length, 4); + + const err = new Error('oops'); + err.expectedStackLength = 2; + err.expectedActiveDomain = d1; + ee.emit('error', err); + + assert.strictEqual(process.domain, d3); + assert.strictEqual(domain._stack.length, 4); + }); + }); + }); +}); diff --git a/test/parallel/test-domain-thrown-error-handler-stack.js b/test/parallel/test-domain-thrown-error-handler-stack.js new file mode 100644 index 00000000000000..564fd8a5d70700 --- /dev/null +++ b/test/parallel/test-domain-thrown-error-handler-stack.js @@ -0,0 +1,46 @@ +'use strict'; + +const common = require('../common'); +const domain = require('domain'); + +/* + * Make sure that when an erorr is thrown from a nested domain, its error + * handler runs outside of that domain, but within the context of any parent + * domain. + */ + +const d = domain.create(); +const d2 = domain.create(); + +d2.on('error', common.mustCall((err) => { + if (domain._stack.length !== 1) { + console.error('domains stack length should be 1 but is %d', + domain._stack.length); + process.exit(1); + } + + if (process.domain !== d) { + console.error('active domain should be %j but is %j', d, process.domain); + process.exit(1); + } + + process.nextTick(() => { + if (domain._stack.length !== 1) { + console.error('domains stack length should be 1 but is %d', + domain._stack.length); + process.exit(1); + } + + if (process.domain !== d) { + console.error('active domain should be %j but is %j', d, + process.domain); + process.exit(1); + } + }); +})); + +d.run(() => { + d2.run(() => { + throw new Error('oops'); + }); +}); diff --git a/test/parallel/test-domain-top-level-error-handler-clears-stack.js b/test/parallel/test-domain-top-level-error-handler-clears-stack.js index 05d5fca4671826..9dcf038abf9ef4 100644 --- a/test/parallel/test-domain-top-level-error-handler-clears-stack.js +++ b/test/parallel/test-domain-top-level-error-handler-clears-stack.js @@ -10,19 +10,18 @@ const domain = require('domain'); const d = domain.create(); d.on('error', common.mustCall(() => { + // Scheduling a callback with process.nextTick _could_ enter a _new_ domain, + // but domain's error handlers are called outside of their domain's context. + // So there should _no_ domain on the domains stack if the domains stack was + // cleared properly when the domain error handler was called. process.nextTick(() => { - // Scheduling a callback with process.nextTick will enter a _new_ domain, - // and the callback will be called after the domain that handled the error - // was exited. So there should be only one domain on the domains stack if - // the domains stack was cleared properly when the domain error handler - // returned. - if (domain._stack.length !== 1) { + if (domain._stack.length !== 0) { // Do not use assert to perform this test: this callback runs in a // different callstack as the original process._fatalException that // handled the original error, thus throwing here would trigger another // call to process._fatalException, and so on recursively and // indefinitely. - console.error('domains stack length should be 1, but instead is:', + console.error('domains stack length should be 0, but instead is:', domain._stack.length); process.exit(1); } diff --git a/test/parallel/test-timers-reset-process-domain-on-throw.js b/test/parallel/test-timers-reset-process-domain-on-throw.js index 55fd43feb81523..a0de01f0922964 100644 --- a/test/parallel/test-timers-reset-process-domain-on-throw.js +++ b/test/parallel/test-timers-reset-process-domain-on-throw.js @@ -26,9 +26,10 @@ function err() { } function handleDomainError(e) { - // In the domain's error handler, the current active domain should be the - // domain within which the error was thrown. - assert.strictEqual(process.domain, d); + assert.strictEqual(e.domain, d); + // Domains' error handlers are called outside of their domain's context, so + // we're not expecting any active domain here. + assert.strictEqual(process.domain, undefined); } } From d90a46c9d0d3b321984a2ebb15f64c68451ae3b5 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 3 Apr 2019 09:28:23 -0700 Subject: [PATCH 2/3] Apply suggestions from code review Co-Authored-By: misterdjules --- lib/domain.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/domain.js b/lib/domain.js index 74e1f50a246b8f..25d9f1f822a3c1 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -199,7 +199,7 @@ Domain.prototype._errorHandler = function(er) { er.domain = this; er.domainThrown = true; } - // Pop all adjacent duplicates of the currently acrtive domain from the stack. + // 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. @@ -452,7 +452,7 @@ EventEmitter.prototype.emit = function(...args) { er.domainThrown = false; } - // Remove the current domain (and its duplicates)from the domains stack and + // 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 From 386845d7633ed90a7327155add3f4342cf95032d Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Mon, 20 May 2019 10:46:46 -0700 Subject: [PATCH 3/3] fix lint error --- test/parallel/test-domain-emit-error-handler-stack.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-domain-emit-error-handler-stack.js b/test/parallel/test-domain-emit-error-handler-stack.js index 61b1fe8462cc8f..d42e4709799258 100644 --- a/test/parallel/test-domain-emit-error-handler-stack.js +++ b/test/parallel/test-domain-emit-error-handler-stack.js @@ -1,7 +1,7 @@ 'use strict'; -const assert = require('assert'); const common = require('../common'); +const assert = require('assert'); const domain = require('domain'); const EventEmitter = require('events');