Skip to content

Commit

Permalink
buffer: convert offset & length to int properly
Browse files Browse the repository at this point in the history
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 ca37fa5 broke
CI.

Refer: #9814
Refer: #9492

PR-URL: #9815

Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
thefourtheye authored and Fishrock123 committed Dec 5, 2016
1 parent 5a2b688 commit 0c745e3
Show file tree
Hide file tree
Showing 5 changed files with 128 additions and 2 deletions.
4 changes: 2 additions & 2 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ function fromArrayLike(obj) {
}

function fromArrayBuffer(obj, byteOffset, length) {
byteOffset >>>= 0;
byteOffset = internalUtil.toInteger(byteOffset);

const maxLength = obj.byteLength - byteOffset;

Expand All @@ -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");
}
Expand Down
18 changes: 18 additions & 0 deletions lib/internal/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};
41 changes: 41 additions & 0 deletions test/parallel/test-buffer-creation-regression.js
Original file line number Diff line number Diff line change
@@ -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);
}
32 changes: 32 additions & 0 deletions test/parallel/test-internal-util-toInteger.js
Original file line number Diff line number Diff line change
@@ -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);
});
35 changes: 35 additions & 0 deletions test/parallel/test-internal-util-toLength.js
Original file line number Diff line number Diff line change
@@ -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);
});

0 comments on commit 0c745e3

Please sign in to comment.