buffer: harden validation of buffer allocation size

ZYSzys · addaleax · commit 6fb7baf93544 · 2019-02-21T22:08:53.000+01:00
This makes using `NaN` as the buffer size throw an error. Fixes: nodejs#26151 PR-URL: nodejs#26162 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
diff --git a/lib/buffer.js b/lib/buffer.js
@@ -256,7 +256,7 @@ function assertSize(size) {
 
   if (typeof size !== 'number') {
     err = new ERR_INVALID_ARG_TYPE('size', 'number', size);
-  } else if (size < 0 || size > kMaxLength) {
+  } else if (!(size >= 0 && size <= kMaxLength)) {
     err = new ERR_INVALID_OPT_VALUE.RangeError('size', size);
   }
 
@@ -458,8 +458,11 @@ Buffer.concat = function concat(list, length) {
 
   if (length === undefined) {
     length = 0;
-    for (i = 0; i < list.length; i++)
-      length += list[i].length;
+    for (i = 0; i < list.length; i++) {
+      if (list[i].length) {
+        length += list[i].length;
+      }
+    }
   } else {
     length = length >>> 0;
   }
diff --git a/test/parallel/test-buffer-alloc.js b/test/parallel/test-buffer-alloc.js
@@ -764,7 +764,6 @@ assert.strictEqual(x.inspect(), '<Buffer 81 a3 66 6f 6f a3 62 61 72>');
 Buffer.allocUnsafe(3.3).fill().toString();
 // throws bad argument error in commit 43cb4ec
 Buffer.alloc(3.3).fill().toString();
-assert.strictEqual(Buffer.allocUnsafe(NaN).length, 0);
 assert.strictEqual(Buffer.allocUnsafe(3.3).length, 3);
 assert.strictEqual(Buffer.from({ length: 3.3 }).length, 3);
 assert.strictEqual(Buffer.from({ length: 'BAM' }).length, 0);
diff --git a/test/parallel/test-buffer-no-negative-allocation.js b/test/parallel/test-buffer-no-negative-allocation.js
@@ -7,22 +7,26 @@ const msg = common.expectsError({
   code: 'ERR_INVALID_OPT_VALUE',
   type: RangeError,
   message: /^The value "[^"]*" is invalid for option "size"$/
-}, 12);
+}, 16);
 
 // Test that negative Buffer length inputs throw errors.
 
 assert.throws(() => Buffer(-Buffer.poolSize), msg);
 assert.throws(() => Buffer(-100), msg);
 assert.throws(() => Buffer(-1), msg);
+assert.throws(() => Buffer(NaN), msg);
 
 assert.throws(() => Buffer.alloc(-Buffer.poolSize), msg);
 assert.throws(() => Buffer.alloc(-100), msg);
 assert.throws(() => Buffer.alloc(-1), msg);
+assert.throws(() => Buffer.alloc(NaN), msg);
 
 assert.throws(() => Buffer.allocUnsafe(-Buffer.poolSize), msg);
 assert.throws(() => Buffer.allocUnsafe(-100), msg);
 assert.throws(() => Buffer.allocUnsafe(-1), msg);
+assert.throws(() => Buffer.allocUnsafe(NaN), msg);
 
 assert.throws(() => Buffer.allocUnsafeSlow(-Buffer.poolSize), msg);
 assert.throws(() => Buffer.allocUnsafeSlow(-100), msg);
 assert.throws(() => Buffer.allocUnsafeSlow(-1), msg);
+assert.throws(() => Buffer.allocUnsafeSlow(NaN), msg);
diff --git a/test/parallel/test-buffer-slow.js b/test/parallel/test-buffer-slow.js
@@ -43,29 +43,17 @@ try {
 assert.strictEqual(SlowBuffer('6').length, 6);
 assert.strictEqual(SlowBuffer(true).length, 1);
 
-// Should create zero-length buffer if parameter is not a number
-assert.strictEqual(SlowBuffer().length, 0);
-assert.strictEqual(SlowBuffer(NaN).length, 0);
-assert.strictEqual(SlowBuffer({}).length, 0);
-assert.strictEqual(SlowBuffer('string').length, 0);
-
 // should throw with invalid length
 const bufferMaxSizeMsg = common.expectsError({
   code: 'ERR_INVALID_OPT_VALUE',
   type: RangeError,
   message: /^The value "[^"]*" is invalid for option "size"$/
-}, 2);
-assert.throws(function() {
-  SlowBuffer(Infinity);
-}, bufferMaxSizeMsg);
-common.expectsError(function() {
-  SlowBuffer(-1);
-}, {
-  code: 'ERR_INVALID_OPT_VALUE',
-  type: RangeError,
-  message: 'The value "-1" is invalid for option "size"'
-});
-
-assert.throws(function() {
-  SlowBuffer(buffer.kMaxLength + 1);
-}, bufferMaxSizeMsg);
+}, 7);
+
+assert.throws(() => SlowBuffer(), bufferMaxSizeMsg);
+assert.throws(() => SlowBuffer(NaN), bufferMaxSizeMsg);
+assert.throws(() => SlowBuffer({}), bufferMaxSizeMsg);
+assert.throws(() => SlowBuffer('string'), bufferMaxSizeMsg);
+assert.throws(() => SlowBuffer(Infinity), bufferMaxSizeMsg);
+assert.throws(() => SlowBuffer(-1), bufferMaxSizeMsg);
+assert.throws(() => SlowBuffer(buffer.kMaxLength + 1), bufferMaxSizeMsg);
