From eeeb0bfdecd44f69054d4ac734de4b1b84c558a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Tue, 9 Jan 2018 20:39:29 +0100 Subject: [PATCH 1/7] stream: add type and range check for highWaterMark The (h|readableH|writableH)ighWaterMark options should only permit positive numbers and zero. --- lib/_stream_readable.js | 15 ++------------- lib/_stream_writable.js | 15 ++------------- lib/internal/streams/state.js | 35 +++++++++++++++++++++++++++++++++++ node.gyp | 1 + 4 files changed, 40 insertions(+), 26 deletions(-) create mode 100644 lib/internal/streams/state.js diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index 21598efa65f254..a5eac9faccd217 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -31,6 +31,7 @@ const util = require('util'); const debug = util.debuglog('stream'); const BufferList = require('internal/streams/BufferList'); const destroyImpl = require('internal/streams/destroy'); +const { getHighWaterMark } = require('internal/streams/state'); const errors = require('internal/errors'); var StringDecoder; @@ -75,19 +76,7 @@ function ReadableState(options, stream) { // the point at which it stops calling _read() to fill the buffer // Note: 0 is a valid value, means "don't call _read preemptively ever" - var hwm = options.highWaterMark; - var readableHwm = options.readableHighWaterMark; - var defaultHwm = this.objectMode ? 16 : 16 * 1024; - - if (hwm || hwm === 0) - this.highWaterMark = hwm; - else if (isDuplex && (readableHwm || readableHwm === 0)) - this.highWaterMark = readableHwm; - else - this.highWaterMark = defaultHwm; - - // cast to ints. - this.highWaterMark = Math.floor(this.highWaterMark); + this.highWaterMark = getHighWaterMark(this, options, isDuplex); // A linked list is used to store data chunks instead of an array because the // linked list can remove elements from the beginning faster than diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index 549bff1599a911..58e88f0d46e177 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -33,6 +33,7 @@ const internalUtil = require('internal/util'); const Stream = require('stream'); const { Buffer } = require('buffer'); const destroyImpl = require('internal/streams/destroy'); +const { getHighWaterMark } = require('internal/streams/state'); const errors = require('internal/errors'); util.inherits(Writable, Stream); @@ -59,19 +60,7 @@ function WritableState(options, stream) { // the point at which write() starts returning false // Note: 0 is a valid value, means that we always return false if // the entire buffer is not flushed immediately on write() - var hwm = options.highWaterMark; - var writableHwm = options.writableHighWaterMark; - var defaultHwm = this.objectMode ? 16 : 16 * 1024; - - if (hwm || hwm === 0) - this.highWaterMark = hwm; - else if (isDuplex && (writableHwm || writableHwm === 0)) - this.highWaterMark = writableHwm; - else - this.highWaterMark = defaultHwm; - - // cast to ints. - this.highWaterMark = Math.floor(this.highWaterMark); + this.highWaterMark = getHighWaterMark(this, options, isDuplex); // if _final has been called this.finalCalled = false; diff --git a/lib/internal/streams/state.js b/lib/internal/streams/state.js new file mode 100644 index 00000000000000..7bf93a935d4acf --- /dev/null +++ b/lib/internal/streams/state.js @@ -0,0 +1,35 @@ +'use strict'; + +const stream = require('stream'); +const errors = require('internal/errors'); + +function getHighWaterMarkFromProperty(hwm, name) { + if (typeof hwm !== 'number' || hwm < 0) + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', name, hwm); + return Math.floor(hwm); +} + +function getHighWaterMark(state, options, isDuplex) { + if (options.highWaterMark != null && !Number.isNaN(options.highWaterMark)) { + return getHighWaterMarkFromProperty(options.highWaterMark, 'highWaterMark'); + } else if (isDuplex) { + if (state instanceof stream.Readable.ReadableState) { + if (options.readableHighWaterMark != null && + !Number.isNaN(options.readableHighWaterMark)) { + return getHighWaterMarkFromProperty(options.readableHighWaterMark, + 'readableHighWaterMark'); + } + } else if (options.writableHighWaterMark != null && + !Number.isNaN(options.writableHighWaterMark)) { + return getHighWaterMarkFromProperty(options.writableHighWaterMark, + 'writableHighWaterMark'); + } + } + + // Default value + return state.objectMode ? 16 : 16 * 1024; +} + +module.exports = { + getHighWaterMark +}; diff --git a/node.gyp b/node.gyp index 04d6eff57395b9..ec70fa64fd3ec5 100644 --- a/node.gyp +++ b/node.gyp @@ -141,6 +141,7 @@ 'lib/internal/streams/BufferList.js', 'lib/internal/streams/legacy.js', 'lib/internal/streams/destroy.js', + 'lib/internal/streams/state.js', 'lib/internal/wrap_js_stream.js', 'deps/v8/tools/splaytree.js', 'deps/v8/tools/codemap.js', From 9dd4390a8f1eb48d7ca78d9d1dc364f87e96f9d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Tue, 9 Jan 2018 20:41:15 +0100 Subject: [PATCH 2/7] test: add test for invalid highWaterMark values --- test/parallel/test-streams-highwatermark.js | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-streams-highwatermark.js b/test/parallel/test-streams-highwatermark.js index aca2415bd8e15f..7dc88da208ccc5 100644 --- a/test/parallel/test-streams-highwatermark.js +++ b/test/parallel/test-streams-highwatermark.js @@ -1,8 +1,9 @@ 'use strict'; -require('../common'); +const common = require('../common'); // This test ensures that the stream implementation correctly handles values -// for highWaterMark which exceed the range of signed 32 bit integers. +// for highWaterMark which exceed the range of signed 32 bit integers and +// rejects invalid values. const assert = require('assert'); const stream = require('stream'); @@ -16,3 +17,15 @@ assert.strictEqual(readable._readableState.highWaterMark, ovfl); const writable = stream.Writable({ highWaterMark: ovfl }); assert.strictEqual(writable._writableState.highWaterMark, ovfl); + +for (const invalidHwm of [true, false, '5', {}, -5]) { + for (const type of [stream.Readable, stream.Writable]) { + common.expectsError(() => { + type({ highWaterMark: invalidHwm }); + }, { + type: TypeError, + code: 'ERR_INVALID_OPT_VALUE', + message: `The value "${invalidHwm}" is invalid for option "highWaterMark"` + }); + } +} From 854c9e73105439711e01744c9863e3276f48b325 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Thu, 11 Jan 2018 13:44:37 +0100 Subject: [PATCH 3/7] fixup! stream: add type and range check for highWaterMark --- lib/internal/streams/state.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/internal/streams/state.js b/lib/internal/streams/state.js index 7bf93a935d4acf..33a6889632ff8e 100644 --- a/lib/internal/streams/state.js +++ b/lib/internal/streams/state.js @@ -4,23 +4,21 @@ const stream = require('stream'); const errors = require('internal/errors'); function getHighWaterMarkFromProperty(hwm, name) { - if (typeof hwm !== 'number' || hwm < 0) + if (typeof hwm !== 'number' || !(hwm >= 0)) throw new errors.TypeError('ERR_INVALID_OPT_VALUE', name, hwm); return Math.floor(hwm); } function getHighWaterMark(state, options, isDuplex) { - if (options.highWaterMark != null && !Number.isNaN(options.highWaterMark)) { + if (options.highWaterMark != null) { return getHighWaterMarkFromProperty(options.highWaterMark, 'highWaterMark'); } else if (isDuplex) { if (state instanceof stream.Readable.ReadableState) { - if (options.readableHighWaterMark != null && - !Number.isNaN(options.readableHighWaterMark)) { + if (options.readableHighWaterMark != null) { return getHighWaterMarkFromProperty(options.readableHighWaterMark, 'readableHighWaterMark'); } - } else if (options.writableHighWaterMark != null && - !Number.isNaN(options.writableHighWaterMark)) { + } else if (options.writableHighWaterMark != null) { return getHighWaterMarkFromProperty(options.writableHighWaterMark, 'writableHighWaterMark'); } From 72cbad6347fd01544428eeef70f142bfa5c84ca3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Thu, 11 Jan 2018 13:44:59 +0100 Subject: [PATCH 4/7] fixup! test: add test for invalid highWaterMark values --- ...st-stream-transform-split-highwatermark.js | 25 ++++++++++++++++--- test/parallel/test-streams-highwatermark.js | 2 +- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-stream-transform-split-highwatermark.js b/test/parallel/test-stream-transform-split-highwatermark.js index af2558ec6decfb..f931d4f6ceb928 100644 --- a/test/parallel/test-stream-transform-split-highwatermark.js +++ b/test/parallel/test-stream-transform-split-highwatermark.js @@ -1,5 +1,5 @@ 'use strict'; -require('../common'); +const common = require('../common'); const assert = require('assert'); const { Transform, Readable, Writable } = require('stream'); @@ -54,14 +54,33 @@ testTransform(0, 0, { writableHighWaterMark: 777, }); -// test undefined, null, NaN -[undefined, null, NaN].forEach((v) => { +// test undefined, null +[undefined, null].forEach((v) => { testTransform(DEFAULT, DEFAULT, { readableHighWaterMark: v }); testTransform(DEFAULT, DEFAULT, { writableHighWaterMark: v }); testTransform(666, DEFAULT, { highWaterMark: v, readableHighWaterMark: 666 }); testTransform(DEFAULT, 777, { highWaterMark: v, writableHighWaterMark: 777 }); }); +// test NaN +{ + common.expectsError(() => { + new Transform({ readableHighWaterMark: NaN }); + }, { + type: TypeError, + code: 'ERR_INVALID_OPT_VALUE', + message: 'The value "NaN" is invalid for option "readableHighWaterMark"' + }); + + common.expectsError(() => { + new Transform({ writableHighWaterMark: NaN }); + }, { + type: TypeError, + code: 'ERR_INVALID_OPT_VALUE', + message: 'The value "NaN" is invalid for option "writableHighWaterMark"' + }); +} + // test non Duplex streams ignore the options { const r = new Readable({ readableHighWaterMark: 666 }); diff --git a/test/parallel/test-streams-highwatermark.js b/test/parallel/test-streams-highwatermark.js index 7dc88da208ccc5..377fe08e90d3f8 100644 --- a/test/parallel/test-streams-highwatermark.js +++ b/test/parallel/test-streams-highwatermark.js @@ -18,7 +18,7 @@ assert.strictEqual(readable._readableState.highWaterMark, ovfl); const writable = stream.Writable({ highWaterMark: ovfl }); assert.strictEqual(writable._writableState.highWaterMark, ovfl); -for (const invalidHwm of [true, false, '5', {}, -5]) { +for (const invalidHwm of [true, false, '5', {}, -5, NaN]) { for (const type of [stream.Readable, stream.Writable]) { common.expectsError(() => { type({ highWaterMark: invalidHwm }); From dfa34e5f923e554038e2b5f041e137acdc179653 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Thu, 11 Jan 2018 14:09:00 +0100 Subject: [PATCH 5/7] fixup! stream: add type and range check for highWaterMark --- lib/_stream_readable.js | 3 ++- lib/_stream_writable.js | 3 ++- lib/internal/streams/state.js | 14 +++----------- 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index a5eac9faccd217..62a6ffde7c90b7 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -76,7 +76,8 @@ function ReadableState(options, stream) { // the point at which it stops calling _read() to fill the buffer // Note: 0 is a valid value, means "don't call _read preemptively ever" - this.highWaterMark = getHighWaterMark(this, options, isDuplex); + this.highWaterMark = getHighWaterMark(this, options, 'readableHighWaterMark', + isDuplex); // A linked list is used to store data chunks instead of an array because the // linked list can remove elements from the beginning faster than diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index 58e88f0d46e177..de96a902552fe7 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -60,7 +60,8 @@ function WritableState(options, stream) { // the point at which write() starts returning false // Note: 0 is a valid value, means that we always return false if // the entire buffer is not flushed immediately on write() - this.highWaterMark = getHighWaterMark(this, options, isDuplex); + this.highWaterMark = getHighWaterMark(this, options, 'writableHighWaterMark', + isDuplex); // if _final has been called this.finalCalled = false; diff --git a/lib/internal/streams/state.js b/lib/internal/streams/state.js index 33a6889632ff8e..013913fe593ef1 100644 --- a/lib/internal/streams/state.js +++ b/lib/internal/streams/state.js @@ -9,19 +9,11 @@ function getHighWaterMarkFromProperty(hwm, name) { return Math.floor(hwm); } -function getHighWaterMark(state, options, isDuplex) { +function getHighWaterMark(state, options, duplexKey, isDuplex) { if (options.highWaterMark != null) { return getHighWaterMarkFromProperty(options.highWaterMark, 'highWaterMark'); - } else if (isDuplex) { - if (state instanceof stream.Readable.ReadableState) { - if (options.readableHighWaterMark != null) { - return getHighWaterMarkFromProperty(options.readableHighWaterMark, - 'readableHighWaterMark'); - } - } else if (options.writableHighWaterMark != null) { - return getHighWaterMarkFromProperty(options.writableHighWaterMark, - 'writableHighWaterMark'); - } + } else if (isDuplex && options[duplexKey] != null) { + return getHighWaterMarkFromProperty(options[duplexKey], duplexKey); } // Default value From 07f69514f93acbe24a444944906d26619184d036 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Thu, 11 Jan 2018 14:27:30 +0100 Subject: [PATCH 6/7] fixup! stream: add type and range check for highWaterMark --- lib/internal/streams/state.js | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/internal/streams/state.js b/lib/internal/streams/state.js index 013913fe593ef1..cc1c0daa0ae24a 100644 --- a/lib/internal/streams/state.js +++ b/lib/internal/streams/state.js @@ -3,17 +3,19 @@ const stream = require('stream'); const errors = require('internal/errors'); -function getHighWaterMarkFromProperty(hwm, name) { - if (typeof hwm !== 'number' || !(hwm >= 0)) - throw new errors.TypeError('ERR_INVALID_OPT_VALUE', name, hwm); - return Math.floor(hwm); -} - function getHighWaterMark(state, options, duplexKey, isDuplex) { - if (options.highWaterMark != null) { - return getHighWaterMarkFromProperty(options.highWaterMark, 'highWaterMark'); - } else if (isDuplex && options[duplexKey] != null) { - return getHighWaterMarkFromProperty(options[duplexKey], duplexKey); + let hwm = options.highWaterMark; + if (hwm != null) { + if (typeof hwm !== 'number' || !(hwm >= 0)) + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'highWaterMark', hwm); + return Math.floor(hwm); + } else if (isDuplex) { + hwm = options[duplexKey]; + if (hwm != null) { + if (typeof hwm !== 'number' || !(hwm >= 0)) + throw new errors.TypeError('ERR_INVALID_OPT_VALUE', duplexKey, hwm); + return Math.floor(hwm); + } } // Default value From 6764dc8948e9ad0119072508c8b8236a4b2099d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Mon, 22 Jan 2018 19:37:10 +0100 Subject: [PATCH 7/7] fixup! stream: add type and range check for highWaterMark --- lib/internal/streams/state.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/internal/streams/state.js b/lib/internal/streams/state.js index cc1c0daa0ae24a..cca79c93de49b5 100644 --- a/lib/internal/streams/state.js +++ b/lib/internal/streams/state.js @@ -1,6 +1,5 @@ 'use strict'; -const stream = require('stream'); const errors = require('internal/errors'); function getHighWaterMark(state, options, duplexKey, isDuplex) {