From b4d48c05167f27f0b1e6d7d8432ea3ffb05014a6 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 2 Dec 2019 13:02:48 +0100 Subject: [PATCH] assert,util: stricter type comparison using deep equal comparisons This veryfies that both input arguments are always of the identical type. It was possible to miss a few cases before. This change applies to all deep equal assert functions (e.g., `assert.deepStrictEqual()`) and to `util.isDeepStrictEqual()`. PR-URL: https://github.com/nodejs/node/pull/30764 Refs: https://github.com/nodejs/node/pull/30743 Reviewed-By: James M Snell Reviewed-By: David Carlier Reviewed-By: Rich Trott --- lib/internal/util/comparisons.js | 89 +++++++++++++++++++++++-------- test/parallel/test-assert-deep.js | 66 +++++++++++++++++++++++ 2 files changed, 133 insertions(+), 22 deletions(-) diff --git a/lib/internal/util/comparisons.js b/lib/internal/util/comparisons.js index b2784217171410..734ecf6224a0bf 100644 --- a/lib/internal/util/comparisons.js +++ b/lib/internal/util/comparisons.js @@ -19,6 +19,7 @@ const { } = primordials; const { compare } = internalBinding('buffer'); +const types = require('internal/util/types'); const { isAnyArrayBuffer, isArrayBufferView, @@ -34,8 +35,17 @@ const { isBigIntObject, isSymbolObject, isFloat32Array, - isFloat64Array -} = require('internal/util/types'); + isFloat64Array, + isUint8Array, + isUint8ClampedArray, + isUint16Array, + isUint32Array, + isInt8Array, + isInt16Array, + isInt32Array, + isBigInt64Array, + isBigUint64Array +} = types; const { getOwnNonIndexProperties, propertyFilter: { @@ -107,6 +117,33 @@ function isEqualBoxedPrimitive(val1, val2) { return false; } +function isIdenticalTypedArrayType(a, b) { + // Fast path to reduce type checks in the common case. + const check = types[`is${a[Symbol.toStringTag]}`]; + if (check !== undefined && check(a)) { + return check(b); + } + // Manipulated Symbol.toStringTag. + for (const check of [ + isUint16Array, + isUint32Array, + isInt8Array, + isInt16Array, + isInt32Array, + isFloat32Array, + isFloat64Array, + isBigInt64Array, + isBigUint64Array, + isUint8ClampedArray, + isUint8Array + ]) { + if (check(a)) { + return check(b); + } + } + return false; +} + // Notes: Type tags are historical [[Class]] properties that can be set by // FunctionTemplate::SetClassName() in C++ or Symbol.toStringTag in JS // and retrieved using Object.prototype.toString.call(obj) in JS @@ -115,15 +152,8 @@ function isEqualBoxedPrimitive(val1, val2) { // There are some unspecified tags in the wild too (e.g. typed array tags). // Since tags can be altered, they only serve fast failures // -// Typed arrays and buffers are checked by comparing the content in their -// underlying ArrayBuffer. This optimization requires that it's -// reasonable to interpret their underlying memory in the same way, -// which is checked by comparing their type tags. -// (e.g. a Uint8Array and a Uint16Array with the same memory content -// could still be different because they will be interpreted differently). -// // For strict comparison, objects should have -// a) The same built-in type tags +// a) The same built-in type tag. // b) The same prototypes. function innerDeepEqual(val1, val2, strict, memos) { @@ -164,9 +194,10 @@ function innerDeepEqual(val1, val2, strict, memos) { if (val1Tag !== val2Tag) { return false; } + if (ArrayIsArray(val1)) { // Check for sparse arrays and general fast path - if (val1.length !== val2.length) { + if (!ArrayIsArray(val2) || val1.length !== val2.length) { return false; } const filter = strict ? ONLY_ENUMERABLE : ONLY_ENUMERABLE | SKIP_SYMBOLS; @@ -176,25 +207,28 @@ function innerDeepEqual(val1, val2, strict, memos) { return false; } return keyCheck(val1, val2, strict, memos, kIsArray, keys1); - } - if (val1Tag === '[object Object]') { + } else if (val1Tag === '[object Object]') { return keyCheck(val1, val2, strict, memos, kNoIterator); - } - if (isDate(val1)) { - if (DatePrototypeGetTime(val1) !== DatePrototypeGetTime(val2)) { + } else if (isDate(val1)) { + if (!isDate(val2) || + DatePrototypeGetTime(val1) !== DatePrototypeGetTime(val2)) { return false; } } else if (isRegExp(val1)) { - if (!areSimilarRegExps(val1, val2)) { + if (!isRegExp(val2) || !areSimilarRegExps(val1, val2)) { return false; } } else if (isNativeError(val1) || val1 instanceof Error) { // Do not compare the stack as it might differ even though the error itself // is otherwise identical. - if (val1.message !== val2.message || val1.name !== val2.name) { + if ((!isNativeError(val2) && !(val2 instanceof Error)) || + val1.message !== val2.message || + val1.name !== val2.name) { return false; } } else if (isArrayBufferView(val1)) { + if (!isIdenticalTypedArrayType(val1, val2)) + return false; if (!strict && (isFloat32Array(val1) || isFloat64Array(val1))) { if (!areSimilarFloatArrays(val1, val2)) { return false; @@ -223,12 +257,23 @@ function innerDeepEqual(val1, val2, strict, memos) { } return keyCheck(val1, val2, strict, memos, kIsMap); } else if (isAnyArrayBuffer(val1)) { - if (!areEqualArrayBuffers(val1, val2)) { + if (!isAnyArrayBuffer(val2) || !areEqualArrayBuffers(val1, val2)) { return false; } - } - if ((isBoxedPrimitive(val1) || isBoxedPrimitive(val2)) && - !isEqualBoxedPrimitive(val1, val2)) { + } else if (isBoxedPrimitive(val1)) { + if (!isEqualBoxedPrimitive(val1, val2)) { + return false; + } + } else if (ArrayIsArray(val2) || + isArrayBufferView(val2) || + isSet(val2) || + isMap(val2) || + isDate(val2) || + isRegExp(val2) || + isAnyArrayBuffer(val2) || + isBoxedPrimitive(val2) || + isNativeError(val2) || + val2 instanceof Error) { return false; } return keyCheck(val1, val2, strict, memos, kNoIterator); diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js index 87f2a5f44f7d64..1cddb476925a13 100644 --- a/test/parallel/test-assert-deep.js +++ b/test/parallel/test-assert-deep.js @@ -1123,3 +1123,69 @@ assert.throws( // The descriptor is not compared. assertDeepAndStrictEqual(a, { a: 5 }); } + +// Verify object types being identical on both sides. +{ + let a = Buffer.from('test'); + let b = Object.create( + Object.getPrototypeOf(a), + Object.getOwnPropertyDescriptors(a) + ); + Object.defineProperty(b, Symbol.toStringTag, { + value: 'Uint8Array' + }); + assertNotDeepOrStrict(a, b); + + a = new Uint8Array(10); + b = new Int8Array(10); + Object.defineProperty(b, Symbol.toStringTag, { + value: 'Uint8Array' + }); + Object.setPrototypeOf(b, Uint8Array.prototype); + assertNotDeepOrStrict(a, b); + + a = [1, 2, 3]; + b = { 0: 1, 1: 2, 2: 3 }; + Object.setPrototypeOf(b, Array.prototype); + Object.defineProperty(b, 'length', { value: 3, enumerable: false }); + Object.defineProperty(b, Symbol.toStringTag, { + value: 'Array' + }); + assertNotDeepOrStrict(a, b); + + a = new Date(2000); + b = Object.create( + Object.getPrototypeOf(a), + Object.getOwnPropertyDescriptors(a) + ); + Object.defineProperty(b, Symbol.toStringTag, { + value: 'Date' + }); + assertNotDeepOrStrict(a, b); + + a = /abc/g; + b = Object.create( + Object.getPrototypeOf(a), + Object.getOwnPropertyDescriptors(a) + ); + Object.defineProperty(b, Symbol.toStringTag, { + value: 'RegExp' + }); + assertNotDeepOrStrict(a, b); + + a = []; + b = /abc/; + Object.setPrototypeOf(b, Array.prototype); + Object.defineProperty(b, Symbol.toStringTag, { + value: 'Array' + }); + assertNotDeepOrStrict(a, b); + + a = Object.create(null); + b = new RangeError('abc'); + Object.defineProperty(a, Symbol.toStringTag, { + value: 'Error' + }); + Object.setPrototypeOf(b, null); + assertNotDeepOrStrict(a, b, assert.AssertionError); +}