From 062beb08df6a1061b548d4b1859b20252ea10b07 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 3 Aug 2017 14:43:41 -0600 Subject: [PATCH] async_hooks: don't abort unnecessarily * id values of -1 are allowed. They indicate that the id was never correctly assigned to the async resource. These will appear in any call graph, and will only be apparent to those using the async_hooks module, then reported in an issue. * ids < -1 are still not allowed and will cause the application to exit the process; because there is no scenario where this should ever happen. * Add asyncId range checks to emitAfterScript(). * Fix emitBeforeScript() range checks which should have been || not &&. * Replace errors with entries in internal/errors. * Fix async_hooks tests that check for exceptions to match new internal/errors entries. NOTE: emit{Before,After,Destroy}() must continue to exit the process because in the case of an exception during hook execution the state of the application is unknowable. For example, an exception could cause a memory leak: const id_map = new Map(); before(id) { id_map.set(id, /* data object or similar */); }, after(id) { throw new Error('id never dies!'); id_map.delete(id); } Allowing a recoverable exception may also cause an abort because of a stack check in Environment::AsyncHooks::pop_ids() that verifies the async id and pop'd ids match. This case would be more difficult to debug than if fatalError() (lib/async_hooks.js) was called immediately. try { async_hooks.emitBefore(null, NaN); } catch (e) { } // do something async_hooks.emitAfter(5); It also allows an edge case where emitBefore() could be called twice and not have the pop_ids() CHECK fail: try { async_hooks.emitBefore(5, NaN); } catch (e) { } async_hooks.emitBefore(5); // do something async_hooks.emitAfter(5); There is the option of allowing mismatches in the stack and ignoring the check if no async hooks are enabled, but I don't believe going this far is necessary. PR-URL: https://github.com/nodejs/node/pull/14722 Reviewed-By: Anna Henningsen Reviewed-By: Refael Ackermann --- lib/async_hooks.js | 68 ++++++++++++------- lib/internal/errors.js | 3 + src/env-inl.h | 7 +- .../test-embedder.api.async-resource.js | 16 +++-- test/async-hooks/test-emit-before-after.js | 16 ++--- test/async-hooks/test-emit-init.js | 24 +++++-- ...t-async-hooks-asyncresource-constructor.js | 22 ++++-- test/parallel/test-async-wrap-constructor.js | 7 +- test/parallel/test-internal-errors.js | 12 ++++ 9 files changed, 124 insertions(+), 51 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 41810271a42..61b9d51e8ab 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -1,6 +1,7 @@ 'use strict'; const async_wrap = process.binding('async_wrap'); +const errors = require('internal/errors'); /* async_hook_fields is a Uint32Array wrapping the uint32_t array of * Environment::AsyncHooks::fields_[]. Each index tracks the number of active * hooks for each type. @@ -108,13 +109,13 @@ function fatalError(e) { class AsyncHook { constructor({ init, before, after, destroy }) { if (init !== undefined && typeof init !== 'function') - throw new TypeError('init must be a function'); + throw new errors.TypeError('ERR_ASYNC_CALLBACK', 'init'); if (before !== undefined && typeof before !== 'function') - throw new TypeError('before must be a function'); + throw new errors.TypeError('ERR_ASYNC_CALLBACK', 'before'); if (after !== undefined && typeof after !== 'function') - throw new TypeError('after must be a function'); + throw new errors.TypeError('ERR_ASYNC_CALLBACK', 'before'); if (destroy !== undefined && typeof destroy !== 'function') - throw new TypeError('destroy must be a function'); + throw new errors.TypeError('ERR_ASYNC_CALLBACK', 'before'); this[init_symbol] = init; this[before_symbol] = before; @@ -235,8 +236,11 @@ class AsyncResource { constructor(type, triggerAsyncId = initTriggerId()) { // Unlike emitInitScript, AsyncResource doesn't supports null as the // triggerAsyncId. - if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < 0) - throw new RangeError('triggerAsyncId must be an unsigned integer'); + if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < -1) { + throw new errors.RangeError('ERR_INVALID_ASYNC_ID', + 'triggerAsyncId', + triggerAsyncId); + } this[async_id_symbol] = ++async_uid_fields[kAsyncUidCntr]; this[trigger_id_symbol] = triggerAsyncId; @@ -330,14 +334,17 @@ function emitInitScript(asyncId, type, triggerAsyncId, resource) { async_uid_fields[kInitTriggerId] = 0; } - // TODO(trevnorris): I'd prefer allowing these checks to not exist, or only - // throw in a debug build, in order to improve performance. - if (!Number.isSafeInteger(asyncId) || asyncId < 0) - throw new RangeError('asyncId must be an unsigned integer'); - if (typeof type !== 'string' || type.length <= 0) - throw new TypeError('type must be a string with length > 0'); - if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < 0) - throw new RangeError('triggerAsyncId must be an unsigned integer'); + if (!Number.isSafeInteger(asyncId) || asyncId < -1) { + throw new errors.RangeError('ERR_INVALID_ASYNC_ID', 'asyncId', asyncId); + } + if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < -1) { + throw new errors.RangeError('ERR_INVALID_ASYNC_ID', + 'triggerAsyncId', + triggerAsyncId); + } + if (typeof type !== 'string' || type.length <= 0) { + throw new errors.TypeError('ERR_ASYNC_TYPE', type); + } emitInitNative(asyncId, type, triggerAsyncId, resource); } @@ -381,13 +388,17 @@ function emitHookFactory(symbol, name) { function emitBeforeScript(asyncId, triggerAsyncId) { - // CHECK(Number.isSafeInteger(asyncId) && asyncId > 0) - // CHECK(Number.isSafeInteger(triggerAsyncId) && triggerAsyncId > 0) - - // Validate the ids. - if (asyncId < 0 || triggerAsyncId < 0) { - fatalError('before(): asyncId or triggerAsyncId is less than zero ' + - `(asyncId: ${asyncId}, triggerAsyncId: ${triggerAsyncId})`); + // Validate the ids. An id of -1 means it was never set and is visible on the + // call graph. An id < -1 should never happen in any circumstance. Throw + // on user calls because async state should still be recoverable. + if (!Number.isSafeInteger(asyncId) || asyncId < -1) { + fatalError( + new errors.RangeError('ERR_INVALID_ASYNC_ID', 'asyncId', asyncId)); + } + if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < -1) { + fatalError(new errors.RangeError('ERR_INVALID_ASYNC_ID', + 'triggerAsyncId', + triggerAsyncId)); } pushAsyncIds(asyncId, triggerAsyncId); @@ -398,6 +409,11 @@ function emitBeforeScript(asyncId, triggerAsyncId) { function emitAfterScript(asyncId) { + if (!Number.isSafeInteger(asyncId) || asyncId < -1) { + fatalError( + new errors.RangeError('ERR_INVALID_ASYNC_ID', 'asyncId', asyncId)); + } + if (async_hook_fields[kAfter] > 0) emitAfterNative(asyncId); @@ -406,9 +422,13 @@ function emitAfterScript(asyncId) { function emitDestroyScript(asyncId) { - // Return early if there are no destroy callbacks, or on attempt to emit - // destroy on the void. - if (async_hook_fields[kDestroy] === 0 || asyncId === 0) + if (!Number.isSafeInteger(asyncId) || asyncId < -1) { + fatalError( + new errors.RangeError('ERR_INVALID_ASYNC_ID', 'asyncId', asyncId)); + } + + // Return early if there are no destroy callbacks, or invalid asyncId. + if (async_hook_fields[kDestroy] === 0 || asyncId <= 0) return; async_wrap.addIdToDestroyList(asyncId); } diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 50a7dd51b59..3eee3371460 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -101,6 +101,8 @@ module.exports = exports = { // Note: Please try to keep these in alphabetical order E('ERR_ARG_NOT_ITERABLE', '%s must be iterable'); E('ERR_ASSERTION', '%s'); +E('ERR_ASYNC_CALLBACK', (name) => `${name} must be a function`); +E('ERR_ASYNC_TYPE', (s) => `Invalid name for async "type": ${s}`); E('ERR_BUFFER_OUT_OF_BOUNDS', bufferOutOfBounds); E('ERR_CHILD_CLOSED_BEFORE_REPLY', 'Child closed before reply received'); E('ERR_CONSOLE_WRITABLE_STREAM', @@ -188,6 +190,7 @@ E('ERR_INVALID_ARRAY_LENGTH', assert.strictEqual(typeof actual, 'number'); return `The array "${name}" (length ${actual}) must be of length ${len}.`; }); +E('ERR_INVALID_ASYNC_ID', (type, id) => `Invalid ${type} value: ${id}`); E('ERR_INVALID_BUFFER_SIZE', 'Buffer size must be a multiple of %s'); E('ERR_INVALID_CALLBACK', 'Callback must be a function'); E('ERR_INVALID_CHAR', invalidChar); diff --git a/src/env-inl.h b/src/env-inl.h index 9597deaa09b..5d6211b7b69 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -127,8 +127,8 @@ inline v8::Local Environment::AsyncHooks::provider_string(int idx) { inline void Environment::AsyncHooks::push_ids(double async_id, double trigger_id) { - CHECK_GE(async_id, 0); - CHECK_GE(trigger_id, 0); + CHECK_GE(async_id, -1); + CHECK_GE(trigger_id, -1); ids_stack_.push({ uid_fields_[kCurrentAsyncId], uid_fields_[kCurrentTriggerId] }); @@ -177,6 +177,7 @@ inline Environment::AsyncHooks::InitScope::InitScope( Environment* env, double init_trigger_id) : env_(env), uid_fields_ref_(env->async_hooks()->uid_fields()) { + CHECK_GE(init_trigger_id, -1); env->async_hooks()->push_ids(uid_fields_ref_[AsyncHooks::kCurrentAsyncId], init_trigger_id); } @@ -190,6 +191,8 @@ inline Environment::AsyncHooks::ExecScope::ExecScope( : env_(env), async_id_(async_id), disposed_(false) { + CHECK_GE(async_id, -1); + CHECK_GE(trigger_id, -1); env->async_hooks()->push_ids(async_id, trigger_id); } diff --git a/test/async-hooks/test-embedder.api.async-resource.js b/test/async-hooks/test-embedder.api.async-resource.js index 78985b955ad..533df6a5c84 100644 --- a/test/async-hooks/test-embedder.api.async-resource.js +++ b/test/async-hooks/test-embedder.api.async-resource.js @@ -12,10 +12,18 @@ const { checkInvocations } = require('./hook-checks'); const hooks = initHooks(); hooks.enable(); -assert.throws(() => new AsyncResource(), - /^TypeError: type must be a string with length > 0$/); -assert.throws(() => new AsyncResource('invalid_trigger_id', null), - /^RangeError: triggerAsyncId must be an unsigned integer$/); +assert.throws(() => { + new AsyncResource(); +}, common.expectsError({ + code: 'ERR_ASYNC_TYPE', + type: TypeError, +})); +assert.throws(() => { + new AsyncResource('invalid_trigger_id', null); +}, common.expectsError({ + code: 'ERR_INVALID_ASYNC_ID', + type: RangeError, +})); assert.strictEqual( new AsyncResource('default_trigger_id').triggerAsyncId(), diff --git a/test/async-hooks/test-emit-before-after.js b/test/async-hooks/test-emit-before-after.js index 1334767c766..2b22739fa94 100644 --- a/test/async-hooks/test-emit-before-after.js +++ b/test/async-hooks/test-emit-before-after.js @@ -8,25 +8,25 @@ const initHooks = require('./init-hooks'); switch (process.argv[2]) { case 'test_invalid_async_id': - async_hooks.emitBefore(-1, 1); + async_hooks.emitBefore(-2, 1); return; case 'test_invalid_trigger_id': - async_hooks.emitBefore(1, -1); + async_hooks.emitBefore(1, -2); return; } assert.ok(!process.argv[2]); const c1 = spawnSync(process.execPath, [__filename, 'test_invalid_async_id']); -assert.strictEqual(c1.stderr.toString().split(/[\r\n]+/g)[0], - 'Error: before(): asyncId or triggerAsyncId is less than ' + - 'zero (asyncId: -1, triggerAsyncId: 1)'); +assert.strictEqual( + c1.stderr.toString().split(/[\r\n]+/g)[0], + 'RangeError [ERR_INVALID_ASYNC_ID]: Invalid asyncId value: -2'); assert.strictEqual(c1.status, 1); const c2 = spawnSync(process.execPath, [__filename, 'test_invalid_trigger_id']); -assert.strictEqual(c2.stderr.toString().split(/[\r\n]+/g)[0], - 'Error: before(): asyncId or triggerAsyncId is less than ' + - 'zero (asyncId: 1, triggerAsyncId: -1)'); +assert.strictEqual( + c2.stderr.toString().split(/[\r\n]+/g)[0], + 'RangeError [ERR_INVALID_ASYNC_ID]: Invalid triggerAsyncId value: -2'); assert.strictEqual(c2.status, 1); const expectedId = async_hooks.newUid(); diff --git a/test/async-hooks/test-emit-init.js b/test/async-hooks/test-emit-init.js index feee3d944b8..631dcd75996 100644 --- a/test/async-hooks/test-emit-init.js +++ b/test/async-hooks/test-emit-init.js @@ -25,12 +25,24 @@ const hooks1 = initHooks({ hooks1.enable(); -assert.throws(() => async_hooks.emitInit(), - /^RangeError: asyncId must be an unsigned integer$/); -assert.throws(() => async_hooks.emitInit(expectedId), - /^TypeError: type must be a string with length > 0$/); -assert.throws(() => async_hooks.emitInit(expectedId, expectedType, -1), - /^RangeError: triggerAsyncId must be an unsigned integer$/); +assert.throws(() => { + async_hooks.emitInit(); +}, common.expectsError({ + code: 'ERR_INVALID_ASYNC_ID', + type: RangeError, +})); +assert.throws(() => { + async_hooks.emitInit(expectedId); +}, common.expectsError({ + code: 'ERR_INVALID_ASYNC_ID', + type: RangeError, +})); +assert.throws(() => { + async_hooks.emitInit(expectedId, expectedType, -2); +}, common.expectsError({ + code: 'ERR_INVALID_ASYNC_ID', + type: RangeError, +})); async_hooks.emitInit(expectedId, expectedType, expectedTriggerId, expectedResource); diff --git a/test/parallel/test-async-hooks-asyncresource-constructor.js b/test/parallel/test-async-hooks-asyncresource-constructor.js index 2fb0bb14ccf..2ab5f067ca6 100644 --- a/test/parallel/test-async-hooks-asyncresource-constructor.js +++ b/test/parallel/test-async-hooks-asyncresource-constructor.js @@ -1,8 +1,8 @@ 'use strict'; -require('../common'); // This tests that AsyncResource throws an error if bad parameters are passed +const common = require('../common'); const assert = require('assert'); const async_hooks = require('async_hooks'); const { AsyncResource } = async_hooks; @@ -14,16 +14,28 @@ async_hooks.createHook({ assert.throws(() => { return new AsyncResource(); -}, /^TypeError: type must be a string with length > 0$/); +}, common.expectsError({ + code: 'ERR_ASYNC_TYPE', + type: TypeError, +})); assert.throws(() => { new AsyncResource(''); -}, /^TypeError: type must be a string with length > 0$/); +}, common.expectsError({ + code: 'ERR_ASYNC_TYPE', + type: TypeError, +})); assert.throws(() => { new AsyncResource('type', -4); -}, /^RangeError: triggerAsyncId must be an unsigned integer$/); +}, common.expectsError({ + code: 'ERR_INVALID_ASYNC_ID', + type: RangeError, +})); assert.throws(() => { new AsyncResource('type', Math.PI); -}, /^RangeError: triggerAsyncId must be an unsigned integer$/); +}, common.expectsError({ + code: 'ERR_INVALID_ASYNC_ID', + type: RangeError, +})); diff --git a/test/parallel/test-async-wrap-constructor.js b/test/parallel/test-async-wrap-constructor.js index 4f344fd99bc..86fce0e3f39 100644 --- a/test/parallel/test-async-wrap-constructor.js +++ b/test/parallel/test-async-wrap-constructor.js @@ -1,8 +1,8 @@ 'use strict'; -require('../common'); // This tests that using falsy values in createHook throws an error. +const common = require('../common'); const assert = require('assert'); const async_hooks = require('async_hooks'); @@ -10,6 +10,9 @@ for (const badArg of [0, 1, false, true, null, 'hello']) { for (const field of ['init', 'before', 'after', 'destroy']) { assert.throws(() => { async_hooks.createHook({ [field]: badArg }); - }, new RegExp(`^TypeError: ${field} must be a function$`)); + }, common.expectsError({ + code: 'ERR_ASYNC_CALLBACK', + type: TypeError, + })); } } diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js index 21c8de8e0da..627dd2f7817 100644 --- a/test/parallel/test-internal-errors.js +++ b/test/parallel/test-internal-errors.js @@ -260,3 +260,15 @@ assert.strictEqual( errors.message('ERR_UNESCAPED_CHARACTERS', ['Request path']), 'Request path contains unescaped characters' ); + + +// Test error messages for async_hooks +assert.strictEqual( + errors.message('ERR_ASYNC_CALLBACK', ['init']), + 'init must be a function'); +assert.strictEqual( + errors.message('ERR_ASYNC_TYPE', [{}]), + 'Invalid name for async "type": [object Object]'); +assert.strictEqual( + errors.message('ERR_INVALID_ASYNC_ID', ['asyncId', undefined]), + 'Invalid asyncId value: undefined');