From 546e8333baec20da35708238bf1a497ae00122c5 Mon Sep 17 00:00:00 2001 From: Jeremy Whitlock Date: Mon, 5 Oct 2015 13:08:53 -0700 Subject: [PATCH] src: fix --abort-on-uncaught-exception Revert 0af4c9ea7434e4f505dbe071357e4bc3b4ab2a8a, parts of 921f2de6cf999b5a4663615e37967b4269d755fe and port https://github.com/nodejs/node-v0.x-archive/pull/25835 from v0.12 to master so that node aborts at the right time when an error is thrown and --abort-on-uncaught-exception is used. Fixes #3035. PR: #3036 PR-URL: https://github.com/nodejs/node/pull/3036 Reviewed-By: Ben Noordhuis --- lib/domain.js | 88 ++++++--- src/async-wrap.cc | 18 +- src/env-inl.h | 9 - src/env.h | 6 +- src/node.cc | 43 ++-- src/node.js | 7 - .../test-domain-top-level-error-handler-throw.js | 50 +++++ ...test-domain-with-abort-on-uncaught-exception.js | 220 +++++++++++++++++++++ 8 files changed, 363 insertions(+), 78 deletions(-) create mode 100644 test/parallel/test-domain-top-level-error-handler-throw.js create mode 100644 test/parallel/test-domain-with-abort-on-uncaught-exception.js diff --git a/lib/domain.js b/lib/domain.js index 4ee4d71..b6321a2 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -59,6 +59,20 @@ Domain.prototype._disposed = undefined; // Called by process._fatalException in case an error was thrown. Domain.prototype._errorHandler = function errorHandler(er) { var caught = false; + var self = this; + + function emitError() { + var handled = self.emit('error', er); + + // Exit all domains on the stack. Uncaught exceptions end the + // current tick and no domains should be left on the stack + // between ticks. + stack.length = 0; + exports.active = process.domain = null; + + return handled; + } + // ignore errors on disposed domains. // // XXX This is a bit stupid. We should probably get rid of @@ -71,38 +85,54 @@ Domain.prototype._errorHandler = function errorHandler(er) { er.domain = this; er.domainThrown = true; } - // wrap this in a try/catch so we don't get infinite throwing - try { - // One of three things will happen here. - // - // 1. There is a handler, caught = true - // 2. There is no handler, caught = false - // 3. It throws, caught = false - // - // If caught is false after this, then there's no need to exit() - // the domain, because we're going to crash the process anyway. - caught = this.emit('error', er); - // Exit all domains on the stack. Uncaught exceptions end the - // current tick and no domains should be left on the stack - // between ticks. - stack.length = 0; - exports.active = process.domain = null; - } catch (er2) { - // 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(); + // The top-level domain-handler is handled separately. + // + // The reason is that if V8 was passed a command line option + // asking it to abort on an uncaught exception (currently + // "--abort-on-uncaught-exception"), we want an uncaught exception + // in the top-level domain error handler to make the + // 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) { + try { + // Set the _emittingTopLevelDomainError 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 + process._emittingTopLevelDomainError = true; + caught = emitError(); + } finally { + process._emittingTopLevelDomainError = false; } - if (stack.length) { - exports.active = process.domain = stack[stack.length - 1]; - caught = process._fatalException(er2); - } else { - caught = false; + } else { + // wrap this in a try/catch so we don't get infinite throwing + try { + // One of three things will happen here. + // + // 1. There is a handler, caught = true + // 2. There is no handler, caught = false + // 3. It throws, caught = false + // + // If caught is false after this, then there's no need to exit() + // the domain, because we're going to crash the process anyway. + caught = emitError(); + } catch (er2) { + // 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(); + } + if (stack.length) { + exports.active = process.domain = stack[stack.length - 1]; + caught = process._fatalException(er2); + } else { + caught = false; + } + return caught; } - return caught; } return caught; }; diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 37de058..ccf357b 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -168,7 +168,6 @@ Local AsyncWrap::MakeCallback(const Local cb, Local process = env()->process_object(); Local domain; bool has_domain = false; - bool has_abort_on_uncaught_and_domains = false; if (env()->using_domains()) { Local domain_v = context->Get(env()->domain_string()); @@ -177,7 +176,6 @@ Local AsyncWrap::MakeCallback(const Local cb, domain = domain_v.As(); if (domain->Get(env()->disposed_string())->IsTrue()) return Undefined(env()->isolate()); - has_abort_on_uncaught_and_domains = env()->using_abort_on_uncaught_exc(); } } @@ -201,21 +199,7 @@ Local AsyncWrap::MakeCallback(const Local cb, try_catch.SetVerbose(true); } - Local ret; - - if (has_abort_on_uncaught_and_domains) { - Local fn = process->Get(env()->domain_abort_uncaught_exc_string()); - if (fn->IsFunction()) { - Local special_context = Array::New(env()->isolate(), 2); - special_context->Set(0, context); - special_context->Set(1, cb); - ret = fn.As()->Call(special_context, argc, argv); - } else { - ret = cb->Call(context, argc, argv); - } - } else { - ret = cb->Call(context, argc, argv); - } + Local ret = cb->Call(context, argc, argv); if (try_catch.HasCaught()) { return Undefined(env()->isolate()); diff --git a/src/env-inl.h b/src/env-inl.h index 0115b00..0b4b4a5 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -208,7 +208,6 @@ inline Environment::Environment(v8::Local context, isolate_data_(IsolateData::GetOrCreate(context->GetIsolate(), loop)), timer_base_(uv_now(loop)), using_domains_(false), - using_abort_on_uncaught_exc_(false), using_asyncwrap_(false), printed_error_(false), trace_sync_io_(false), @@ -341,14 +340,6 @@ inline uint64_t Environment::timer_base() const { return timer_base_; } -inline bool Environment::using_abort_on_uncaught_exc() const { - return using_abort_on_uncaught_exc_; -} - -inline void Environment::set_using_abort_on_uncaught_exc(bool value) { - using_abort_on_uncaught_exc_ = value; -} - inline bool Environment::using_domains() const { return using_domains_; } diff --git a/src/env.h b/src/env.h index fc3d566..4a11c50 100644 --- a/src/env.h +++ b/src/env.h @@ -69,7 +69,7 @@ namespace node { V(dev_string, "dev") \ V(disposed_string, "_disposed") \ V(domain_string, "domain") \ - V(domain_abort_uncaught_exc_string, "_makeCallbackAbortOnUncaught") \ + V(emitting_top_level_domain_error_string, "_emittingTopLevelDomainError") \ V(exchange_string, "exchange") \ V(idle_string, "idle") \ V(irq_string, "irq") \ @@ -432,9 +432,6 @@ class Environment { inline ares_channel* cares_channel_ptr(); inline ares_task_list* cares_task_list(); - inline bool using_abort_on_uncaught_exc() const; - inline void set_using_abort_on_uncaught_exc(bool value); - inline bool using_domains() const; inline void set_using_domains(bool value); @@ -540,7 +537,6 @@ class Environment { ares_channel cares_channel_; ares_task_list cares_task_list_; bool using_domains_; - bool using_abort_on_uncaught_exc_; bool using_asyncwrap_; bool printed_error_; bool trace_sync_io_; diff --git a/src/node.cc b/src/node.cc index faac8b4..e74d513 100644 --- a/src/node.cc +++ b/src/node.cc @@ -124,7 +124,6 @@ static bool force_repl = false; static bool syntax_check_only = false; static bool trace_deprecation = false; static bool throw_deprecation = false; -static bool abort_on_uncaught_exception = false; static bool trace_sync_io = false; static bool track_heap_objects = false; static const char* eval_string = nullptr; @@ -907,6 +906,33 @@ void* ArrayBufferAllocator::Allocate(size_t size) { } +static bool IsDomainActive(const Environment* env) { + if (!env->using_domains()) + return false; + + Local domain_array = env->domain_array().As(); + if (domain_array->Length() == 0) + return false; + + Local domain_v = domain_array->Get(0); + return !domain_v->IsNull(); +} + + +static bool ShouldAbortOnUncaughtException(Isolate* isolate) { + HandleScope scope(isolate); + + Environment* env = Environment::GetCurrent(isolate); + Local process_object = env->process_object(); + Local emitting_top_level_domain_error_key = + env->emitting_top_level_domain_error_string(); + bool isEmittingTopLevelDomainError = + process_object->Get(emitting_top_level_domain_error_key)->BooleanValue(); + + return !IsDomainActive(env) || isEmittingTopLevelDomainError; +} + + void SetupDomainUse(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -2197,11 +2223,7 @@ void FatalException(Isolate* isolate, if (false == caught->BooleanValue()) { ReportException(env, error, message); - if (abort_on_uncaught_exception) { - ABORT(); - } else { - exit(1); - } + exit(1); } } @@ -3229,9 +3251,6 @@ static void ParseArgs(int* argc, track_heap_objects = true; } else if (strcmp(arg, "--throw-deprecation") == 0) { throw_deprecation = true; - } else if (strcmp(arg, "--abort-on-uncaught-exception") == 0 || - strcmp(arg, "--abort_on_uncaught_exception") == 0) { - abort_on_uncaught_exception = true; } else if (strcmp(arg, "--v8-options") == 0) { new_v8_argv[new_v8_argc] = "--help"; new_v8_argc += 1; @@ -3935,8 +3954,10 @@ static void StartNodeInstance(void* arg) { Environment* env = CreateEnvironment(isolate, context, instance_data); array_buffer_allocator->set_env(env); Context::Scope context_scope(context); - if (instance_data->is_main()) - env->set_using_abort_on_uncaught_exc(abort_on_uncaught_exception); + + node_isolate->SetAbortOnUncaughtExceptionCallback( + ShouldAbortOnUncaughtException); + // Start debug agent when argv has --debug if (instance_data->use_debug_agent()) StartDebug(env, debug_wait_connect); diff --git a/src/node.js b/src/node.js index da0fc67..7cfd2c0 100644 --- a/src/node.js +++ b/src/node.js @@ -210,13 +210,6 @@ }; startup.processFatal = function() { - process._makeCallbackAbortOnUncaught = function() { - try { - return this[1].apply(this[0], arguments); - } catch (err) { - process._fatalException(err); - } - }; process._fatalException = function(er) { var caught; diff --git a/test/parallel/test-domain-top-level-error-handler-throw.js b/test/parallel/test-domain-top-level-error-handler-throw.js new file mode 100644 index 0000000..2b9e704 --- /dev/null +++ b/test/parallel/test-domain-top-level-error-handler-throw.js @@ -0,0 +1,50 @@ +'use strict'; + +/* + * The goal of this test is to make sure that when a top-level error + * handler throws an error following the handling of a previous error, + * the process reports the error message from the error thrown in the + * top-level error handler, not the one from the previous error. + */ + +const common = require('../common'); + +const domainErrHandlerExMessage = 'exception from domain error handler'; +const internalExMessage = 'You should NOT see me'; + +if (process.argv[2] === 'child') { + var domain = require('domain'); + var d = domain.create(); + + d.on('error', function() { + throw new Error(domainErrHandlerExMessage); + }); + + d.run(function doStuff() { + process.nextTick(function() { + throw new Error(internalExMessage); + }); + }); +} else { + var fork = require('child_process').fork; + var assert = require('assert'); + + var child = fork(process.argv[1], ['child'], {silent:true}); + var stderrOutput = ''; + if (child) { + child.stderr.on('data', function onStderrData(data) { + stderrOutput += data.toString(); + }); + + child.on('exit', function onChildExited(exitCode, signal) { + assert(stderrOutput.indexOf(domainErrHandlerExMessage) !== -1); + assert(stderrOutput.indexOf(internalExMessage) === -1); + + var expectedExitCode = 7; + var expectedSignal = null; + + assert.equal(exitCode, expectedExitCode); + assert.equal(signal, expectedSignal); + }); + } +} diff --git a/test/parallel/test-domain-with-abort-on-uncaught-exception.js b/test/parallel/test-domain-with-abort-on-uncaught-exception.js new file mode 100644 index 0000000..9892ad6 --- /dev/null +++ b/test/parallel/test-domain-with-abort-on-uncaught-exception.js @@ -0,0 +1,220 @@ +'use strict'; + +const assert = require('assert'); +const fs = require('fs'); +const common = require('../common'); + +/* + * The goal of this test is to make sure that: + * + * - Even if --abort_on_uncaught_exception is passed on the command line, + * setting up a top-level domain error handler and throwing an error + * within this domain does *not* make the process abort. The process exits + * gracefully. + * + * - When passing --abort_on_uncaught_exception on the command line and + * setting up a top-level domain error handler, an error thrown + * within this domain's error handler *does* make the process abort. + * + * - When *not* passing --abort_on_uncaught_exception on the command line and + * setting up a top-level domain error handler, an error thrown within this + * domain's error handler does *not* make the process abort, but makes it exit + * with the proper failure exit code. + * + * - When throwing an error within the top-level domain's error handler + * within a try/catch block, the process should exit gracefully, whether or + * not --abort_on_uncaught_exception is passed on the command line. + */ + +const domainErrHandlerExMessage = 'exception from domain error handler'; + +if (process.argv[2] === 'child') { + var domain = require('domain'); + var d = domain.create(); + var triggeredProcessUncaughtException = false; + + process.on('uncaughtException', function onUncaughtException() { + // The process' uncaughtException event must not be emitted when + // an error handler is setup on the top-level domain. + // Exiting with exit code of 42 here so that it would assert when + // the parent checks the child exit code. + process.exit(42); + }); + + d.on('error', function(err) { + // Swallowing the error on purpose if 'throwInDomainErrHandler' is not + // set + if (process.argv.indexOf('throwInDomainErrHandler') !== -1) { + // If useTryCatch is set, wrap the throw in a try/catch block. + // This is to make sure that a caught exception does not trigger + // an abort. + if (process.argv.indexOf('useTryCatch') !== -1) { + try { + throw new Error(domainErrHandlerExMessage); + } catch (e) { + } + } else { + throw new Error(domainErrHandlerExMessage); + } + } + }); + + d.run(function doStuff() { + // Throwing from within different types of callbacks as each of them + // handles domains differently + process.nextTick(function() { + throw new Error('Error from nextTick callback'); + }); + + fs.exists('/non/existing/file', function onExists(exists) { + throw new Error('Error from fs.exists callback'); + }); + + setImmediate(function onSetImmediate() { + throw new Error('Error from setImmediate callback'); + }); + + setTimeout(function onTimeout() { + throw new Error('Error from setTimeout callback'); + }, 0); + + throw new Error('Error from domain.run callback'); + }); +} else { + var exec = require('child_process').exec; + + function testDomainExceptionHandling(cmdLineOption, options) { + if (typeof cmdLineOption === 'object') { + options = cmdLineOption; + cmdLineOption = undefined; + } + + var throwInDomainErrHandlerOpt; + if (options.throwInDomainErrHandler) + throwInDomainErrHandlerOpt = 'throwInDomainErrHandler'; + + var cmdToExec = ''; + if (process.platform !== 'win32') { + // Do not create core files, as it can take a lot of disk space on + // continuous testing and developers' machines + cmdToExec += 'ulimit -c 0 && '; + } + + var useTryCatchOpt; + if (options.useTryCatch) + useTryCatchOpt = 'useTryCatch'; + + cmdToExec += process.argv[0] + ' '; + cmdToExec += (cmdLineOption ? cmdLineOption : '') + ' '; + cmdToExec += process.argv[1] + ' '; + cmdToExec += [ + 'child', + throwInDomainErrHandlerOpt, + useTryCatchOpt + ].join(' '); + + var child = exec(cmdToExec); + + if (child) { + var childTriggeredOnUncaughtExceptionHandler = false; + child.on('message', function onChildMsg(msg) { + if (msg === 'triggeredProcessUncaughtEx') { + childTriggeredOnUncaughtExceptionHandler = true; + } + }); + + child.on('exit', function onChildExited(exitCode, signal) { + var expectedExitCodes; + var expectedSignals; + + // When throwing errors from the top-level domain error handler + // outside of a try/catch block, the process should not exit gracefully + if (!options.useTryCatch && options.throwInDomainErrHandler) { + if (cmdLineOption === '--abort_on_uncaught_exception') { + // If the top-level domain's error handler throws, and only if + // --abort_on_uncaught_exception is passed on the command line, + // the process must abort. + // + // We use an array of values since the actual exit code can differ + // across compilers. + // Depending on the compiler used, node will exit with either + // exit code 132 (SIGILL) or 134 (SIGABRT). + expectedExitCodes = [132, 134]; + + // On platforms using a non-GNU compiler, base::OS::Abort raises + // an illegal instruction signal. + // On platforms using a GNU compiler but with KSH being the + // default shell (like SmartOS), when a process aborts, KSH exits + // with an exit code that is greater than 256, and thus the exit + // code emitted with the 'exit' event is null and the signal is + // set to either SIGABRT or SIGILL. + expectedSignals = ['SIGABRT', 'SIGILL']; + + // On Windows, v8's base::OS::Abort triggers an access violation, + // which corresponds to exit code 3221225477 (0xC0000005) + if (process.platform === 'win32') + expectedExitCodes = [3221225477]; + + // When using --abort-on-uncaught-exception, V8 will use + // base::OS::Abort to terminate the process. + // Depending on the compiler used, the shell or other aspects of + // the platform used to build the node binary, this will actually + // make V8 exit by aborting or by raising a signal. In any case, + // one of them (exit code or signal) needs to be set to one of + // the expected exit codes or signals. + if (signal !== null) { + assert.ok(expectedSignals.indexOf(signal) > -1); + } else { + assert.ok(expectedExitCodes.indexOf(exitCode) > -1); + } + } else { + // By default, uncaught exceptions make node exit with an exit + // code of 7. + assert.equal(exitCode, 7); + assert.equal(signal, null); + } + } else { + // If the top-level domain's error handler does not throw, + // the process must exit gracefully, whether or not + // --abort_on_uncaught_exception was passed on the command line + assert.equal(exitCode, 0); + assert.equal(signal, null); + } + }); + } + } + + testDomainExceptionHandling('--abort_on_uncaught_exception', { + throwInDomainErrHandler: false, + useTryCatch: false + }); + + testDomainExceptionHandling('--abort_on_uncaught_exception', { + throwInDomainErrHandler: false, + useTryCatch: true + }); + + testDomainExceptionHandling('--abort_on_uncaught_exception', { + throwInDomainErrHandler: true, + useTryCatch: false + }); + + testDomainExceptionHandling('--abort_on_uncaught_exception', { + throwInDomainErrHandler: true, + useTryCatch: true + }); + + testDomainExceptionHandling({ + throwInDomainErrHandler: false + }); + + testDomainExceptionHandling({ + throwInDomainErrHandler: false, + useTryCatch: false + }); + + testDomainExceptionHandling({ + throwInDomainErrHandler: true, + useTryCatch: true + }); +} -- 2.7.4