From ab135e7bc826d213b88273fb5b2d2e0fbaf3f159 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 18 Nov 2018 04:19:16 +0100 Subject: [PATCH 1/5] util: improve internal `isError()` validation The current internal isError function checked the toString value instead of using the more precise `util.types.isNativeError()` check. The `instanceof` check is not removed due to possible errors that are not native but still an instance of Error. --- lib/internal/util.js | 6 +++++- lib/util.js | 12 ++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/internal/util.js b/lib/internal/util.js index 3524b9e1d62112..d8109778e8288a 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -12,6 +12,9 @@ const { arrow_message_private_symbol: kArrowMessagePrivateSymbolIndex, decorated_private_symbol: kDecoratedPrivateSymbolIndex } = internalBinding('util'); +const { + isNativeError +} = internalBinding('types'); const { errmap } = internalBinding('uv'); @@ -26,7 +29,8 @@ function removeColors(str) { } function isError(e) { - return objectToString(e) === '[object Error]' || e instanceof Error; + // An error could be an instance of Error while not being a native error. + return isNativeError(e) || e instanceof Error; } function objectToString(o) { diff --git a/lib/util.js b/lib/util.js index 22c2b260daf319..c76d77acbc0c8e 100644 --- a/lib/util.js +++ b/lib/util.js @@ -42,10 +42,16 @@ const { const { deprecate, getSystemErrorName: internalErrorName, - isError, promisify, } = require('internal/util'); +const ReflectApply = Reflect.apply; + +function uncurryThis(func) { + return (thisArg, ...args) => ReflectApply(func, thisArg, args); +} +const objectToString = uncurryThis(Object.prototype.toString); + let CIRCULAR_ERROR_MESSAGE; let internalDeepEqual; @@ -434,7 +440,9 @@ module.exports = exports = { isRegExp, isObject, isDate, - isError, + isError(e) { + return objectToString(e) === '[object Error]' || e instanceof Error; + }, isFunction, isPrimitive, log, From 9009e4c3dac037325faaec40af05e029cd95ecd6 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 30 Nov 2018 19:00:10 +0100 Subject: [PATCH 2/5] fixup: add test case --- test/parallel/test-internal-util-helpers.js | 25 +++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 test/parallel/test-internal-util-helpers.js diff --git a/test/parallel/test-internal-util-helpers.js b/test/parallel/test-internal-util-helpers.js new file mode 100644 index 00000000000000..2ab8a1c1434f2c --- /dev/null +++ b/test/parallel/test-internal-util-helpers.js @@ -0,0 +1,25 @@ +// Flags: --expose-internals +'use strict'; + +require('../common'); +const assert = require('assert'); +const { types } = require('util'); +const { isError } = require('internal/util'); + +// Special cased errors. +{ + const fake = { [Symbol.toStringTag]: 'Error' }; + assert(!types.isNativeError(fake)); + assert(!(fake instanceof Error)); + assert(!isError(fake)); + + const err = new Error('test'); + const newErr = Object.create( + Object.getPrototypeOf(err), + Object.getOwnPropertyDescriptors(err)); + Object.defineProperty(err, 'message', { value: err.message }); + assert(types.isNativeError(err)); + assert(!types.isNativeError(newErr)); + assert(newErr instanceof Error); + assert(isError(newErr)); +} From 434eb4120af0bd037e457aa536445c8808c0be10 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 2 Dec 2018 20:48:05 +0100 Subject: [PATCH 3/5] fixup: add further test case --- test/parallel/test-internal-util-helpers.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/parallel/test-internal-util-helpers.js b/test/parallel/test-internal-util-helpers.js index 2ab8a1c1434f2c..60c27f9a2942cf 100644 --- a/test/parallel/test-internal-util-helpers.js +++ b/test/parallel/test-internal-util-helpers.js @@ -5,6 +5,7 @@ require('../common'); const assert = require('assert'); const { types } = require('util'); const { isError } = require('internal/util'); +const vm = require('vm'); // Special cased errors. { @@ -22,4 +23,10 @@ const { isError } = require('internal/util'); assert(!types.isNativeError(newErr)); assert(newErr instanceof Error); assert(isError(newErr)); + + const context = vm.createContext({}); + const differentRealmErr = vm.runInContext('new Error()', context); + assert(types.isNativeError(differentRealmErr)); + assert(!(differentRealmErr instanceof Error)); + assert(isError(differentRealmErr)); } From d3460112cd2c292c75780379dcef6b36db3cbdfe Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 2 Dec 2018 21:05:37 +0100 Subject: [PATCH 4/5] fixup: more verbose comment --- lib/internal/util.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/internal/util.js b/lib/internal/util.js index d8109778e8288a..23c201da371099 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -29,7 +29,9 @@ function removeColors(str) { } function isError(e) { - // An error could be an instance of Error while not being a native error. + // An error could be an instance of Error while not being a native error + // or could be from a different realm and not be instance of Error but still + // be a native error. return isNativeError(e) || e instanceof Error; } From ddc82e42ca2f9caecdc061d08c15cbcba9a2592c Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 2 Dec 2018 21:10:34 +0100 Subject: [PATCH 5/5] fixup: add test explanation --- test/parallel/test-internal-util-helpers.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-internal-util-helpers.js b/test/parallel/test-internal-util-helpers.js index 60c27f9a2942cf..bf60cff9bda4be 100644 --- a/test/parallel/test-internal-util-helpers.js +++ b/test/parallel/test-internal-util-helpers.js @@ -7,7 +7,12 @@ const { types } = require('util'); const { isError } = require('internal/util'); const vm = require('vm'); -// Special cased errors. +// Special cased errors. Test the internal function which is used in +// `util.inspect()`, the `repl` and maybe more. This verifies that errors from +// different realms, and non native instances of error are properly detected as +// error while definitely false ones are not detected. This is different than +// the public `util.isError()` function which falsy detects the fake errors as +// actual errors. { const fake = { [Symbol.toStringTag]: 'Error' }; assert(!types.isNativeError(fake));