From 720d01f00668f25cdff2fd1270bdd1c7fb35b9f5 Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Sun, 27 Nov 2016 15:08:17 +0530 Subject: [PATCH] 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 f4d16f41fc..3453f5fe8c 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -228,7 +228,7 @@ function fromArrayLike(obj) { } function fromArrayBuffer(obj, byteOffset, length) { - byteOffset >>>= 0; + byteOffset = internalUtil.toInteger(byteOffset); const maxLength = obj.byteLength - byteOffset; @@ -238,7 +238,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 4ada8dd0cc..ae8b1e0b64 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -161,3 +161,21 @@ 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; + 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 0000000000..650fbf48f1 --- /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 0000000000..57a411964d --- /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 0000000000..ce594c47c1 --- /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); +});