From 3802007e401a6357992a670b398513dce91f99d0 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 2 Dec 2019 13:02:48 +0100 Subject: [PATCH 1/3] 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()`. --- lib/internal/util/comparisons.js | 88 +++++++++++++++++++++++-------- test/parallel/test-assert-deep.js | 30 +++++++++++ 2 files changed, 96 insertions(+), 22 deletions(-) diff --git a/lib/internal/util/comparisons.js b/lib/internal/util/comparisons.js index b2784217171410..6d5564967cec75 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,22 @@ 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) || + 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..76bdff0176021d 100644 --- a/test/parallel/test-assert-deep.js +++ b/test/parallel/test-assert-deep.js @@ -1123,3 +1123,33 @@ 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); +} From 17aad5d7aec79cb8d726c67d0615fd02e96ea125 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 2 Dec 2019 13:15:11 +0100 Subject: [PATCH 2/3] test: run more assert tests This makes sure the assertion does not depend on the argument order. It also removes comments that do not apply anymore and verifies the behavior for the loose and strict implementation. --- test/parallel/test-assert-deep.js | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js index 76bdff0176021d..3ec70a55bd2b20 100644 --- a/test/parallel/test-assert-deep.js +++ b/test/parallel/test-assert-deep.js @@ -109,10 +109,7 @@ class MyDate extends Date { const date2 = new MyDate('2016'); -// deepEqual returns true as long as the time are the same, -// but deepStrictEqual checks own properties -assert.notDeepEqual(date, date2); -assert.notDeepEqual(date2, date); +assertNotDeepOrStrict(date, date2); assert.throws( () => assert.deepStrictEqual(date, date2), { @@ -142,9 +139,7 @@ class MyRegExp extends RegExp { const re1 = new RegExp('test'); const re2 = new MyRegExp('test'); -// deepEqual returns true as long as the regexp-specific properties -// are the same, but deepStrictEqual checks all properties -assert.notDeepEqual(re1, re2); +assertNotDeepOrStrict(re1, re2); assert.throws( () => assert.deepStrictEqual(re1, re2), { @@ -684,7 +679,7 @@ assert.throws( } ); -assert.deepEqual(new Date(2000, 3, 14), new Date(2000, 3, 14)); +assertDeepAndStrictEqual(new Date(2000, 3, 14), new Date(2000, 3, 14)); assert.throws(() => { assert.deepEqual(new Date(), new Date(2000, 3, 14)); }, AssertionError, @@ -702,7 +697,7 @@ assert.throws( 'notDeepEqual("a".repeat(1024), "a".repeat(1024))' ); -assert.notDeepEqual(new Date(), new Date(2000, 3, 14)); +assertNotDeepOrStrict(new Date(), new Date(2000, 3, 14)); assertDeepAndStrictEqual(/a/, /a/); assertDeepAndStrictEqual(/a/g, /a/g); @@ -743,7 +738,7 @@ a2.b = true; a2.a = 'test'; assert.throws(() => assert.deepEqual(Object.keys(a1), Object.keys(a2)), AssertionError); -assert.deepEqual(a1, a2); +assertDeepAndStrictEqual(a1, a2); // Having an identical prototype property. const nbRoot = { @@ -899,14 +894,12 @@ assert.throws( /* eslint-enable */ -assert.deepStrictEqual({ a: 4, b: '1' }, { b: '1', a: 4 }); +assertDeepAndStrictEqual({ a: 4, b: '1' }, { b: '1', a: 4 }); assert.throws( () => assert.deepStrictEqual([0, 1, 2, 'a', 'b'], [0, 1, 2, 'b', 'a']), AssertionError); -assert.deepStrictEqual(a1, a2); - // Prototype check. function Constructor1(first, last) { this.first = first; @@ -926,7 +919,7 @@ assert.throws(() => assert.deepStrictEqual(obj1, obj2), AssertionError); Constructor2.prototype = Constructor1.prototype; obj2 = new Constructor2('Ryan', 'Dahl'); -assert.deepStrictEqual(obj1, obj2); +assertDeepAndStrictEqual(obj1, obj2); // Check extra properties on errors. { @@ -1047,7 +1040,7 @@ assert.throws( Object.defineProperty(a, 'getTime', { value: () => 5 }); - assert.deepStrictEqual(a, b); + assertDeepAndStrictEqual(a, b); } // Verify that extra keys will be tested for when using fake arrays. @@ -1064,7 +1057,7 @@ assert.throws( Object.defineProperty(a, 'length', { value: 2 }); - assert.notDeepStrictEqual(a, [1, 1]); + assertNotDeepOrStrict(a, [1, 1]); } // Verify that changed tags will still check for the error message. From 668ab351d195bad1c8ab4578f03d725792436361 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 2 Dec 2019 21:09:46 +0100 Subject: [PATCH 3/3] fixup: fix isRegExp and add more tests --- lib/internal/util/comparisons.js | 1 + test/parallel/test-assert-deep.js | 36 +++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/lib/internal/util/comparisons.js b/lib/internal/util/comparisons.js index 6d5564967cec75..734ecf6224a0bf 100644 --- a/lib/internal/util/comparisons.js +++ b/lib/internal/util/comparisons.js @@ -269,6 +269,7 @@ function innerDeepEqual(val1, val2, strict, memos) { isSet(val2) || isMap(val2) || isDate(val2) || + isRegExp(val2) || isAnyArrayBuffer(val2) || isBoxedPrimitive(val2) || isNativeError(val2) || diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js index 3ec70a55bd2b20..9d30f466f34928 100644 --- a/test/parallel/test-assert-deep.js +++ b/test/parallel/test-assert-deep.js @@ -1145,4 +1145,40 @@ assert.throws( 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); }