Skip to content

Commit 8f90dcc

Browse files
committed
buffer: throw on negative .allocUnsafe() argument
Add a check for `size < 0` to `assertSize()`, as passing a negative value almost certainly indicates a programming error. This also lines up the behaviour of `.allocUnsafe()` with the ones of `.alloc()` and `.allocUnsafeSlow()` (which previously threw errors from the Uint8Array constructor). Notably, this also affects `Buffer()` calls with negative arguments. PR-URL: #7079 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yorkie Liu <[email protected]>
1 parent be73480 commit 8f90dcc

File tree

3 files changed

+38
-8
lines changed

3 files changed

+38
-8
lines changed

lib/buffer.js

+8-2
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,14 @@ Buffer.from = function(value, encodingOrOffset, length) {
101101
Object.setPrototypeOf(Buffer, Uint8Array);
102102

103103
function assertSize(size) {
104-
if (typeof size !== 'number') {
105-
const err = new TypeError('"size" argument must be a number');
104+
let err = null;
105+
106+
if (typeof size !== 'number')
107+
err = new TypeError('"size" argument must be a number');
108+
else if (size < 0)
109+
err = new RangeError('"size" argument must not be negative');
110+
111+
if (err) {
106112
// The following hides the 'assertSize' method from the
107113
// callstack. This is done simply to hide the internal
108114
// details of the implementation from bleeding out to users.

test/parallel/test-buffer-alloc.js

+18-1
Original file line numberDiff line numberDiff line change
@@ -981,7 +981,6 @@ assert.equal(0, Buffer.from('hello').slice(0, 0).length);
981981
Buffer.allocUnsafe(3.3).fill().toString();
982982
// throws bad argument error in commit 43cb4ec
983983
Buffer.alloc(3.3).fill().toString();
984-
assert.equal(Buffer.allocUnsafe(-1).length, 0);
985984
assert.equal(Buffer.allocUnsafe(NaN).length, 0);
986985
assert.equal(Buffer.allocUnsafe(3.3).length, 3);
987986
assert.equal(Buffer.from({length: 3.3}).length, 3);
@@ -1479,3 +1478,21 @@ assert.equal(ubuf.buffer.byteLength, 10);
14791478
assert.doesNotThrow(() => {
14801479
Buffer.from(new ArrayBuffer());
14811480
});
1481+
1482+
assert.throws(() => Buffer.alloc(-Buffer.poolSize),
1483+
'"size" argument must not be negative');
1484+
assert.throws(() => Buffer.alloc(-100),
1485+
'"size" argument must not be negative');
1486+
assert.throws(() => Buffer.allocUnsafe(-Buffer.poolSize),
1487+
'"size" argument must not be negative');
1488+
assert.throws(() => Buffer.allocUnsafe(-100),
1489+
'"size" argument must not be negative');
1490+
assert.throws(() => Buffer.allocUnsafeSlow(-Buffer.poolSize),
1491+
'"size" argument must not be negative');
1492+
assert.throws(() => Buffer.allocUnsafeSlow(-100),
1493+
'"size" argument must not be negative');
1494+
1495+
assert.throws(() => Buffer.alloc({ valueOf: () => 1 }),
1496+
/"size" argument must be a number/);
1497+
assert.throws(() => Buffer.alloc({ valueOf: () => -1 }),
1498+
/"size" argument must be a number/);

test/parallel/test-buffer.js

+12-5
Original file line numberDiff line numberDiff line change
@@ -1005,7 +1005,6 @@ assert.equal(0, Buffer('hello').slice(0, 0).length);
10051005

10061006
// Call .fill() first, stops valgrind warning about uninitialized memory reads.
10071007
Buffer(3.3).fill().toString(); // throws bad argument error in commit 43cb4ec
1008-
assert.equal(Buffer(-1).length, 0);
10091008
assert.equal(Buffer(NaN).length, 0);
10101009
assert.equal(Buffer(3.3).length, 3);
10111010
assert.equal(Buffer({length: 3.3}).length, 3);
@@ -1491,10 +1490,10 @@ assert.equal(SlowBuffer.prototype.offset, undefined);
14911490

14921491
{
14931492
// Test that large negative Buffer length inputs don't affect the pool offset.
1494-
assert.deepStrictEqual(Buffer(-Buffer.poolSize), Buffer.from(''));
1495-
assert.deepStrictEqual(Buffer(-100), Buffer.from(''));
1496-
assert.deepStrictEqual(Buffer.allocUnsafe(-Buffer.poolSize), Buffer.from(''));
1497-
assert.deepStrictEqual(Buffer.allocUnsafe(-100), Buffer.from(''));
1493+
// Use the fromArrayLike() variant here because it's more lenient
1494+
// about its input and passes the length directly to allocate().
1495+
assert.deepStrictEqual(Buffer({ length: -Buffer.poolSize }), Buffer.from(''));
1496+
assert.deepStrictEqual(Buffer({ length: -100 }), Buffer.from(''));
14981497

14991498
// Check pool offset after that by trying to write string into the pool.
15001499
assert.doesNotThrow(() => Buffer.from('abc'));
@@ -1522,3 +1521,11 @@ assert.equal(SlowBuffer.prototype.offset, undefined);
15221521
}
15231522
}
15241523
}
1524+
1525+
// Test that large negative Buffer length inputs throw errors.
1526+
assert.throws(() => Buffer(-Buffer.poolSize),
1527+
'"size" argument must not be negative');
1528+
assert.throws(() => Buffer(-100),
1529+
'"size" argument must not be negative');
1530+
assert.throws(() => Buffer(-1),
1531+
'"size" argument must not be negative');

0 commit comments

Comments
 (0)