Skip to content

Commit

Permalink
buffer: throw on negative .allocUnsafe() argument
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
addaleax committed Aug 8, 2016
1 parent be73480 commit be32633
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 8 deletions.
10 changes: 8 additions & 2 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,14 @@ Buffer.from = function(value, encodingOrOffset, length) {
Object.setPrototypeOf(Buffer, Uint8Array);

function assertSize(size) {
if (typeof size !== 'number') {
const err = new TypeError('"size" argument must be a number');
let err = null;

if (typeof size !== 'number')
err = new TypeError('"size" argument must be a number');
else if (size < 0)
err = new RangeError('"size" argument must not be negative');

if (err) {
// The following hides the 'assertSize' method from the
// callstack. This is done simply to hide the internal
// details of the implementation from bleeding out to users.
Expand Down
19 changes: 18 additions & 1 deletion test/parallel/test-buffer-alloc.js
Original file line number Diff line number Diff line change
Expand Up @@ -981,7 +981,6 @@ assert.equal(0, Buffer.from('hello').slice(0, 0).length);
Buffer.allocUnsafe(3.3).fill().toString();
// throws bad argument error in commit 43cb4ec
Buffer.alloc(3.3).fill().toString();
assert.equal(Buffer.allocUnsafe(-1).length, 0);
assert.equal(Buffer.allocUnsafe(NaN).length, 0);
assert.equal(Buffer.allocUnsafe(3.3).length, 3);
assert.equal(Buffer.from({length: 3.3}).length, 3);
Expand Down Expand Up @@ -1479,3 +1478,21 @@ assert.equal(ubuf.buffer.byteLength, 10);
assert.doesNotThrow(() => {
Buffer.from(new ArrayBuffer());
});

assert.throws(() => Buffer.alloc(-Buffer.poolSize),
'"size" argument must not be negative');
assert.throws(() => Buffer.alloc(-100),
'"size" argument must not be negative');
assert.throws(() => Buffer.allocUnsafe(-Buffer.poolSize),
'"size" argument must not be negative');
assert.throws(() => Buffer.allocUnsafe(-100),
'"size" argument must not be negative');
assert.throws(() => Buffer.allocUnsafeSlow(-Buffer.poolSize),
'"size" argument must not be negative');
assert.throws(() => Buffer.allocUnsafeSlow(-100),
'"size" argument must not be negative');

assert.throws(() => Buffer.alloc({ valueOf: () => 1 }),
/"size" argument must be a number/);
assert.throws(() => Buffer.alloc({ valueOf: () => -1 }),
/"size" argument must be a number/);
17 changes: 12 additions & 5 deletions test/parallel/test-buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1005,7 +1005,6 @@ assert.equal(0, Buffer('hello').slice(0, 0).length);

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

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

// Check pool offset after that by trying to write string into the pool.
assert.doesNotThrow(() => Buffer.from('abc'));
Expand Down Expand Up @@ -1522,3 +1521,11 @@ assert.equal(SlowBuffer.prototype.offset, undefined);
}
}
}

// Test that large negative Buffer length inputs throw errors.
assert.throws(() => Buffer(-Buffer.poolSize),
'"size" argument must not be negative');
assert.throws(() => Buffer(-100),
'"size" argument must not be negative');
assert.throws(() => Buffer(-1),
'"size" argument must not be negative');

0 comments on commit be32633

Please sign in to comment.