From 3355c26c3f58863aec11ec26e419fed5eecb44a4 Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Sun, 6 Nov 2016 19:01:19 +0530 Subject: [PATCH 1/3] buffer: convert offset & length to int properly As per ecma-262 2015's #sec-%typedarray%-buffer-byteoffset-length, `offset` would be an integer, not a 32 bit unsigned integer. Also, `length` would be an integer with the maximum value of 2^53 - 1, not a 32 bit unsigned integer. This would be a problem because, if we create a buffer from an arraybuffer, from an offset which is greater than 2^32, it would be actually pointing to a different location in arraybuffer. For example, if we use 2^40 as offset, then the actual value used will be 0, because `byteOffset >>>= 0` will convert `byteOffset` to a 32 bit unsigned int, which is based on 2^32 modulo. --- lib/buffer.js | 4 +-- lib/internal/util.js | 28 +++++++++++++++++++ .../test-buffer-creation-regression.js | 21 ++++++++++++++ test/parallel/test-internal-util-toInteger.js | 19 +++++++++++++ test/parallel/test-internal-util-toLength.js | 20 +++++++++++++ 5 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-buffer-creation-regression.js create mode 100644 test/parallel/test-internal-util-toInteger.js create mode 100644 test/parallel/test-internal-util-toLength.js diff --git a/lib/buffer.js b/lib/buffer.js index b2325098bcbb9d..94bf9cdca30408 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -238,7 +238,7 @@ function fromArrayLike(obj) { } function fromArrayBuffer(obj, byteOffset, length) { - byteOffset >>>= 0; + byteOffset = internalUtil.toInteger(byteOffset); const maxLength = obj.byteLength - byteOffset; @@ -248,7 +248,7 @@ function fromArrayBuffer(obj, byteOffset, length) { if (length === undefined) { length = maxLength; } else { - length >>>= 0; + length = internalUtil.toLength(length); if (length > maxLength) throw new RangeError("'length' is out of bounds"); } diff --git a/lib/internal/util.js b/lib/internal/util.js index 4ada8dd0cc16f0..007bd9fa2775e6 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -161,3 +161,31 @@ exports.cachedResult = function cachedResult(fn) { return result; }; }; + +/* + * Implementation of ToInteger as per ECMAScript Specification + * Refer: http://www.ecma-international.org/ecma-262/6.0/#sec-tointeger + */ +const toInteger = exports.toInteger = function toInteger(argument) { + const number = +argument; + if (Number.isNaN(number) || number === 0) { + return 0; + } else if (!Number.isFinite(number)) { + return number; + } else { + return Math.trunc(number); + } +}; + +/* + * Implementation of ToLength as per ECMAScript Specification + * Refer: http://www.ecma-international.org/ecma-262/6.0/#sec-tolength + */ +exports.toLength = function toLength(argument) { + const len = toInteger(argument); + if (len <= 0) { + return 0; + } else { + return Math.min(len, Math.pow(2, 53) - 1); + } +}; diff --git a/test/parallel/test-buffer-creation-regression.js b/test/parallel/test-buffer-creation-regression.js new file mode 100644 index 00000000000000..10ee68c6834c5b --- /dev/null +++ b/test/parallel/test-buffer-creation-regression.js @@ -0,0 +1,21 @@ +'use strict'; + +require('../common'); +const assert = require('assert'); + +function test(size, offset, length) { + const arrayBuffer = new ArrayBuffer(size); + + const uint8Array = new Uint8Array(arrayBuffer, offset, length); + for (let i = 0; i < length; i += 1) { + uint8Array[i] = 1; + } + + const buffer = new Buffer(arrayBuffer, offset, length); + for (let i = 0; i < length; i += 1) { + assert.strictEqual(buffer[i], 1); + } +} + +test(200, 50, 100); +test(8589934592 /* 1 << 40 */, 4294967296 /* 1 << 39 */, 1000); diff --git a/test/parallel/test-internal-util-toInteger.js b/test/parallel/test-internal-util-toInteger.js new file mode 100644 index 00000000000000..35ddfa4fe536de --- /dev/null +++ b/test/parallel/test-internal-util-toInteger.js @@ -0,0 +1,19 @@ +// Flags: --expose-internals +'use strict'; + +require('../common'); +const assert = require('assert'); +const {toInteger} = require('internal/util'); + +assert.strictEqual(toInteger(+0), 0); +assert.strictEqual(toInteger(-0), 0); +assert.strictEqual(toInteger(NaN), 0); +assert.strictEqual(toInteger(Infinity), Infinity); +assert.strictEqual(toInteger(-Infinity), -Infinity); + +assert.strictEqual(toInteger(0.1), 0); +assert.strictEqual(toInteger(-0.1), 0); +assert.strictEqual(toInteger(0.5), 0); +assert.strictEqual(toInteger(-0.5), 0); +assert.strictEqual(toInteger(0.9), 0); +assert.strictEqual(toInteger(-0.9), 0); diff --git a/test/parallel/test-internal-util-toLength.js b/test/parallel/test-internal-util-toLength.js new file mode 100644 index 00000000000000..f5a8b1ec52e711 --- /dev/null +++ b/test/parallel/test-internal-util-toLength.js @@ -0,0 +1,20 @@ +// Flags: --expose-internals +'use strict'; + +require('../common'); +const assert = require('assert'); +const {toLength} = require('internal/util'); + +assert.strictEqual(toLength(+0), 0); +assert.strictEqual(toLength(-0), 0); +assert.strictEqual(toLength(NaN), 0); +assert.strictEqual(toLength(Math.pow(2, 60)), 9007199254740991); +assert.strictEqual(toLength(Infinity), 9007199254740991); +assert.strictEqual(toLength(-Infinity), 0); + +assert.strictEqual(toLength(0.1), 0); +assert.strictEqual(toLength(-0.1), 0); +assert.strictEqual(toLength(0.5), 0); +assert.strictEqual(toLength(-0.5), 0); +assert.strictEqual(toLength(0.9), 0); +assert.strictEqual(toLength(-0.9), 0); From 241d32a01e255490231b70ac0ae06a47988a86a0 Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Sat, 12 Nov 2016 17:03:44 +0530 Subject: [PATCH 2/3] simplify implementation and improve tests --- lib/internal/util.js | 14 +------ test/parallel/test-internal-util-toInteger.js | 31 ++++++++++----- test/parallel/test-internal-util-toLength.js | 39 +++++++++++++------ 3 files changed, 51 insertions(+), 33 deletions(-) diff --git a/lib/internal/util.js b/lib/internal/util.js index 007bd9fa2775e6..ae8b1e0b649486 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -168,13 +168,7 @@ exports.cachedResult = function cachedResult(fn) { */ const toInteger = exports.toInteger = function toInteger(argument) { const number = +argument; - if (Number.isNaN(number) || number === 0) { - return 0; - } else if (!Number.isFinite(number)) { - return number; - } else { - return Math.trunc(number); - } + return Number.isNaN(number) ? 0 : Math.trunc(number); }; /* @@ -183,9 +177,5 @@ const toInteger = exports.toInteger = function toInteger(argument) { */ exports.toLength = function toLength(argument) { const len = toInteger(argument); - if (len <= 0) { - return 0; - } else { - return Math.min(len, Math.pow(2, 53) - 1); - } + return len <= 0 ? 0 : Math.min(len, Number.MAX_SAFE_INTEGER); }; diff --git a/test/parallel/test-internal-util-toInteger.js b/test/parallel/test-internal-util-toInteger.js index 35ddfa4fe536de..57a411964da90f 100644 --- a/test/parallel/test-internal-util-toInteger.js +++ b/test/parallel/test-internal-util-toInteger.js @@ -5,15 +5,28 @@ require('../common'); const assert = require('assert'); const {toInteger} = require('internal/util'); -assert.strictEqual(toInteger(+0), 0); -assert.strictEqual(toInteger(-0), 0); -assert.strictEqual(toInteger(NaN), 0); +const expectZero = [ + '0', '-0', NaN, {}, [], {'a': 'b'}, [1, 2], '0x', '0o', '0b', false, + '', ' ', undefined, null +]; +expectZero.forEach(function(value) { + assert.strictEqual(toInteger(value), 0); +}); + assert.strictEqual(toInteger(Infinity), Infinity); assert.strictEqual(toInteger(-Infinity), -Infinity); -assert.strictEqual(toInteger(0.1), 0); -assert.strictEqual(toInteger(-0.1), 0); -assert.strictEqual(toInteger(0.5), 0); -assert.strictEqual(toInteger(-0.5), 0); -assert.strictEqual(toInteger(0.9), 0); -assert.strictEqual(toInteger(-0.9), 0); +const expectSame = [ + '0x100', '0o100', '0b100', 0x100, -0x100, 0o100, -0o100, 0b100, -0b100, true +]; +expectSame.forEach(function(value) { + assert.strictEqual(toInteger(value), +value, `${value} is not an Integer`); +}); + +const expectIntegers = new Map([ + [[1], 1], [[-1], -1], [['1'], 1], [['-1'], -1], + [3.14, 3], [-3.14, -3], ['3.14', 3], ['-3.14', -3], +]); +expectIntegers.forEach(function(expected, value) { + assert.strictEqual(toInteger(value), expected); +}); diff --git a/test/parallel/test-internal-util-toLength.js b/test/parallel/test-internal-util-toLength.js index f5a8b1ec52e711..ce594c47c1db19 100644 --- a/test/parallel/test-internal-util-toLength.js +++ b/test/parallel/test-internal-util-toLength.js @@ -4,17 +4,32 @@ require('../common'); const assert = require('assert'); const {toLength} = require('internal/util'); +const maxValue = Number.MAX_SAFE_INTEGER; -assert.strictEqual(toLength(+0), 0); -assert.strictEqual(toLength(-0), 0); -assert.strictEqual(toLength(NaN), 0); -assert.strictEqual(toLength(Math.pow(2, 60)), 9007199254740991); -assert.strictEqual(toLength(Infinity), 9007199254740991); -assert.strictEqual(toLength(-Infinity), 0); +const expectZero = [ + '0', '-0', NaN, {}, [], {'a': 'b'}, [1, 2], '0x', '0o', '0b', false, + '', ' ', undefined, null, -1, -1.25, -1.1, -1.9, -Infinity +]; +expectZero.forEach(function(value) { + assert.strictEqual(toLength(value), 0); +}); -assert.strictEqual(toLength(0.1), 0); -assert.strictEqual(toLength(-0.1), 0); -assert.strictEqual(toLength(0.5), 0); -assert.strictEqual(toLength(-0.5), 0); -assert.strictEqual(toLength(0.9), 0); -assert.strictEqual(toLength(-0.9), 0); +assert.strictEqual(toLength(maxValue - 1), maxValue - 1); +assert.strictEqual(maxValue, maxValue); +assert.strictEqual(toLength(Infinity), maxValue); +assert.strictEqual(toLength(maxValue + 1), maxValue); + + +[ + '0x100', '0o100', '0b100', 0x100, -0x100, 0o100, -0o100, 0b100, -0b100, true +].forEach(function(value) { + assert.strictEqual(toLength(value), +value > 0 ? +value : 0); +}); + +const expectIntegers = new Map([ + [[1], 1], [[-1], 0], [['1'], 1], [['-1'], 0], + [3.14, 3], [-3.14, 0], ['3.14', 3], ['-3.14', 0], +]); +expectIntegers.forEach(function(expected, value) { + assert.strictEqual(toLength(value), expected); +}); From c5f868ad6bae6f4ebee6914c443d7dda1491f4c5 Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Sun, 20 Nov 2016 20:34:19 +0530 Subject: [PATCH 3/3] change to Buffer.from --- test/parallel/test-buffer-creation-regression.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-buffer-creation-regression.js b/test/parallel/test-buffer-creation-regression.js index 10ee68c6834c5b..3432d81bf6eddd 100644 --- a/test/parallel/test-buffer-creation-regression.js +++ b/test/parallel/test-buffer-creation-regression.js @@ -11,7 +11,7 @@ function test(size, offset, length) { uint8Array[i] = 1; } - const buffer = new Buffer(arrayBuffer, offset, length); + const buffer = Buffer.from(arrayBuffer, offset, length); for (let i = 0; i < length; i += 1) { assert.strictEqual(buffer[i], 1); }