Skip to content

Commit

Permalink
async_hooks: don't abort unnecessarily
Browse files Browse the repository at this point in the history
* 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: #14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
  • Loading branch information
trevnorris authored and MylesBorins committed Sep 11, 2017
1 parent bf40f9d commit 02020ef
Show file tree
Hide file tree
Showing 8 changed files with 112 additions and 51 deletions.
68 changes: 44 additions & 24 deletions lib/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const internalUtil = require('internal/util');
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.
Expand Down Expand Up @@ -109,13 +110,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;
Expand Down Expand Up @@ -236,8 +237,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;
Expand Down Expand Up @@ -342,14 +346,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);
}
Expand Down Expand Up @@ -393,13 +400,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);
Expand All @@ -410,6 +421,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);

Expand All @@ -418,9 +434,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);
}
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,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', (msg) => msg);
E('ERR_ASYNC_CALLBACK', (name) => `${name} must be a function`);
E('ERR_ASYNC_TYPE', (s) => `Invalid name for async "type": ${s}`);
E('ERR_ENCODING_INVALID_ENCODED_DATA',
(enc) => `The encoded data was not valid for encoding ${enc}`);
E('ERR_ENCODING_NOT_SUPPORTED',
Expand Down Expand Up @@ -184,6 +186,7 @@ E('ERR_HTTP2_UNSUPPORTED_PROTOCOL',
(protocol) => `protocol "${protocol}" is unsupported.`);
E('ERR_INDEX_OUT_OF_RANGE', 'Index out of range');
E('ERR_INVALID_ARG_TYPE', invalidArgType);
E('ERR_INVALID_ASYNC_ID', (type, id) => `Invalid ${type} value: ${id}`);
E('ERR_INVALID_CALLBACK', 'callback must be a function');
E('ERR_INVALID_FD', (fd) => `"fd" must be a positive integer: ${fd}`);
E('ERR_INVALID_FILE_URL_HOST', 'File URL host %s');
Expand Down
7 changes: 5 additions & 2 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,8 @@ inline v8::Local<v8::String> 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] });
Expand Down Expand Up @@ -181,6 +181,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);
}
Expand All @@ -194,6 +195,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);
}

Expand Down
16 changes: 12 additions & 4 deletions test/async-hooks/test-embedder.api.async-resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
16 changes: 8 additions & 8 deletions test/async-hooks/test-emit-before-after.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
24 changes: 18 additions & 6 deletions test/async-hooks/test-emit-init.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
22 changes: 17 additions & 5 deletions test/parallel/test-async-hooks-asyncresource-constructor.js
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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,
}));
7 changes: 5 additions & 2 deletions test/parallel/test-async-wrap-constructor.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
'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');

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,
}));
}
}

0 comments on commit 02020ef

Please sign in to comment.