From 6fb7baf935442a1ceddcd0a585892456438f95aa Mon Sep 17 00:00:00 2001 From: ZYSzys <17367077526@163.com> Date: Sun, 17 Feb 2019 21:59:10 +0800 Subject: [PATCH] buffer: harden validation of buffer allocation size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes using `NaN` as the buffer size throw an error. Fixes: https://github.com/nodejs/node/issues/26151 PR-URL: https://github.com/nodejs/node/pull/26162 Reviewed-By: Rich Trott Reviewed-By: Anna Henningsen Reviewed-By: Luigi Pinca Reviewed-By: Сковорода Никита Андреевич Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater --- lib/buffer.js | 9 ++++-- test/parallel/test-buffer-alloc.js | 1 - .../test-buffer-no-negative-allocation.js | 6 +++- test/parallel/test-buffer-slow.js | 30 ++++++------------- 4 files changed, 20 insertions(+), 26 deletions(-) diff --git a/lib/buffer.js b/lib/buffer.js index 995f5096239a65..8b7aba1aaf06df 100644 --- 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 index 8257a221c98e4a..2b8e9c5f2250e9 100644 --- a/test/parallel/test-buffer-alloc.js +++ b/test/parallel/test-buffer-alloc.js @@ -764,7 +764,6 @@ assert.strictEqual(x.inspect(), ''); 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 index b34477aa8c4b45..3a4958c8f62c60 100644 --- 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 index cfea22b02cfd12..99362add3bb336 100644 --- 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);