From 8f90dcc1b8e4ac3d8597ea2ee3927f325cc980d3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 31 May 2016 19:41:30 +0200 Subject: [PATCH] buffer: throw on negative .allocUnsafe() argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/pull/7079 Reviewed-By: Trevor Norris Reviewed-By: Сковорода Никита Андреевич Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Yorkie Liu --- lib/buffer.js | 10 ++++++++-- test/parallel/test-buffer-alloc.js | 19 ++++++++++++++++++- test/parallel/test-buffer.js | 17 ++++++++++++----- 3 files changed, 38 insertions(+), 8 deletions(-) diff --git a/lib/buffer.js b/lib/buffer.js index 21f7d2f21d15a3..e40b81a693c9fd 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -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. diff --git a/test/parallel/test-buffer-alloc.js b/test/parallel/test-buffer-alloc.js index fead5cd807753f..df83fa83010832 100644 --- a/test/parallel/test-buffer-alloc.js +++ b/test/parallel/test-buffer-alloc.js @@ -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); @@ -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/); diff --git a/test/parallel/test-buffer.js b/test/parallel/test-buffer.js index 7f51b690fa5211..3fe1edbde0c585 100644 --- a/test/parallel/test-buffer.js +++ b/test/parallel/test-buffer.js @@ -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); @@ -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')); @@ -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');