From 7a211901958fd0d8ad85234e3516eefb2a70d86e Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Sun, 27 Nov 2016 15:08:17 +0530 Subject: [PATCH 1/2] buffer: convert offset & length to int properly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. This is a redo, as the ca37fa527f174b547893817fe8c67a3befa02317 broke CI. Refer: https://github.com/nodejs/node/pull/9814 Refer: https://github.com/nodejs/node/pull/9492 PR-URL: https://github.com/nodejs/node/pull/9815 Reviewed-By: Michaël Zasso Reviewed-By: Trevor Norris Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Matteo Collina --- lib/buffer.js | 4 +- lib/internal/util.js | 18 ++++++++ .../test-buffer-creation-regression.js | 41 +++++++++++++++++++ test/parallel/test-internal-util-toInteger.js | 32 +++++++++++++++ test/parallel/test-internal-util-toLength.js | 35 ++++++++++++++++ 5 files changed, 128 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 b1c0fc702ec1d5..84a97ea325580a 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -216,7 +216,7 @@ function fromArrayLike(obj) { } function fromArrayBuffer(obj, byteOffset, length) { - byteOffset >>>= 0; + byteOffset = internalUtil.toInteger(byteOffset); const maxLength = obj.byteLength - byteOffset; @@ -226,7 +226,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 ab17d78204039d..8162434da7cfca 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -165,3 +165,21 @@ exports.normalizeEncoding = function normalizeEncoding(enc) { } } }; + +/* + * 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; + return Number.isNaN(number) ? 0 : 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); + return len <= 0 ? 0 : Math.min(len, Number.MAX_SAFE_INTEGER); +}; diff --git a/test/parallel/test-buffer-creation-regression.js b/test/parallel/test-buffer-creation-regression.js new file mode 100644 index 00000000000000..650fbf48f12ae6 --- /dev/null +++ b/test/parallel/test-buffer-creation-regression.js @@ -0,0 +1,41 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); + +function test(arrayBuffer, offset, length) { + const uint8Array = new Uint8Array(arrayBuffer, offset, length); + for (let i = 0; i < length; i += 1) { + uint8Array[i] = 1; + } + + const buffer = Buffer.from(arrayBuffer, offset, length); + for (let i = 0; i < length; i += 1) { + assert.strictEqual(buffer[i], 1); + } +} + +const acceptableOOMErrors = [ + 'Array buffer allocation failed', + 'Invalid array buffer length' +]; + +const testCases = [ + [200, 50, 100], + [4294967296 /* 1 << 32 */, 2147483648 /* 1 << 31 */, 1000], + [8589934592 /* 1 << 33 */, 4294967296 /* 1 << 32 */, 1000] +]; + +for (let index = 0, arrayBuffer; index < testCases.length; index += 1) { + const [size, offset, length] = testCases[index]; + + try { + arrayBuffer = new ArrayBuffer(size); + } catch (e) { + if (e instanceof RangeError && acceptableOOMErrors.includes(e.message)) + return common.skip(`Unable to allocate ${size} bytes for ArrayBuffer`); + throw e; + } + + test(arrayBuffer, offset, length); +} diff --git a/test/parallel/test-internal-util-toInteger.js b/test/parallel/test-internal-util-toInteger.js new file mode 100644 index 00000000000000..57a411964da90f --- /dev/null +++ b/test/parallel/test-internal-util-toInteger.js @@ -0,0 +1,32 @@ +// Flags: --expose-internals +'use strict'; + +require('../common'); +const assert = require('assert'); +const {toInteger} = require('internal/util'); + +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); + +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 new file mode 100644 index 00000000000000..ce594c47c1db19 --- /dev/null +++ b/test/parallel/test-internal-util-toLength.js @@ -0,0 +1,35 @@ +// Flags: --expose-internals +'use strict'; + +require('../common'); +const assert = require('assert'); +const {toLength} = require('internal/util'); +const maxValue = Number.MAX_SAFE_INTEGER; + +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(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 b878aeb4cd153c458f5cc0463bca6de5a05510a0 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 6 Dec 2016 21:11:29 -0800 Subject: [PATCH 2/2] test: move long-running test to sequential test-buffer-creation-regression is flaky on some SmartOS hosts in CI, timing out. Move to sequential so it does not compete with other tests for resources. Reduce three test cases to just the one needed to identify the regression. PR-URL: https://github.com/nodejs/node/pull/10161 Reviewed-By: Santiago Gimeno Reviewed-By: Colin Ihrig Reviewed-By: Italo A. Casas --- .../test-buffer-creation-regression.js | 41 ------------------- .../test-buffer-creation-regression.js | 36 ++++++++++++++++ 2 files changed, 36 insertions(+), 41 deletions(-) delete mode 100644 test/parallel/test-buffer-creation-regression.js create mode 100644 test/sequential/test-buffer-creation-regression.js diff --git a/test/parallel/test-buffer-creation-regression.js b/test/parallel/test-buffer-creation-regression.js deleted file mode 100644 index 650fbf48f12ae6..00000000000000 --- a/test/parallel/test-buffer-creation-regression.js +++ /dev/null @@ -1,41 +0,0 @@ -'use strict'; - -const common = require('../common'); -const assert = require('assert'); - -function test(arrayBuffer, offset, length) { - const uint8Array = new Uint8Array(arrayBuffer, offset, length); - for (let i = 0; i < length; i += 1) { - uint8Array[i] = 1; - } - - const buffer = Buffer.from(arrayBuffer, offset, length); - for (let i = 0; i < length; i += 1) { - assert.strictEqual(buffer[i], 1); - } -} - -const acceptableOOMErrors = [ - 'Array buffer allocation failed', - 'Invalid array buffer length' -]; - -const testCases = [ - [200, 50, 100], - [4294967296 /* 1 << 32 */, 2147483648 /* 1 << 31 */, 1000], - [8589934592 /* 1 << 33 */, 4294967296 /* 1 << 32 */, 1000] -]; - -for (let index = 0, arrayBuffer; index < testCases.length; index += 1) { - const [size, offset, length] = testCases[index]; - - try { - arrayBuffer = new ArrayBuffer(size); - } catch (e) { - if (e instanceof RangeError && acceptableOOMErrors.includes(e.message)) - return common.skip(`Unable to allocate ${size} bytes for ArrayBuffer`); - throw e; - } - - test(arrayBuffer, offset, length); -} diff --git a/test/sequential/test-buffer-creation-regression.js b/test/sequential/test-buffer-creation-regression.js new file mode 100644 index 00000000000000..b5c8450e036cc8 --- /dev/null +++ b/test/sequential/test-buffer-creation-regression.js @@ -0,0 +1,36 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); + +function test(arrayBuffer, offset, length) { + const uint8Array = new Uint8Array(arrayBuffer, offset, length); + for (let i = 0; i < length; i += 1) { + uint8Array[i] = 1; + } + + const buffer = Buffer.from(arrayBuffer, offset, length); + for (let i = 0; i < length; i += 1) { + assert.strictEqual(buffer[i], 1); + } +} + +const acceptableOOMErrors = [ + 'Array buffer allocation failed', + 'Invalid array buffer length' +]; + +const size = 8589934592; /* 1 << 33 */ +const offset = 4294967296; /* 1 << 32 */ +const length = 1000; +let arrayBuffer; + +try { + arrayBuffer = new ArrayBuffer(size); +} catch (e) { + if (e instanceof RangeError && acceptableOOMErrors.includes(e.message)) + return common.skip(`Unable to allocate ${size} bytes for ArrayBuffer`); + throw e; +} + +test(arrayBuffer, offset, length);