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: https://github.com/nodejs/node/pull/7079
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>

3 files changed, 38 insertions, 8 deletions

diff --git a/lib/buffer.js b/lib/buffer.js
index 21f7d2f21d..e40b81a693 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 fead5cd807..df83fa8301 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 7f51b690fa..3fe1edbde0 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');