From f558b00286980cbade3de63c0c40781a8b617434 Mon Sep 17 00:00:00 2001 From: Sebastian Van Sande Date: Fri, 3 Feb 2017 16:05:28 +0100 Subject: [PATCH 1/3] buffer: check byteLength in readIntBE() and readIntLE() The 'byteLength' argument should be required and of type 'number'. It should have a value between 1 and 6. This is a fix for issue: https://github.com/nodejs/node/issues/10515 --- benchmark/buffers/buffer-read.js | 22 ++++++++++++++++------ doc/api/buffer.md | 5 ++++- lib/buffer.js | 30 ++++++++++++++++++++++++------ test/parallel/test-buffer-read.js | 18 +++++++++++++++++- 4 files changed, 61 insertions(+), 14 deletions(-) diff --git a/benchmark/buffers/buffer-read.js b/benchmark/buffers/buffer-read.js index 339da75befce4d..22f8dcbd3dea12 100644 --- a/benchmark/buffers/buffer-read.js +++ b/benchmark/buffers/buffer-read.js @@ -15,7 +15,9 @@ const types = [ 'FloatLE', 'FloatBE', 'DoubleLE', - 'DoubleBE' + 'DoubleBE', + 'IntLE', + 'IntBE', ]; const bench = common.createBenchmark(main, { @@ -34,11 +36,19 @@ function main(conf) { const fn = `read${type}`; buff.writeDoubleLE(0, 0, noAssert); - const testFunction = new Function('buff', ` - for (var i = 0; i !== ${len}; i++) { - buff.${fn}(0, ${JSON.stringify(noAssert)}); - } - `); + + var call; + if (fn === 'readIntLE' || fn === 'readIntBE') { + call = `buff.${fn}(0, 1, ${JSON.stringify(noAssert)})`; + } else { + call = `buff.${fn}(0, ${JSON.stringify(noAssert)})`; + } + + const testFunction = new Function( + 'buff', + `for (var i = 0; i !== ${len}; ++i) { ${call}; }` + ); + bench.start(); testFunction(buff); bench.end(len / 1e6); diff --git a/doc/api/buffer.md b/doc/api/buffer.md index 07404a043bcc40..5158209ec55aff 100644 --- a/doc/api/buffer.md +++ b/doc/api/buffer.md @@ -1776,8 +1776,11 @@ console.log(buf.readIntLE(0, 6).toString(16)); // Prints: 1234567890ab console.log(buf.readIntBE(0, 6).toString(16)); -// Throws an exception: RangeError: Index out of range +// Throws ERR_INDEX_OUT_OF_RANGE: console.log(buf.readIntBE(1, 6).toString(16)); + +// Throws ERR_OUT_OF_RANGE: +console.log(buf.readIntBE(1, 0).toString(16)); ``` ### buf.readUInt8(offset[, noAssert]) diff --git a/lib/buffer.js b/lib/buffer.js index 180bfe6badaf97..cd08453c243561 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -200,10 +200,11 @@ Buffer.from = function from(value, encodingOrOffset, length) { ); } - if (typeof value === 'number') + if (typeof value === 'number') { throw new errors.TypeError( 'ERR_INVALID_ARG_TYPE', 'value', 'not number', value ); + } const valueOf = value.valueOf && value.valueOf(); if (valueOf !== null && valueOf !== undefined && valueOf !== value) @@ -447,10 +448,11 @@ Buffer[kIsEncodingSymbol] = Buffer.isEncoding; Buffer.concat = function concat(list, length) { var i; - if (!Array.isArray(list)) + if (!Array.isArray(list)) { throw new errors.TypeError( 'ERR_INVALID_ARG_TYPE', 'list', ['Array', 'Buffer', 'Uint8Array'] ); + } if (list.length === 0) return new FastBuffer(); @@ -467,10 +469,11 @@ Buffer.concat = function concat(list, length) { var pos = 0; for (i = 0; i < list.length; i++) { var buf = list[i]; - if (!isUint8Array(buf)) + if (!isUint8Array(buf)) { throw new errors.TypeError( 'ERR_INVALID_ARG_TYPE', 'list', ['Array', 'Buffer', 'Uint8Array'] ); + } _copy(buf, buffer, pos); pos += buf.length; } @@ -1024,6 +1027,14 @@ function checkOffset(offset, ext, length) { throw new errors.RangeError('ERR_INDEX_OUT_OF_RANGE'); } +function checkByteLength(byteLength) { + if (byteLength < 1 || byteLength > 6) { + throw new errors.RangeError('ERR_OUT_OF_RANGE', + 'byteLength', + '>= 1 and <= 6'); + } +} + Buffer.prototype.readUIntLE = function readUIntLE(offset, byteLength, noAssert) { @@ -1109,8 +1120,11 @@ Buffer.prototype.readUInt32BE = function readUInt32BE(offset, noAssert) { Buffer.prototype.readIntLE = function readIntLE(offset, byteLength, noAssert) { offset = offset >>> 0; byteLength = byteLength >>> 0; - if (!noAssert) + + if (!noAssert) { + checkByteLength(byteLength); checkOffset(offset, byteLength, this.length); + } var val = this[offset]; var mul = 1; @@ -1129,8 +1143,11 @@ Buffer.prototype.readIntLE = function readIntLE(offset, byteLength, noAssert) { Buffer.prototype.readIntBE = function readIntBE(offset, byteLength, noAssert) { offset = offset >>> 0; byteLength = byteLength >>> 0; - if (!noAssert) + + if (!noAssert) { + checkByteLength(byteLength); checkOffset(offset, byteLength, this.length); + } var i = byteLength; var mul = 1; @@ -1612,9 +1629,10 @@ if (process.binding('config').hasIntl) { // Transcodes the Buffer from one encoding to another, returning a new // Buffer instance. transcode = function transcode(source, fromEncoding, toEncoding) { - if (!isUint8Array(source)) + if (!isUint8Array(source)) { throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'source', ['Buffer', 'Uint8Array'], source); + } if (source.length === 0) return Buffer.alloc(0); fromEncoding = normalizeEncoding(fromEncoding) || fromEncoding; diff --git a/test/parallel/test-buffer-read.js b/test/parallel/test-buffer-read.js index 88f97db3097b6e..c5b3373cbf23cb 100644 --- a/test/parallel/test-buffer-read.js +++ b/test/parallel/test-buffer-read.js @@ -9,7 +9,7 @@ function read(buff, funx, args, expected) { assert.strictEqual(buff[funx](...args), expected); common.expectsError( - () => buff[funx](-1), + () => buff[funx](-1, args[1]), { code: 'ERR_INDEX_OUT_OF_RANGE' } @@ -142,3 +142,19 @@ assert.throws(() => Buffer.allocUnsafe(8).readFloatLE(-1), RangeError); assert.strictEqual(buf.readIntLE(0, 6), 0x060504030201); assert.strictEqual(buf.readIntBE(0, 6), 0x010203040506); } + +// test for byteLength parameter not between 1 and 6 (inclusive) +common.expectsError(() => { buf.readIntLE(1); }, { code: 'ERR_OUT_OF_RANGE' }); +common.expectsError(() => { buf.readIntLE(1, 'string'); }, + { code: 'ERR_OUT_OF_RANGE' }); +common.expectsError(() => { buf.readIntLE(1, 0); }, + { code: 'ERR_OUT_OF_RANGE' }); +common.expectsError(() => { buf.readIntLE(1, 7); }, + { code: 'ERR_OUT_OF_RANGE' }); +common.expectsError(() => { buf.readIntBE(1); }, { code: 'ERR_OUT_OF_RANGE' }); +common.expectsError(() => { buf.readIntBE(1, 'string'); }, + { code: 'ERR_OUT_OF_RANGE' }); +common.expectsError(() => { buf.readIntBE(1, 0); }, + { code: 'ERR_OUT_OF_RANGE' }); +common.expectsError(() => { buf.readIntBE(1, 7); }, + { code: 'ERR_OUT_OF_RANGE' }); From bac64baa749b64dcd658b609b503f1ef5d64a0b5 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 1 Jan 2018 20:00:59 -0800 Subject: [PATCH 2/3] benchmark: improve buffer.readIntLE()/readIntBE() benchmarks Split them into their own benhmark file and use different byteLength values. --- .../buffers/buffer-read-with-byteLength.js | 34 +++++++++++++++++++ benchmark/buffers/buffer-read.js | 22 ++++-------- test/sequential/test-benchmark-buffer.js | 3 ++ 3 files changed, 43 insertions(+), 16 deletions(-) create mode 100644 benchmark/buffers/buffer-read-with-byteLength.js diff --git a/benchmark/buffers/buffer-read-with-byteLength.js b/benchmark/buffers/buffer-read-with-byteLength.js new file mode 100644 index 00000000000000..013947da9dd485 --- /dev/null +++ b/benchmark/buffers/buffer-read-with-byteLength.js @@ -0,0 +1,34 @@ +'use strict'; +const common = require('../common.js'); + +const types = [ + 'IntLE', + 'IntBE', +]; + +const bench = common.createBenchmark(main, { + noAssert: ['false', 'true'], + buffer: ['fast', 'slow'], + type: types, + millions: [1], + byteLength: [1, 2, 4, 6] +}); + +function main(conf) { + const noAssert = conf.noAssert === 'true'; + const len = conf.millions * 1e6; + const clazz = conf.buf === 'fast' ? Buffer : require('buffer').SlowBuffer; + const buff = new clazz(8); + const type = conf.type || 'UInt8'; + const fn = `read${type}`; + + buff.writeDoubleLE(0, 0, noAssert); + const testFunction = new Function('buff', ` + for (var i = 0; i !== ${len}; i++) { + buff.${fn}(0, ${conf.byteLength}, ${JSON.stringify(noAssert)}); + } + `); + bench.start(); + testFunction(buff); + bench.end(len / 1e6); +} diff --git a/benchmark/buffers/buffer-read.js b/benchmark/buffers/buffer-read.js index 22f8dcbd3dea12..339da75befce4d 100644 --- a/benchmark/buffers/buffer-read.js +++ b/benchmark/buffers/buffer-read.js @@ -15,9 +15,7 @@ const types = [ 'FloatLE', 'FloatBE', 'DoubleLE', - 'DoubleBE', - 'IntLE', - 'IntBE', + 'DoubleBE' ]; const bench = common.createBenchmark(main, { @@ -36,19 +34,11 @@ function main(conf) { const fn = `read${type}`; buff.writeDoubleLE(0, 0, noAssert); - - var call; - if (fn === 'readIntLE' || fn === 'readIntBE') { - call = `buff.${fn}(0, 1, ${JSON.stringify(noAssert)})`; - } else { - call = `buff.${fn}(0, ${JSON.stringify(noAssert)})`; - } - - const testFunction = new Function( - 'buff', - `for (var i = 0; i !== ${len}; ++i) { ${call}; }` - ); - + const testFunction = new Function('buff', ` + for (var i = 0; i !== ${len}; i++) { + buff.${fn}(0, ${JSON.stringify(noAssert)}); + } + `); bench.start(); testFunction(buff); bench.end(len / 1e6); diff --git a/test/sequential/test-benchmark-buffer.js b/test/sequential/test-benchmark-buffer.js index 26600cf4f26a87..2cee628ada8d79 100644 --- a/test/sequential/test-benchmark-buffer.js +++ b/test/sequential/test-benchmark-buffer.js @@ -9,7 +9,9 @@ runBenchmark('buffers', 'aligned=true', 'args=1', 'buffer=fast', + 'byteLength=1', 'encoding=utf8', + 'endian=BE', 'len=2', 'method=', 'n=1', @@ -20,6 +22,7 @@ runBenchmark('buffers', 'size=1', 'source=array', 'type=', + 'value=0', 'withTotalLength=0' ], { NODEJS_BENCHMARK_ZERO_ALLOWED: 1 }); From 710d1dc3546a0961fcb961c06d6d00a8c1c97d22 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 2 Jan 2018 16:10:57 -0800 Subject: [PATCH 3/3] buffer: check byteLength in readIntBE() and readIntLE() --- benchmark/buffers/buffer-read-with-byteLength.js | 4 +++- lib/buffer.js | 8 ++++++-- test/parallel/test-buffer-read.js | 10 ++++++++-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/benchmark/buffers/buffer-read-with-byteLength.js b/benchmark/buffers/buffer-read-with-byteLength.js index 013947da9dd485..2a659c1bec5e19 100644 --- a/benchmark/buffers/buffer-read-with-byteLength.js +++ b/benchmark/buffers/buffer-read-with-byteLength.js @@ -2,8 +2,10 @@ const common = require('../common.js'); const types = [ - 'IntLE', 'IntBE', + 'IntLE', + 'UIntBE', + 'UIntLE' ]; const bench = common.createBenchmark(main, { diff --git a/lib/buffer.js b/lib/buffer.js index cd08453c243561..03f0cb3377ea96 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -1040,8 +1040,10 @@ Buffer.prototype.readUIntLE = function readUIntLE(offset, byteLength, noAssert) { offset = offset >>> 0; byteLength = byteLength >>> 0; - if (!noAssert) + if (!noAssert) { + checkByteLength(byteLength); checkOffset(offset, byteLength, this.length); + } var val = this[offset]; var mul = 1; @@ -1057,8 +1059,10 @@ Buffer.prototype.readUIntBE = function readUIntBE(offset, byteLength, noAssert) { offset = offset >>> 0; byteLength = byteLength >>> 0; - if (!noAssert) + if (!noAssert) { + checkByteLength(byteLength); checkOffset(offset, byteLength, this.length); + } var val = this[offset + --byteLength]; var mul = 1; diff --git a/test/parallel/test-buffer-read.js b/test/parallel/test-buffer-read.js index c5b3373cbf23cb..d024a3280333d8 100644 --- a/test/parallel/test-buffer-read.js +++ b/test/parallel/test-buffer-read.js @@ -57,8 +57,14 @@ read(buf, 'readUInt32BE', [1], 0xfd48eacf); read(buf, 'readUInt32LE', [1], 0xcfea48fd); // testing basic functionality of readUIntBE() and readUIntLE() -read(buf, 'readUIntBE', [2, 0], 0xfd); -read(buf, 'readUIntLE', [2, 0], 0x48); +read(buf, 'readUIntBE', [2, 2], 0x48ea); +read(buf, 'readUIntLE', [2, 2], 0xea48); + +// invalid byteLength parameter for readUIntBE() and readUIntLE() +common.expectsError(() => { buf.readUIntBE(2, 0); }, + { code: 'ERR_OUT_OF_RANGE' }); +common.expectsError(() => { buf.readUIntLE(2, 7); }, + { code: 'ERR_OUT_OF_RANGE' }); // attempt to overflow buffers, similar to previous bug in array buffers assert.throws(() => Buffer.allocUnsafe(8).readFloatBE(0xffffffff),