From 884eb3536f9054b38df0fc006cd959712f01f349 Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Sun, 27 Nov 2016 15:08:17 +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. 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 --- lib/buffer.js | 4 +-- lib/internal/util.js | 18 ++++++++++ .../test-buffer-creation-regression.js | 33 +++++++++++++++++ test/parallel/test-internal-util-toInteger.js | 32 +++++++++++++++++ test/parallel/test-internal-util-toLength.js | 35 +++++++++++++++++++ 5 files changed, 120 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..ae8b1e0b649486 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 00000000000000..13a662ef20e814 --- /dev/null +++ b/test/parallel/test-buffer-creation-regression.js @@ -0,0 +1,33 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const os = require('os'); + +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 = Buffer.from(arrayBuffer, offset, length); + for (let i = 0; i < length; i += 1) { + assert.strictEqual(buffer[i], 1); + } +} + +const testCases = [ + [200, 50, 100], + [4294967296 /* 1 << 32 */, 2147483648 /* 1 << 31 */, 1000], + [8589934592 /* 1 << 33 */, 4294967296 /* 1 << 32 */, 1000] +]; + +for (let caseIndex = 0; caseIndex < testCases.length; caseIndex += 1) { + const [size, offset, length] = testCases[caseIndex]; + if (os.freemem() < size) { + return common.skip('Not enough memory to run the test'); + } + test(size, 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 9aa3ab5937dc29a78f8d6fbe0c7cc573733a6437 Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Sun, 27 Nov 2016 16:37:28 +0530 Subject: [PATCH 2/3] avoid failing in race condition --- .../test-buffer-creation-regression.js | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/test/parallel/test-buffer-creation-regression.js b/test/parallel/test-buffer-creation-regression.js index 13a662ef20e814..7c51ad50393040 100644 --- a/test/parallel/test-buffer-creation-regression.js +++ b/test/parallel/test-buffer-creation-regression.js @@ -2,11 +2,8 @@ const common = require('../common'); const assert = require('assert'); -const os = require('os'); - -function test(size, offset, length) { - const arrayBuffer = new ArrayBuffer(size); +function test(arrayBuffer, offset, length) { const uint8Array = new Uint8Array(arrayBuffer, offset, length); for (let i = 0; i < length; i += 1) { uint8Array[i] = 1; @@ -24,10 +21,16 @@ const testCases = [ [8589934592 /* 1 << 33 */, 4294967296 /* 1 << 32 */, 1000] ]; -for (let caseIndex = 0; caseIndex < testCases.length; caseIndex += 1) { - const [size, offset, length] = testCases[caseIndex]; - if (os.freemem() < size) { - return common.skip('Not enough memory to run the test'); +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 && e.message === 'Invalid array buffer length') + return common.skip(`Unable to allocate ${size} bytes for ArrayBuffer`); + throw e; } - test(size, offset, length); + + test(arrayBuffer, offset, length); } From 7d4f1c787e4ed6b034a046dfd2a46ba63024f31f Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Sun, 27 Nov 2016 17:00:13 +0530 Subject: [PATCH 3/3] include all expected error messages --- test/parallel/test-buffer-creation-regression.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-buffer-creation-regression.js b/test/parallel/test-buffer-creation-regression.js index 7c51ad50393040..650fbf48f12ae6 100644 --- a/test/parallel/test-buffer-creation-regression.js +++ b/test/parallel/test-buffer-creation-regression.js @@ -15,6 +15,11 @@ function test(arrayBuffer, offset, length) { } } +const acceptableOOMErrors = [ + 'Array buffer allocation failed', + 'Invalid array buffer length' +]; + const testCases = [ [200, 50, 100], [4294967296 /* 1 << 32 */, 2147483648 /* 1 << 31 */, 1000], @@ -27,7 +32,7 @@ for (let index = 0, arrayBuffer; index < testCases.length; index += 1) { try { arrayBuffer = new ArrayBuffer(size); } catch (e) { - if (e instanceof RangeError && e.message === 'Invalid array buffer length') + if (e instanceof RangeError && acceptableOOMErrors.includes(e.message)) return common.skip(`Unable to allocate ${size} bytes for ArrayBuffer`); throw e; }