From 8d23afc807303a9afc1851802e1d88f0d3a84c29 Mon Sep 17 00:00:00 2001 From: Delapouite Date: Thu, 7 Dec 2017 11:11:41 +0100 Subject: [PATCH 01/29] doc: add link to debugger in process.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/17522 Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig Reviewed-By: Tobias Nießen Reviewed-By: Anatoli Papirovski Reviewed-By: Jon Moss Reviewed-By: Luigi Pinca Reviewed-By: Myles Borins --- doc/api/process.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/api/process.md b/doc/api/process.md index 54704e6775d..8c91a6c9d7c 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -379,7 +379,7 @@ terminal programs. It is important to take note of the following: -* `SIGUSR1` is reserved by Node.js to start the debugger. It's possible to +* `SIGUSR1` is reserved by Node.js to start the [debugger][]. It's possible to install a listener but doing so will _not_ stop the debugger from starting. * `SIGTERM` and `SIGINT` have default handlers on non-Windows platforms that resets the terminal mode before exiting with code `128 + signal number`. If @@ -1971,6 +1971,7 @@ cases: [`v8.setFlagsFromString()`]: v8.html#v8_v8_setflagsfromstring_flags [Child Process]: child_process.html [Cluster]: cluster.html +[debugger]: debugger.html [Duplex]: stream.html#stream_duplex_and_transform_streams [LTS]: https://github.com/nodejs/LTS/ [note on process I/O]: process.html#process_a_note_on_process_i_o From f3aaaa52b738e20192468a5157721c0a62027a16 Mon Sep 17 00:00:00 2001 From: Mithun Sasidharan Date: Fri, 8 Dec 2017 22:44:28 +0530 Subject: [PATCH 02/29] test: refactored to remove unnecessary variables PR-URL: https://github.com/nodejs/node/pull/17553 Reviewed-By: Jon Moss Reviewed-By: Luigi Pinca --- test/parallel/test-net-connect-options-fd.js | 5 ++--- test/parallel/test-net-connect-options-path.js | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/test/parallel/test-net-connect-options-fd.js b/test/parallel/test-net-connect-options-fd.js index 1ffc92257ce..50c2a08efeb 100644 --- a/test/parallel/test-net-connect-options-fd.js +++ b/test/parallel/test-net-connect-options-fd.js @@ -83,14 +83,13 @@ const forAllClients = (cb) => common.mustCall(cb, CLIENT_VARIANTS); path: serverPath }); const getConnectCb = (index) => common.mustCall(function clientOnConnect() { - const client = this; // Test if it's wrapping an existing fd assert(handleMap.has(index)); const oldHandle = handleMap.get(index); assert.strictEqual(oldHandle.fd, this._handle.fd); - client.write(String(oldHandle.fd)); + this.write(String(oldHandle.fd)); console.error(`[Pipe]Sending data through fd ${oldHandle.fd}`); - client.on('error', function(err) { + this.on('error', function(err) { console.error(err); assert.fail(null, null, `[Pipe Client]${err}`); }); diff --git a/test/parallel/test-net-connect-options-path.js b/test/parallel/test-net-connect-options-path.js index 07c5446fc6c..3868b85a78a 100644 --- a/test/parallel/test-net-connect-options-path.js +++ b/test/parallel/test-net-connect-options-path.js @@ -20,9 +20,8 @@ const CLIENT_VARIANTS = 12; }, CLIENT_VARIANTS)) .listen(serverPath, common.mustCall(function() { const getConnectCb = () => common.mustCall(function() { - const client = this; - client.end(); - client.on('close', common.mustCall(function() { + this.end(); + this.on('close', common.mustCall(function() { counter++; if (counter === CLIENT_VARIANTS) { server.close(); From 4df7c190bf449e3d0d8877f45bb7490d7a801118 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Sat, 9 Dec 2017 17:32:48 -0500 Subject: [PATCH 03/29] benchmark: fix http/simple.js benchmark autocannon appears to have trouble recognizing URLs that contain true or false within them. Use 0 or 1 instead to represent the same. PR-URL: https://github.com/nodejs/node/pull/17583 Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig Reviewed-By: Anna Henningsen --- benchmark/fixtures/simple-http-server.js | 2 +- benchmark/http/simple.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/benchmark/fixtures/simple-http-server.js b/benchmark/fixtures/simple-http-server.js index 5f56bebdafd..d2eda0c085c 100644 --- a/benchmark/fixtures/simple-http-server.js +++ b/benchmark/fixtures/simple-http-server.js @@ -34,7 +34,7 @@ module.exports = http.createServer(function(req, res) { const arg = params[2]; const n_chunks = parseInt(params[3], 10); const resHow = params.length >= 5 ? params[4] : 'normal'; - const chunkedEnc = params.length >= 6 && params[5] === 'false' ? false : true; + const chunkedEnc = params.length >= 6 && params[5] === '0' ? false : true; var status = 200; var n, i; diff --git a/benchmark/http/simple.js b/benchmark/http/simple.js index 238cd1885d3..544aad49688 100644 --- a/benchmark/http/simple.js +++ b/benchmark/http/simple.js @@ -8,14 +8,14 @@ const bench = common.createBenchmark(main, { len: [4, 1024, 102400], chunks: [1, 4], c: [50, 500], - chunkedEnc: ['true', 'false'], + chunkedEnc: [1, 0], res: ['normal', 'setHeader', 'setHeaderWH'] }); function main(conf) { process.env.PORT = PORT; var server = require('../fixtures/simple-http-server.js') - .listen(process.env.PORT || common.PORT) + .listen(PORT) .on('listening', function() { const path = `/${conf.type}/${conf.len}/${conf.chunks}/${conf.res}/${conf.chunkedEnc}`; From 094bfaf769f5b6bc8ca6fbcc289cdf5e0f07073f Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Fri, 8 Dec 2017 16:41:38 -0500 Subject: [PATCH 04/29] test: replace assert.throws w/ common.expectsError PR-URL: https://github.com/nodejs/node/pull/17557 Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig --- .../test-embedder.api.async-resource.js | 6 +- test/parallel/test-assert-fail.js | 2 + test/parallel/test-assert.js | 3 + test/parallel/test-buffer-alloc.js | 5 +- test/parallel/test-buffer-fill.js | 10 ++-- test/parallel/test-console-instance.js | 6 +- test/parallel/test-dns.js | 16 ++--- test/parallel/test-http-outgoing-proto.js | 60 +++++++++---------- ...est-http2-client-request-options-errors.js | 7 +-- test/parallel/test-http2-connect-method.js | 18 +++--- test/parallel/test-http2-respond-file-204.js | 7 +-- .../test-http2-server-push-disabled.js | 6 +- .../test-http2-server-rst-before-respond.js | 6 +- .../test-https-options-boolean-check.js | 12 ++-- test/parallel/test-internal-errors.js | 20 +++---- test/parallel/test-performance.js | 13 ++-- test/parallel/test-performanceobserver.js | 39 ++++++------ test/parallel/test-readline-interface.js | 24 ++++---- test/sequential/test-tls-lookup.js | 6 +- 19 files changed, 134 insertions(+), 132 deletions(-) diff --git a/test/async-hooks/test-embedder.api.async-resource.js b/test/async-hooks/test-embedder.api.async-resource.js index eeeaa447c96..89a889b2019 100644 --- a/test/async-hooks/test-embedder.api.async-resource.js +++ b/test/async-hooks/test-embedder.api.async-resource.js @@ -17,12 +17,12 @@ common.expectsError( code: 'ERR_INVALID_ARG_TYPE', type: TypeError, }); -assert.throws(() => { +common.expectsError(() => { new AsyncResource('invalid_trigger_id', { triggerAsyncId: null }); -}, common.expectsError({ +}, { code: 'ERR_INVALID_ASYNC_ID', type: RangeError, -})); +}); assert.strictEqual( new AsyncResource('default_trigger_id').triggerAsyncId(), diff --git a/test/parallel/test-assert-fail.js b/test/parallel/test-assert-fail.js index 14d28e5cd00..8d67a6e63f5 100644 --- a/test/parallel/test-assert-fail.js +++ b/test/parallel/test-assert-fail.js @@ -1,5 +1,7 @@ 'use strict'; +/* eslint-disable prefer-common-expectserror */ + const common = require('../common'); const assert = require('assert'); diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index 8c4d38ea826..8bf9b0e6d34 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -20,6 +20,9 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. 'use strict'; + +/* eslint-disable prefer-common-expectserror */ + const common = require('../common'); const assert = require('assert'); const a = assert; diff --git a/test/parallel/test-buffer-alloc.js b/test/parallel/test-buffer-alloc.js index 272f8c20e1c..4d16f1ba3d8 100644 --- a/test/parallel/test-buffer-alloc.js +++ b/test/parallel/test-buffer-alloc.js @@ -974,12 +974,11 @@ assert.strictEqual(SlowBuffer.prototype.offset, undefined); } // ParseArrayIndex() should reject values that don't fit in a 32 bits size_t. -assert.throws(() => { +common.expectsError(() => { const a = Buffer.alloc(1); const b = Buffer.alloc(1); a.copy(b, 0, 0x100000000, 0x100000001); -}, common.expectsError( - { code: undefined, type: RangeError, message: 'Index out of range' })); +}, { code: undefined, type: RangeError, message: 'Index out of range' }); // Unpooled buffer (replaces SlowBuffer) { diff --git a/test/parallel/test-buffer-fill.js b/test/parallel/test-buffer-fill.js index d33af46f724..f72c26fd48e 100644 --- a/test/parallel/test-buffer-fill.js +++ b/test/parallel/test-buffer-fill.js @@ -424,21 +424,19 @@ common.expectsError(() => { // Testing process.binding. Make sure "end" is properly checked for -1 wrap // around. -assert.throws(() => { +common.expectsError(() => { process.binding('buffer').fill(Buffer.alloc(1), 1, 1, -2, 1); -}, common.expectsError( - { code: undefined, type: RangeError, message: 'Index out of range' })); +}, { code: undefined, type: RangeError, message: 'Index out of range' }); // Test that bypassing 'length' won't cause an abort. -assert.throws(() => { +common.expectsError(() => { const buf = new Buffer('w00t'); Object.defineProperty(buf, 'length', { value: 1337, enumerable: true }); buf.fill(''); -}, common.expectsError( - { code: undefined, type: RangeError, message: 'Index out of range' })); +}, { code: undefined, type: RangeError, message: 'Index out of range' }); assert.deepStrictEqual( Buffer.allocUnsafeSlow(16).fill('ab', 'utf16le'), diff --git a/test/parallel/test-console-instance.js b/test/parallel/test-console-instance.js index 9f31ebf3afc..7e931e0d3b3 100644 --- a/test/parallel/test-console-instance.js +++ b/test/parallel/test-console-instance.js @@ -47,17 +47,17 @@ common.expectsError( ); // Console constructor should throw if stderr exists but is not writable -assert.throws( +common.expectsError( () => { out.write = () => {}; err.write = undefined; new Console(out, err); }, - common.expectsError({ + { code: 'ERR_CONSOLE_WRITABLE_STREAM', type: TypeError, message: /stderr/ - }) + } ); out.write = err.write = (d) => {}; diff --git a/test/parallel/test-dns.js b/test/parallel/test-dns.js index f0e4b29d7c9..dba14b397bc 100644 --- a/test/parallel/test-dns.js +++ b/test/parallel/test-dns.js @@ -205,20 +205,20 @@ assert.doesNotThrow(() => { }, common.mustCall()); }); -assert.throws(() => dns.lookupService('0.0.0.0'), common.expectsError({ +common.expectsError(() => dns.lookupService('0.0.0.0'), { code: 'ERR_MISSING_ARGS', type: TypeError, message: 'The "host", "port", and "callback" arguments must be specified' -})); +}); const invalidHost = 'fasdfdsaf'; -assert.throws(() => { +common.expectsError(() => { dns.lookupService(invalidHost, 0, common.mustNotCall()); -}, common.expectsError({ +}, { code: 'ERR_INVALID_OPT_VALUE', type: TypeError, message: `The value "${invalidHost}" is invalid for option "host"` -})); +}); const portErr = (port) => { common.expectsError( @@ -238,9 +238,9 @@ portErr(undefined); portErr(65538); portErr('test'); -assert.throws(() => { +common.expectsError(() => { dns.lookupService('0.0.0.0', 80, null); -}, common.expectsError({ +}, { code: 'ERR_INVALID_CALLBACK', type: TypeError -})); +}); diff --git a/test/parallel/test-http-outgoing-proto.js b/test/parallel/test-http-outgoing-proto.js index 47dbb07e2be..6b7987db10d 100644 --- a/test/parallel/test-http-outgoing-proto.js +++ b/test/parallel/test-http-outgoing-proto.js @@ -21,80 +21,80 @@ assert.strictEqual( typeof ServerResponse.prototype._implicitHeader, 'function'); // validateHeader -assert.throws(() => { +common.expectsError(() => { const outgoingMessage = new OutgoingMessage(); outgoingMessage.setHeader(); -}, common.expectsError({ +}, { code: 'ERR_INVALID_HTTP_TOKEN', type: TypeError, message: 'Header name must be a valid HTTP token ["undefined"]' -})); +}); -assert.throws(() => { +common.expectsError(() => { const outgoingMessage = new OutgoingMessage(); outgoingMessage.setHeader('test'); -}, common.expectsError({ +}, { code: 'ERR_HTTP_INVALID_HEADER_VALUE', type: TypeError, message: 'Invalid value "undefined" for header "test"' -})); +}); -assert.throws(() => { +common.expectsError(() => { const outgoingMessage = new OutgoingMessage(); outgoingMessage.setHeader(404); -}, common.expectsError({ +}, { code: 'ERR_INVALID_HTTP_TOKEN', type: TypeError, message: 'Header name must be a valid HTTP token ["404"]' -})); +}); -assert.throws(() => { +common.expectsError(() => { const outgoingMessage = new OutgoingMessage(); outgoingMessage.setHeader.call({ _header: 'test' }, 'test', 'value'); -}, common.expectsError({ +}, { code: 'ERR_HTTP_HEADERS_SENT', type: Error, message: 'Cannot set headers after they are sent to the client' -})); +}); -assert.throws(() => { +common.expectsError(() => { const outgoingMessage = new OutgoingMessage(); outgoingMessage.setHeader('200', 'あ'); -}, common.expectsError({ +}, { code: 'ERR_INVALID_CHAR', type: TypeError, message: 'Invalid character in header content ["200"]' -})); +}); // write -assert.throws(() => { +common.expectsError(() => { const outgoingMessage = new OutgoingMessage(); outgoingMessage.write(); -}, common.expectsError({ +}, { code: 'ERR_METHOD_NOT_IMPLEMENTED', type: Error, message: 'The _implicitHeader() method is not implemented' -})); +}); assert(OutgoingMessage.prototype.write.call({ _header: 'test' })); -assert.throws(() => { +common.expectsError(() => { const outgoingMessage = new OutgoingMessage(); outgoingMessage.write.call({ _header: 'test', _hasBody: 'test' }); -}, common.expectsError({ +}, { code: 'ERR_INVALID_ARG_TYPE', type: TypeError, message: 'The first argument must be one of type string or Buffer' -})); +}); -assert.throws(() => { +common.expectsError(() => { const outgoingMessage = new OutgoingMessage(); outgoingMessage.write.call({ _header: 'test', _hasBody: 'test' }, 1); -}, common.expectsError({ +}, { code: 'ERR_INVALID_ARG_TYPE', type: TypeError, message: 'The first argument must be one of type string or Buffer' -})); +}); // addTrailers() // The `Error` comes from the JavaScript engine so confirm that it is a @@ -105,20 +105,20 @@ assert.throws(() => { outgoingMessage.addTrailers(); }, TypeError); -assert.throws(() => { +common.expectsError(() => { const outgoingMessage = new OutgoingMessage(); outgoingMessage.addTrailers({ 'あ': 'value' }); -}, common.expectsError({ +}, { code: 'ERR_INVALID_HTTP_TOKEN', type: TypeError, message: 'Trailer name must be a valid HTTP token ["あ"]' -})); +}); -assert.throws(() => { +common.expectsError(() => { const outgoingMessage = new OutgoingMessage(); outgoingMessage.addTrailers({ 404: 'あ' }); -}, common.expectsError({ +}, { code: 'ERR_INVALID_CHAR', type: TypeError, message: 'Invalid character in trailer content ["404"]' -})); +}); diff --git a/test/parallel/test-http2-client-request-options-errors.js b/test/parallel/test-http2-client-request-options-errors.js index 5d3fc0ab5a1..4b146a1bc8c 100644 --- a/test/parallel/test-http2-client-request-options-errors.js +++ b/test/parallel/test-http2-client-request-options-errors.js @@ -3,7 +3,6 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); -const assert = require('assert'); const http2 = require('http2'); // Check if correct errors are emitted when wrong type of data is passed @@ -40,19 +39,19 @@ server.listen(0, common.mustCall(() => { return; } - assert.throws( + common.expectsError( () => client.request({ ':method': 'CONNECT', ':authority': `localhost:${port}` }, { [option]: types[type] }), - common.expectsError({ + { type: TypeError, code: 'ERR_INVALID_OPT_VALUE', message: `The value "${String(types[type])}" is invalid ` + `for option "${option}"` - }) + } ); }); }); diff --git a/test/parallel/test-http2-connect-method.js b/test/parallel/test-http2-connect-method.js index 78c9a345293..4d443d5c217 100644 --- a/test/parallel/test-http2-connect-method.js +++ b/test/parallel/test-http2-connect-method.js @@ -55,36 +55,36 @@ server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${proxy.address().port}`); // confirm that :authority is required and :scheme & :path are forbidden - assert.throws( + common.expectsError( () => client.request({ [HTTP2_HEADER_METHOD]: 'CONNECT' }), - common.expectsError({ + { code: 'ERR_HTTP2_CONNECT_AUTHORITY', message: ':authority header is required for CONNECT requests' - }) + } ); - assert.throws( + common.expectsError( () => client.request({ [HTTP2_HEADER_METHOD]: 'CONNECT', [HTTP2_HEADER_AUTHORITY]: `localhost:${port}`, [HTTP2_HEADER_SCHEME]: 'http' }), - common.expectsError({ + { code: 'ERR_HTTP2_CONNECT_SCHEME', message: 'The :scheme header is forbidden for CONNECT requests' - }) + } ); - assert.throws( + common.expectsError( () => client.request({ [HTTP2_HEADER_METHOD]: 'CONNECT', [HTTP2_HEADER_AUTHORITY]: `localhost:${port}`, [HTTP2_HEADER_PATH]: '/' }), - common.expectsError({ + { code: 'ERR_HTTP2_CONNECT_PATH', message: 'The :path header is forbidden for CONNECT requests' - }) + } ); // valid CONNECT request diff --git a/test/parallel/test-http2-respond-file-204.js b/test/parallel/test-http2-respond-file-204.js index 8181dbb317d..4be2d42c779 100644 --- a/test/parallel/test-http2-respond-file-204.js +++ b/test/parallel/test-http2-respond-file-204.js @@ -5,7 +5,6 @@ if (!common.hasCrypto) common.skip('missing crypto'); const fixtures = require('../common/fixtures'); const http2 = require('http2'); -const assert = require('assert'); const { HTTP2_HEADER_CONTENT_TYPE, @@ -16,16 +15,16 @@ const fname = fixtures.path('elipses.txt'); const server = http2.createServer(); server.on('stream', (stream) => { - assert.throws(() => { + common.expectsError(() => { stream.respondWithFile(fname, { [HTTP2_HEADER_STATUS]: 204, [HTTP2_HEADER_CONTENT_TYPE]: 'text/plain' }); - }, common.expectsError({ + }, { code: 'ERR_HTTP2_PAYLOAD_FORBIDDEN', type: Error, message: 'Responses with 204 status must not have a payload' - })); + }); stream.respond({}); stream.end(); }); diff --git a/test/parallel/test-http2-server-push-disabled.js b/test/parallel/test-http2-server-push-disabled.js index c0148fe63b6..33390f2ecae 100644 --- a/test/parallel/test-http2-server-push-disabled.js +++ b/test/parallel/test-http2-server-push-disabled.js @@ -21,16 +21,16 @@ server.on('stream', common.mustCall((stream) => { // and pushStream() must throw. assert.strictEqual(stream.pushAllowed, false); - assert.throws(() => { + common.expectsError(() => { stream.pushStream({ ':scheme': 'http', ':path': '/foobar', ':authority': `localhost:${server.address().port}`, }, common.mustNotCall()); - }, common.expectsError({ + }, { code: 'ERR_HTTP2_PUSH_DISABLED', type: Error - })); + }); stream.respond({ ':status': 200 }); stream.end('test'); diff --git a/test/parallel/test-http2-server-rst-before-respond.js b/test/parallel/test-http2-server-rst-before-respond.js index 47ba68bd29e..950beea4eb3 100644 --- a/test/parallel/test-http2-server-rst-before-respond.js +++ b/test/parallel/test-http2-server-rst-before-respond.js @@ -14,15 +14,15 @@ server.on('stream', common.mustCall(onStream)); function onStream(stream, headers, flags) { stream.rstStream(); - assert.throws(() => { + common.expectsError(() => { stream.additionalHeaders({ ':status': 123, abc: 123 }); - }, common.expectsError({ + }, { code: 'ERR_HTTP2_INVALID_STREAM', message: /^The stream has been destroyed$/ - })); + }); } server.listen(0); diff --git a/test/parallel/test-https-options-boolean-check.js b/test/parallel/test-https-options-boolean-check.js index eae319988e7..ed124d43fbb 100644 --- a/test/parallel/test-https-options-boolean-check.js +++ b/test/parallel/test-https-options-boolean-check.js @@ -101,16 +101,16 @@ const invalidCertRE = /^The "cert" argument must be one of type string, Buffer, [[keyStr, keyStr2], true, invalidCertRE], [true, [certBuff, certBuff2], invalidKeyRE] ].map((params) => { - assert.throws(() => { + common.expectsError(() => { https.createServer({ key: params[0], cert: params[1] }); - }, common.expectsError({ + }, { code: 'ERR_INVALID_ARG_TYPE', type: TypeError, message: params[2] - })); + }); }); // Checks to ensure https.createServer works with the CA parameter @@ -142,15 +142,15 @@ const invalidCertRE = /^The "cert" argument must be one of type string, Buffer, [keyBuff, certBuff, true], [keyBuff, certBuff, [caCert, true]] ].map((params) => { - assert.throws(() => { + common.expectsError(() => { https.createServer({ key: params[0], cert: params[1], ca: params[2] }); - }, common.expectsError({ + }, { code: 'ERR_INVALID_ARG_TYPE', type: TypeError, message: /^The "ca" argument must be one of type string, Buffer, TypedArray, or DataView$/ - })); + }); }); diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js index 1b4abcf72c9..78582a53504 100644 --- a/test/parallel/test-internal-errors.js +++ b/test/parallel/test-internal-errors.js @@ -141,29 +141,29 @@ common.expectsError( // Tests for common.expectsError assert.doesNotThrow(() => { - assert.throws(() => { + common.expectsError(() => { throw new errors.TypeError('TEST_ERROR_1', 'a'); - }, common.expectsError({ code: 'TEST_ERROR_1' })); + }, { code: 'TEST_ERROR_1' }); }); assert.doesNotThrow(() => { - assert.throws(() => { + common.expectsError(() => { throw new errors.TypeError('TEST_ERROR_1', 'a'); - }, common.expectsError({ code: 'TEST_ERROR_1', - type: TypeError, - message: /^Error for testing/ })); + }, { code: 'TEST_ERROR_1', + type: TypeError, + message: /^Error for testing/ }); }); assert.doesNotThrow(() => { - assert.throws(() => { + common.expectsError(() => { throw new errors.TypeError('TEST_ERROR_1', 'a'); - }, common.expectsError({ code: 'TEST_ERROR_1', type: TypeError })); + }, { code: 'TEST_ERROR_1', type: TypeError }); }); assert.doesNotThrow(() => { - assert.throws(() => { + common.expectsError(() => { throw new errors.TypeError('TEST_ERROR_1', 'a'); - }, common.expectsError({ code: 'TEST_ERROR_1', type: Error })); + }, { code: 'TEST_ERROR_1', type: Error }); }); common.expectsError(() => { diff --git a/test/parallel/test-performance.js b/test/parallel/test-performance.js index 23ae4370c3c..ba15479050f 100644 --- a/test/parallel/test-performance.js +++ b/test/parallel/test-performance.js @@ -68,12 +68,13 @@ assert.strictEqual(typeof performance.timeOrigin, 'number'); }); [undefined, null, 'foo', 1].forEach((i) => { - assert.throws(() => performance.measure('test', 'A', i), - common.expectsError({ - code: 'ERR_INVALID_PERFORMANCE_MARK', - type: Error, - message: `The "${i}" performance mark has not been set` - })); + common.expectsError( + () => performance.measure('test', 'A', i), + { + code: 'ERR_INVALID_PERFORMANCE_MARK', + type: Error, + message: `The "${i}" performance mark has not been set` + }); }); performance.clearMeasures(); diff --git a/test/parallel/test-performanceobserver.js b/test/parallel/test-performanceobserver.js index 9b7dba5eb2a..779e9740d7c 100644 --- a/test/parallel/test-performanceobserver.js +++ b/test/parallel/test-performanceobserver.js @@ -28,33 +28,34 @@ assert.strictEqual(counts[NODE_PERFORMANCE_ENTRY_TYPE_FUNCTION], 0); { [1, null, undefined, {}, [], Infinity].forEach((i) => { - assert.throws(() => new PerformanceObserver(i), - common.expectsError({ - code: 'ERR_INVALID_CALLBACK', - type: TypeError, - message: 'Callback must be a function' - })); + common.expectsError(() => new PerformanceObserver(i), + { + code: 'ERR_INVALID_CALLBACK', + type: TypeError, + message: 'Callback must be a function' + }); }); const observer = new PerformanceObserver(common.mustNotCall()); [1, null, undefined].forEach((i) => { //observer.observe(i); - assert.throws(() => observer.observe(i), - common.expectsError({ - code: 'ERR_INVALID_ARG_TYPE', - type: TypeError, - message: 'The "options" argument must be of type Object' - })); + common.expectsError( + () => observer.observe(i), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "options" argument must be of type Object' + }); }); [1, undefined, null, {}, Infinity].forEach((i) => { - assert.throws(() => observer.observe({ entryTypes: i }), - common.expectsError({ - code: 'ERR_INVALID_OPT_VALUE', - type: TypeError, - message: 'The value "[object Object]" is invalid for ' + - 'option "entryTypes"' - })); + common.expectsError(() => observer.observe({ entryTypes: i }), + { + code: 'ERR_INVALID_OPT_VALUE', + type: TypeError, + message: 'The value "[object Object]" is invalid ' + + 'for option "entryTypes"' + }); }); } diff --git a/test/parallel/test-readline-interface.js b/test/parallel/test-readline-interface.js index fd622f8cabc..22cb5891bb5 100644 --- a/test/parallel/test-readline-interface.js +++ b/test/parallel/test-readline-interface.js @@ -357,46 +357,46 @@ function isWarned(emitter) { // constructor throws if completer is not a function or undefined { const fi = new FakeInput(); - assert.throws(function() { + common.expectsError(function() { readline.createInterface({ input: fi, completer: 'string is not valid' }); - }, common.expectsError({ + }, { type: TypeError, code: 'ERR_INVALID_OPT_VALUE' - })); + }); } // constructor throws if historySize is not a positive number { const fi = new FakeInput(); - assert.throws(function() { + common.expectsError(function() { readline.createInterface({ input: fi, historySize: 'not a number' }); - }, common.expectsError({ + }, { type: RangeError, code: 'ERR_INVALID_OPT_VALUE' - })); + }); - assert.throws(function() { + common.expectsError(function() { readline.createInterface({ input: fi, historySize: -1 }); - }, common.expectsError({ + }, { type: RangeError, code: 'ERR_INVALID_OPT_VALUE' - })); + }); - assert.throws(function() { + common.expectsError(function() { readline.createInterface({ input: fi, historySize: NaN }); - }, common.expectsError({ + }, { type: RangeError, code: 'ERR_INVALID_OPT_VALUE' - })); + }); } // duplicate lines are removed from history when diff --git a/test/sequential/test-tls-lookup.js b/test/sequential/test-tls-lookup.js index fddbb20ee00..ff759cf2fe6 100644 --- a/test/sequential/test-tls-lookup.js +++ b/test/sequential/test-tls-lookup.js @@ -13,12 +13,12 @@ const tls = require('tls'); lookup: input }; - assert.throws(function() { + common.expectsError(function() { tls.connect(opts); - }, common.expectsError({ + }, { code: 'ERR_INVALID_ARG_TYPE', type: TypeError - })); + }); }); connectDoesNotThrow(common.mustCall(() => {})); From e51fb90a6db53588ab2b884e4309d4eea9e37bbd Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Fri, 8 Dec 2017 16:06:02 -0500 Subject: [PATCH 05/29] tools: prefer common.expectsError in tests Add lint rule to validate that common.expectsError(fn, err) is being used instead of assert.throws(fn, common.expectsError(err)); PR-URL: https://github.com/nodejs/node/pull/17557 Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig --- test/.eslintrc.yaml | 1 + .../test-eslint-prefer-common-expectserror.js | 27 +++++++++++++++++++ .../prefer-common-expectserror.js | 21 +++++++++++++++ 3 files changed, 49 insertions(+) create mode 100644 test/parallel/test-eslint-prefer-common-expectserror.js create mode 100644 tools/eslint-rules/prefer-common-expectserror.js diff --git a/test/.eslintrc.yaml b/test/.eslintrc.yaml index aa320996aa4..7a5d002e1b5 100644 --- a/test/.eslintrc.yaml +++ b/test/.eslintrc.yaml @@ -10,6 +10,7 @@ rules: # Custom rules in tools/eslint-rules prefer-assert-iferror: error prefer-assert-methods: error + prefer-common-expectserror: error prefer-common-mustnotcall: error crypto-check: error inspector-check: error diff --git a/test/parallel/test-eslint-prefer-common-expectserror.js b/test/parallel/test-eslint-prefer-common-expectserror.js new file mode 100644 index 00000000000..16ce66bc24e --- /dev/null +++ b/test/parallel/test-eslint-prefer-common-expectserror.js @@ -0,0 +1,27 @@ +'use strict'; + +require('../common'); + +const RuleTester = require('../../tools/eslint').RuleTester; +const rule = require('../../tools/eslint-rules/prefer-common-expectserror'); + +const message = 'Please use common.expectsError(fn, err) instead of ' + + 'assert.throws(fn, common.expectsError(err)).'; + +new RuleTester().run('prefer-common-expectserror', rule, { + valid: [ + 'assert.throws(fn, /[a-z]/)', + 'assert.throws(function () {}, function() {})', + 'common.expectsError(function() {}, err)' + ], + invalid: [ + { + code: 'assert.throws(function() {}, common.expectsError(err))', + errors: [{ message }] + }, + { + code: 'assert.throws(fn, common.expectsError(err))', + errors: [{ message }] + } + ] +}); diff --git a/tools/eslint-rules/prefer-common-expectserror.js b/tools/eslint-rules/prefer-common-expectserror.js new file mode 100644 index 00000000000..f33241697a6 --- /dev/null +++ b/tools/eslint-rules/prefer-common-expectserror.js @@ -0,0 +1,21 @@ +'use strict'; + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +const msg = 'Please use common.expectsError(fn, err) instead of ' + + 'assert.throws(fn, common.expectsError(err)).'; + +const astSelector = + 'CallExpression[arguments.length=2]' + + '[callee.object.name="assert"]' + + '[callee.property.name="throws"]' + + '[arguments.1.callee.object.name="common"]' + + '[arguments.1.callee.property.name="expectsError"]'; + +module.exports = function(context) { + return { + [astSelector]: (node) => context.report(node, msg) + }; +}; From 73ad3f9bea3993b486621aaf9e61484dc37741d4 Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Fri, 10 Nov 2017 16:01:00 -0800 Subject: [PATCH 06/29] inspector: Fix crash for WS connection Attaching WS session will now include a roundtrip onto the main thread to make sure there is no other session (e.g. JS bindings) This change also required refactoring WS socket implementation to better support scenarios like this. Fixes: https://github.com/nodejs/node/issues/16852 PR-URL: https://github.com/nodejs/node/pull/17085 Reviewed-By: James M Snell Reviewed-By: Timothy Gu --- src/inspector_agent.cc | 4 - src/inspector_agent.h | 1 - src/inspector_io.cc | 73 +- src/inspector_io.h | 8 +- src/inspector_socket.cc | 825 ++++++++++-------- src/inspector_socket.h | 96 +- src/inspector_socket_server.cc | 306 +++---- src/inspector_socket_server.h | 28 +- src/node.cc | 2 +- test/cctest/test_inspector_socket.cc | 601 ++++++------- test/cctest/test_inspector_socket_server.cc | 17 +- test/common/inspector-helper.js | 55 +- ...st-inspector-no-crash-ws-after-bindings.js | 30 + 13 files changed, 1022 insertions(+), 1024 deletions(-) create mode 100644 test/parallel/test-inspector-no-crash-ws-after-bindings.js diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 9677dca37b5..216b43ca6c3 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -548,10 +548,6 @@ void Agent::Connect(InspectorSessionDelegate* delegate) { client_->connectFrontend(delegate); } -bool Agent::IsConnected() { - return io_ && io_->IsConnected(); -} - void Agent::WaitForDisconnect() { CHECK_NE(client_, nullptr); client_->contextDestroyed(parent_env_->context()); diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 29b9546b514..a2d61d0c8db 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -48,7 +48,6 @@ class Agent { bool IsStarted() { return !!client_; } // IO thread started, and client connected - bool IsConnected(); bool IsWaitingForConnect(); void WaitForDisconnect(); diff --git a/src/inspector_io.cc b/src/inspector_io.cc index 538cbab3c9f..9af4458c6b2 100644 --- a/src/inspector_io.cc +++ b/src/inspector_io.cc @@ -136,7 +136,7 @@ class InspectorIoDelegate: public node::inspector::SocketServerDelegate { const std::string& script_name, bool wait); // Calls PostIncomingMessage() with appropriate InspectorAction: // kStartSession - bool StartSession(int session_id, const std::string& target_id) override; + void StartSession(int session_id, const std::string& target_id) override; // kSendMessage void MessageReceived(int session_id, const std::string& message) override; // kEndSession @@ -145,19 +145,22 @@ class InspectorIoDelegate: public node::inspector::SocketServerDelegate { std::vector GetTargetIds() override; std::string GetTargetTitle(const std::string& id) override; std::string GetTargetUrl(const std::string& id) override; - bool IsConnected() { return connected_; } void ServerDone() override { io_->ServerDone(); } + void AssignTransport(InspectorSocketServer* server) { + server_ = server; + } + private: InspectorIo* io_; - bool connected_; int session_id_; const std::string script_name_; const std::string script_path_; const std::string target_id_; bool waiting_; + InspectorSocketServer* server_; }; void InterruptCallback(v8::Isolate*, void* agent) { @@ -226,10 +229,6 @@ void InspectorIo::Stop() { DispatchMessages(); } -bool InspectorIo::IsConnected() { - return delegate_ != nullptr && delegate_->IsConnected(); -} - bool InspectorIo::IsStarted() { return platform_ != nullptr; } @@ -264,6 +263,7 @@ void InspectorIo::IoThreadAsyncCb(uv_async_t* async) { MessageQueue outgoing_message_queue; io->SwapBehindLock(&io->outgoing_message_queue_, &outgoing_message_queue); for (const auto& outgoing : outgoing_message_queue) { + int session_id = std::get<1>(outgoing); switch (std::get<0>(outgoing)) { case TransportAction::kKill: transport->TerminateConnections(); @@ -272,8 +272,14 @@ void InspectorIo::IoThreadAsyncCb(uv_async_t* async) { transport->Stop(nullptr); break; case TransportAction::kSendMessage: - std::string message = StringViewToUtf8(std::get<2>(outgoing)->string()); - transport->Send(std::get<1>(outgoing), message); + transport->Send(session_id, + StringViewToUtf8(std::get<2>(outgoing)->string())); + break; + case TransportAction::kAcceptSession: + transport->AcceptSession(session_id); + break; + case TransportAction::kDeclineSession: + transport->DeclineSession(session_id); break; } } @@ -293,6 +299,7 @@ void InspectorIo::ThreadMain() { wait_for_connect_); delegate_ = &delegate; Transport server(&delegate, &loop, options_.host_name(), options_.port()); + delegate.AssignTransport(&server); TransportAndIo queue_transport(&server, this); thread_req_.data = &queue_transport; if (!server.Start()) { @@ -308,6 +315,7 @@ void InspectorIo::ThreadMain() { uv_run(&loop, UV_RUN_DEFAULT); thread_req_.data = nullptr; CHECK_EQ(uv_loop_close(&loop), 0); + delegate.AssignTransport(nullptr); delegate_ = nullptr; } @@ -358,6 +366,21 @@ void InspectorIo::NotifyMessageReceived() { incoming_message_cond_.Broadcast(scoped_lock); } +TransportAction InspectorIo::Attach(int session_id) { + Agent* agent = parent_env_->inspector_agent(); + if (agent->delegate() != nullptr) + return TransportAction::kDeclineSession; + + CHECK_EQ(session_delegate_, nullptr); + session_id_ = session_id; + state_ = State::kConnected; + fprintf(stderr, "Debugger attached.\n"); + session_delegate_ = std::unique_ptr( + new IoSessionDelegate(this)); + agent->Connect(session_delegate_.get()); + return TransportAction::kAcceptSession; +} + void InspectorIo::DispatchMessages() { // This function can be reentered if there was an incoming message while // V8 was processing another inspector request (e.g. if the user is @@ -375,16 +398,14 @@ void InspectorIo::DispatchMessages() { MessageQueue::value_type task; std::swap(dispatching_message_queue_.front(), task); dispatching_message_queue_.pop_front(); + int id = std::get<1>(task); StringView message = std::get<2>(task)->string(); switch (std::get<0>(task)) { case InspectorAction::kStartSession: - CHECK_EQ(session_delegate_, nullptr); - session_id_ = std::get<1>(task); - state_ = State::kConnected; - fprintf(stderr, "Debugger attached.\n"); - session_delegate_ = std::unique_ptr( - new IoSessionDelegate(this)); - parent_env_->inspector_agent()->Connect(session_delegate_.get()); + Write(Attach(id), id, StringView()); + break; + case InspectorAction::kStartSessionUnconditionally: + Attach(id); break; case InspectorAction::kEndSession: CHECK_NE(session_delegate_, nullptr); @@ -428,22 +449,23 @@ InspectorIoDelegate::InspectorIoDelegate(InspectorIo* io, const std::string& script_name, bool wait) : io_(io), - connected_(false), session_id_(0), script_name_(script_name), script_path_(script_path), target_id_(GenerateID()), - waiting_(wait) { } + waiting_(wait), + server_(nullptr) { } -bool InspectorIoDelegate::StartSession(int session_id, +void InspectorIoDelegate::StartSession(int session_id, const std::string& target_id) { - if (connected_) - return false; - connected_ = true; - session_id_++; - io_->PostIncomingMessage(InspectorAction::kStartSession, session_id, ""); - return true; + session_id_ = session_id; + InspectorAction action = InspectorAction::kStartSession; + if (waiting_) { + action = InspectorAction::kStartSessionUnconditionally; + server_->AcceptSession(session_id); + } + io_->PostIncomingMessage(action, session_id, ""); } void InspectorIoDelegate::MessageReceived(int session_id, @@ -464,7 +486,6 @@ void InspectorIoDelegate::MessageReceived(int session_id, } void InspectorIoDelegate::EndSession(int session_id) { - connected_ = false; io_->PostIncomingMessage(InspectorAction::kEndSession, session_id, ""); } diff --git a/src/inspector_io.h b/src/inspector_io.h index 7c15466eed9..79ccc6095ff 100644 --- a/src/inspector_io.h +++ b/src/inspector_io.h @@ -36,6 +36,7 @@ class InspectorIoDelegate; enum class InspectorAction { kStartSession, + kStartSessionUnconditionally, // First attach with --inspect-brk kEndSession, kSendMessage }; @@ -44,7 +45,9 @@ enum class InspectorAction { enum class TransportAction { kKill, kSendMessage, - kStop + kStop, + kAcceptSession, + kDeclineSession }; class InspectorIo { @@ -61,7 +64,6 @@ class InspectorIo { void Stop(); bool IsStarted(); - bool IsConnected(); void WaitForDisconnect(); // Called from thread to queue an incoming message and trigger @@ -124,6 +126,8 @@ class InspectorIo { void WaitForFrontendMessageWhilePaused(); // Broadcast incoming_message_cond_ void NotifyMessageReceived(); + // Attach session to an inspector. Either kAcceptSession or kDeclineSession + TransportAction Attach(int session_id); const DebugOptions options_; diff --git a/src/inspector_socket.cc b/src/inspector_socket.cc index 49d337b70b1..23b77f6aa56 100644 --- a/src/inspector_socket.cc +++ b/src/inspector_socket.cc @@ -1,4 +1,6 @@ #include "inspector_socket.h" + +#include "http_parser.h" #include "util-inl.h" #define NODE_WANT_INTERNALS 1 @@ -18,12 +20,71 @@ namespace node { namespace inspector { -static const char CLOSE_FRAME[] = {'\x88', '\x00'}; +class TcpHolder { + public: + using Pointer = std::unique_ptr; -enum ws_decode_result { - FRAME_OK, FRAME_INCOMPLETE, FRAME_CLOSE, FRAME_ERROR + static Pointer Accept(uv_stream_t* server, + InspectorSocket::DelegatePointer delegate); + void SetHandler(ProtocolHandler* handler); + int WriteRaw(const std::vector& buffer, uv_write_cb write_cb); + uv_tcp_t* tcp() { + return &tcp_; + } + InspectorSocket::Delegate* delegate(); + + private: + static TcpHolder* From(void* handle) { + return node::ContainerOf(&TcpHolder::tcp_, + reinterpret_cast(handle)); + } + static void OnClosed(uv_handle_t* handle); + static void OnDataReceivedCb(uv_stream_t* stream, ssize_t nread, + const uv_buf_t* buf); + static void DisconnectAndDispose(TcpHolder* holder); + explicit TcpHolder(InspectorSocket::DelegatePointer delegate); + ~TcpHolder() = default; + void ReclaimUvBuf(const uv_buf_t* buf, ssize_t read); + + uv_tcp_t tcp_; + const InspectorSocket::DelegatePointer delegate_; + ProtocolHandler* handler_; + std::vector buffer; +}; + + +class ProtocolHandler { + public: + ProtocolHandler(InspectorSocket* inspector, TcpHolder::Pointer tcp); + + virtual void AcceptUpgrade(const std::string& accept_key) = 0; + virtual void OnData(std::vector* data) = 0; + virtual void OnEof() = 0; + virtual void Write(const std::vector data) = 0; + virtual void CancelHandshake() = 0; + + std::string GetHost(); + + InspectorSocket* inspector() { + return inspector_; + } + + static void Shutdown(ProtocolHandler* handler) { + handler->Shutdown(); + } + + protected: + virtual ~ProtocolHandler() = default; + virtual void Shutdown() = 0; + int WriteRaw(const std::vector& buffer, uv_write_cb write_cb); + InspectorSocket::Delegate* delegate(); + + InspectorSocket* const inspector_; + TcpHolder::Pointer tcp_; }; +namespace { + #if DUMP_READS || DUMP_WRITES static void dump_hex(const char* buf, size_t len) { const char* ptr = buf; @@ -50,64 +111,52 @@ static void dump_hex(const char* buf, size_t len) { } #endif -static void remove_from_beginning(std::vector* buffer, size_t count) { - buffer->erase(buffer->begin(), buffer->begin() + count); -} - -static void dispose_inspector(uv_handle_t* handle) { - InspectorSocket* inspector = inspector_from_stream(handle); - inspector_cb close = - inspector->ws_mode ? inspector->ws_state->close_cb : nullptr; - inspector->buffer.clear(); - delete inspector->ws_state; - inspector->ws_state = nullptr; - if (close) { - close(inspector, 0); - } -} - -static void close_connection(InspectorSocket* inspector) { - uv_handle_t* socket = reinterpret_cast(&inspector->tcp); - if (!uv_is_closing(socket)) { - uv_read_stop(reinterpret_cast(socket)); - uv_close(socket, dispose_inspector); - } -} - -struct WriteRequest { - WriteRequest(InspectorSocket* inspector, const char* data, size_t size) - : inspector(inspector) - , storage(data, data + size) - , buf(uv_buf_init(&storage[0], storage.size())) {} +class WriteRequest { + public: + WriteRequest(ProtocolHandler* handler, const std::vector& buffer) + : handler(handler) + , storage(buffer) + , buf(uv_buf_init(storage.data(), storage.size())) {} static WriteRequest* from_write_req(uv_write_t* req) { return node::ContainerOf(&WriteRequest::req, req); } - InspectorSocket* const inspector; + static void Cleanup(uv_write_t* req, int status) { + delete WriteRequest::from_write_req(req); + } + + ProtocolHandler* const handler; std::vector storage; uv_write_t req; uv_buf_t buf; }; -// Cleanup -static void write_request_cleanup(uv_write_t* req, int status) { - delete WriteRequest::from_write_req(req); +void allocate_buffer(uv_handle_t* stream, size_t len, uv_buf_t* buf) { + *buf = uv_buf_init(new char[len], len); } -static int write_to_client(InspectorSocket* inspector, - const char* msg, - size_t len, - uv_write_cb write_cb = write_request_cleanup) { -#if DUMP_WRITES - printf("%s (%ld bytes):\n", __FUNCTION__, len); - dump_hex(msg, len); -#endif +static void remove_from_beginning(std::vector* buffer, size_t count) { + buffer->erase(buffer->begin(), buffer->begin() + count); +} - // Freed in write_request_cleanup - WriteRequest* wr = new WriteRequest(inspector, msg, len); - uv_stream_t* stream = reinterpret_cast(&inspector->tcp); - return uv_write(&wr->req, stream, &wr->buf, 1, write_cb) < 0; +// Cleanup + +static const char CLOSE_FRAME[] = {'\x88', '\x00'}; + +enum ws_decode_result { + FRAME_OK, FRAME_INCOMPLETE, FRAME_CLOSE, FRAME_ERROR +}; + +static void generate_accept_string(const std::string& client_key, + char (*buffer)[ACCEPT_KEY_LENGTH]) { + // Magic string from websockets spec. + static const char ws_magic[] = "258EAFA5-E914-47DA-95CA-C5AB0DC85B11"; + std::string input(client_key + ws_magic); + char hash[SHA_DIGEST_LENGTH]; + SHA1(reinterpret_cast(&input[0]), input.size(), + reinterpret_cast(hash)); + node::base64_encode(hash, sizeof(hash), *buffer, sizeof(*buffer)); } // Constants for hybi-10 frame format. @@ -134,11 +183,11 @@ const size_t kTwoBytePayloadLengthField = 126; const size_t kEightBytePayloadLengthField = 127; const size_t kMaskingKeyWidthInBytes = 4; -static std::vector encode_frame_hybi17(const char* message, - size_t data_length) { +static std::vector encode_frame_hybi17(const std::vector& message) { std::vector frame; OpCode op_code = kOpCodeText; frame.push_back(kFinalBit | op_code); + const size_t data_length = message.size(); if (data_length <= kMaxSingleBytePayloadLength) { frame.push_back(static_cast(data_length)); } else if (data_length <= 0xFFFF) { @@ -158,7 +207,7 @@ static std::vector encode_frame_hybi17(const char* message, extended_payload_length + 8); CHECK_EQ(0, remaining); } - frame.insert(frame.end(), message, message + data_length); + frame.insert(frame.end(), message.begin(), message.end()); return frame; } @@ -248,272 +297,368 @@ static ws_decode_result decode_frame_hybi17(const std::vector& buffer, return closed ? FRAME_CLOSE : FRAME_OK; } -static void invoke_read_callback(InspectorSocket* inspector, - int status, const uv_buf_t* buf) { - if (inspector->ws_state->read_cb) { - inspector->ws_state->read_cb( - reinterpret_cast(&inspector->tcp), status, buf); + +// WS protocol +class WsHandler : public ProtocolHandler { + public: + WsHandler(InspectorSocket* inspector, TcpHolder::Pointer tcp) + : ProtocolHandler(inspector, std::move(tcp)), + OnCloseSent(&WsHandler::WaitForCloseReply), + OnCloseRecieved(&WsHandler::CloseFrameReceived), + dispose_(false) { } + + void AcceptUpgrade(const std::string& accept_key) override { } + void CancelHandshake() override {} + + void OnEof() override { + tcp_.reset(); + if (dispose_) + delete this; } -} -static void shutdown_complete(InspectorSocket* inspector) { - close_connection(inspector); -} + void OnData(std::vector* data) override { + // 1. Parse. + int processed = 0; + do { + processed = ParseWsFrames(*data); + // 2. Fix the data size & length + if (processed > 0) { + remove_from_beginning(data, processed); + } + } while (processed > 0 && !data->empty()); + } -static void on_close_frame_written(uv_write_t* req, int status) { - WriteRequest* wr = WriteRequest::from_write_req(req); - InspectorSocket* inspector = wr->inspector; - delete wr; - inspector->ws_state->close_sent = true; - if (inspector->ws_state->received_close) { - shutdown_complete(inspector); + void Write(const std::vector data) { + std::vector output = encode_frame_hybi17(data); + WriteRaw(output, WriteRequest::Cleanup); } -} -static void close_frame_received(InspectorSocket* inspector) { - inspector->ws_state->received_close = true; - if (!inspector->ws_state->close_sent) { - invoke_read_callback(inspector, 0, 0); - write_to_client(inspector, CLOSE_FRAME, sizeof(CLOSE_FRAME), - on_close_frame_written); - } else { - shutdown_complete(inspector); + protected: + void Shutdown() override { + if (tcp_) { + dispose_ = true; + SendClose(); + } else { + delete this; + } } -} -static int parse_ws_frames(InspectorSocket* inspector) { - int bytes_consumed = 0; - std::vector output; - bool compressed = false; - - ws_decode_result r = decode_frame_hybi17(inspector->buffer, - true /* client_frame */, - &bytes_consumed, &output, - &compressed); - // Compressed frame means client is ignoring the headers and misbehaves - if (compressed || r == FRAME_ERROR) { - invoke_read_callback(inspector, UV_EPROTO, nullptr); - close_connection(inspector); - bytes_consumed = 0; - } else if (r == FRAME_CLOSE) { - close_frame_received(inspector); - bytes_consumed = 0; - } else if (r == FRAME_OK && inspector->ws_state->alloc_cb - && inspector->ws_state->read_cb) { - uv_buf_t buffer; - size_t len = output.size(); - inspector->ws_state->alloc_cb( - reinterpret_cast(&inspector->tcp), - len, &buffer); - CHECK_GE(buffer.len, len); - memcpy(buffer.base, &output[0], len); - invoke_read_callback(inspector, len, &buffer); - } - return bytes_consumed; -} + private: + using Callback = void (WsHandler::*)(void); -static void prepare_buffer(uv_handle_t* stream, size_t len, uv_buf_t* buf) { - *buf = uv_buf_init(new char[len], len); -} + static void OnCloseFrameWritten(uv_write_t* req, int status) { + WriteRequest* wr = WriteRequest::from_write_req(req); + WsHandler* handler = static_cast(wr->handler); + delete wr; + Callback cb = handler->OnCloseSent; + (handler->*cb)(); + } -static void reclaim_uv_buf(InspectorSocket* inspector, const uv_buf_t* buf, - ssize_t read) { - if (read > 0) { - std::vector& buffer = inspector->buffer; - buffer.insert(buffer.end(), buf->base, buf->base + read); + void WaitForCloseReply() { + OnCloseRecieved = &WsHandler::OnEof; } - delete[] buf->base; -} -static void websockets_data_cb(uv_stream_t* stream, ssize_t nread, - const uv_buf_t* buf) { - InspectorSocket* inspector = inspector_from_stream(stream); - reclaim_uv_buf(inspector, buf, nread); - if (nread < 0 || nread == UV_EOF) { - inspector->connection_eof = true; - if (!inspector->shutting_down && inspector->ws_state->read_cb) { - inspector->ws_state->read_cb(stream, nread, nullptr); + void SendClose() { + WriteRaw(std::vector(CLOSE_FRAME, CLOSE_FRAME + sizeof(CLOSE_FRAME)), + OnCloseFrameWritten); + } + + void CloseFrameReceived() { + OnCloseSent = &WsHandler::OnEof; + SendClose(); + } + + int ParseWsFrames(const std::vector& buffer) { + int bytes_consumed = 0; + std::vector output; + bool compressed = false; + + ws_decode_result r = decode_frame_hybi17(buffer, + true /* client_frame */, + &bytes_consumed, &output, + &compressed); + // Compressed frame means client is ignoring the headers and misbehaves + if (compressed || r == FRAME_ERROR) { + OnEof(); + bytes_consumed = 0; + } else if (r == FRAME_CLOSE) { + (this->*OnCloseRecieved)(); + bytes_consumed = 0; + } else if (r == FRAME_OK) { + delegate()->OnWsFrame(output); } - if (inspector->ws_state->close_sent && - !inspector->ws_state->received_close) { - shutdown_complete(inspector); // invoke callback + return bytes_consumed; + } + + + Callback OnCloseSent; + Callback OnCloseRecieved; + bool dispose_; +}; + +// HTTP protocol +class HttpEvent { + public: + HttpEvent(const std::string& path, bool upgrade, + bool isGET, const std::string& ws_key) : path(path), + upgrade(upgrade), + isGET(isGET), + ws_key(ws_key) { } + + std::string path; + bool upgrade; + bool isGET; + std::string ws_key; + std::string current_header_; +}; + +class HttpHandler : public ProtocolHandler { + public: + explicit HttpHandler(InspectorSocket* inspector, TcpHolder::Pointer tcp) + : ProtocolHandler(inspector, std::move(tcp)), + parsing_value_(false) { + http_parser_init(&parser_, HTTP_REQUEST); + http_parser_settings_init(&parser_settings); + parser_settings.on_header_field = OnHeaderField; + parser_settings.on_header_value = OnHeaderValue; + parser_settings.on_message_complete = OnMessageComplete; + parser_settings.on_url = OnPath; + } + + void AcceptUpgrade(const std::string& accept_key) override { + char accept_string[ACCEPT_KEY_LENGTH]; + generate_accept_string(accept_key, &accept_string); + const char accept_ws_prefix[] = "HTTP/1.1 101 Switching Protocols\r\n" + "Upgrade: websocket\r\n" + "Connection: Upgrade\r\n" + "Sec-WebSocket-Accept: "; + const char accept_ws_suffix[] = "\r\n\r\n"; + std::vector reply(accept_ws_prefix, + accept_ws_prefix + sizeof(accept_ws_prefix) - 1); + reply.insert(reply.end(), accept_string, + accept_string + sizeof(accept_string)); + reply.insert(reply.end(), accept_ws_suffix, + accept_ws_suffix + sizeof(accept_ws_suffix) - 1); + if (WriteRaw(reply, WriteRequest::Cleanup) >= 0) { + inspector_->SwitchProtocol(new WsHandler(inspector_, std::move(tcp_))); + } else { + tcp_.reset(); } - } else { - #if DUMP_READS - printf("%s read %ld bytes\n", __FUNCTION__, nread); - if (nread > 0) { - dump_hex(inspector->buffer.data() + inspector->buffer.size() - nread, - nread); - } - #endif - // 2. Parse. - int processed = 0; - do { - processed = parse_ws_frames(inspector); - // 3. Fix the buffer size & length - if (processed > 0) { - remove_from_beginning(&inspector->buffer, processed); + } + + void CancelHandshake() { + const char HANDSHAKE_FAILED_RESPONSE[] = + "HTTP/1.0 400 Bad Request\r\n" + "Content-Type: text/html; charset=UTF-8\r\n\r\n" + "WebSockets request was expected\r\n"; + WriteRaw(std::vector(HANDSHAKE_FAILED_RESPONSE, + HANDSHAKE_FAILED_RESPONSE + sizeof(HANDSHAKE_FAILED_RESPONSE) - 1), + ThenCloseAndReportFailure); + } + + + void OnEof() override { + tcp_.reset(); + } + + void OnData(std::vector* data) override { + http_parser_execute(&parser_, &parser_settings, data->data(), data->size()); + data->clear(); + if (parser_.http_errno != HPE_OK) { + CancelHandshake(); + } + // Event handling may delete *this + std::vector events; + std::swap(events, events_); + for (const HttpEvent& event : events) { + bool shouldContinue = event.isGET && !event.upgrade; + if (!event.isGET) { + CancelHandshake(); + } else if (!event.upgrade) { + delegate()->OnHttpGet(event.path); + } else if (event.ws_key.empty()) { + CancelHandshake(); + } else { + delegate()->OnSocketUpgrade(event.path, event.ws_key); } - } while (processed > 0 && !inspector->buffer.empty()); + if (!shouldContinue) + return; + } } -} -int inspector_read_start(InspectorSocket* inspector, - uv_alloc_cb alloc_cb, uv_read_cb read_cb) { - CHECK(inspector->ws_mode); - CHECK(!inspector->shutting_down || read_cb == nullptr); - inspector->ws_state->close_sent = false; - inspector->ws_state->alloc_cb = alloc_cb; - inspector->ws_state->read_cb = read_cb; - int err = - uv_read_start(reinterpret_cast(&inspector->tcp), - prepare_buffer, - websockets_data_cb); - if (err < 0) { - close_connection(inspector); - } - return err; -} + void Write(const std::vector data) override { + WriteRaw(data, WriteRequest::Cleanup); + } -void inspector_read_stop(InspectorSocket* inspector) { - uv_read_stop(reinterpret_cast(&inspector->tcp)); - inspector->ws_state->alloc_cb = nullptr; - inspector->ws_state->read_cb = nullptr; -} + protected: + void Shutdown() override { + delete this; + } -static void generate_accept_string(const std::string& client_key, - char (*buffer)[ACCEPT_KEY_LENGTH]) { - // Magic string from websockets spec. - static const char ws_magic[] = "258EAFA5-E914-47DA-95CA-C5AB0DC85B11"; - std::string input(client_key + ws_magic); - char hash[SHA_DIGEST_LENGTH]; - SHA1(reinterpret_cast(&input[0]), input.size(), - reinterpret_cast(hash)); - node::base64_encode(hash, sizeof(hash), *buffer, sizeof(*buffer)); -} + private: + static void ThenCloseAndReportFailure(uv_write_t* req, int status) { + ProtocolHandler* handler = WriteRequest::from_write_req(req)->handler; + WriteRequest::Cleanup(req, status); + handler->inspector()->SwitchProtocol(nullptr); + } -static int header_value_cb(http_parser* parser, const char* at, size_t length) { - static const char SEC_WEBSOCKET_KEY_HEADER[] = "Sec-WebSocket-Key"; - auto inspector = static_cast(parser->data); - auto state = inspector->http_parsing_state; - state->parsing_value = true; - if (state->current_header.size() == sizeof(SEC_WEBSOCKET_KEY_HEADER) - 1 && - node::StringEqualNoCaseN(state->current_header.data(), - SEC_WEBSOCKET_KEY_HEADER, - sizeof(SEC_WEBSOCKET_KEY_HEADER) - 1)) { - state->ws_key.append(at, length); - } - return 0; -} + static int OnHeaderValue(http_parser* parser, const char* at, size_t length) { + static const char SEC_WEBSOCKET_KEY_HEADER[] = "Sec-WebSocket-Key"; + HttpHandler* handler = From(parser); + handler->parsing_value_ = true; + if (handler->current_header_.size() == + sizeof(SEC_WEBSOCKET_KEY_HEADER) - 1 && + node::StringEqualNoCaseN(handler->current_header_.data(), + SEC_WEBSOCKET_KEY_HEADER, + sizeof(SEC_WEBSOCKET_KEY_HEADER) - 1)) { + handler->ws_key_.append(at, length); + } + return 0; + } -static int header_field_cb(http_parser* parser, const char* at, size_t length) { - auto inspector = static_cast(parser->data); - auto state = inspector->http_parsing_state; - if (state->parsing_value) { - state->parsing_value = false; - state->current_header.clear(); + static int OnHeaderField(http_parser* parser, const char* at, size_t length) { + HttpHandler* handler = From(parser); + if (handler->parsing_value_) { + handler->parsing_value_ = false; + handler->current_header_.clear(); + } + handler->current_header_.append(at, length); + return 0; } - state->current_header.append(at, length); - return 0; -} -static int path_cb(http_parser* parser, const char* at, size_t length) { - auto inspector = static_cast(parser->data); - auto state = inspector->http_parsing_state; - state->path.append(at, length); - return 0; -} + static int OnPath(http_parser* parser, const char* at, size_t length) { + HttpHandler* handler = From(parser); + handler->path_.append(at, length); + return 0; + } + + static HttpHandler* From(http_parser* parser) { + return node::ContainerOf(&HttpHandler::parser_, parser); + } + + static int OnMessageComplete(http_parser* parser) { + // Event needs to be fired after the parser is done. + HttpHandler* handler = From(parser); + handler->events_.push_back(HttpEvent(handler->path_, parser->upgrade, + parser->method == HTTP_GET, + handler->ws_key_)); + handler->path_ = ""; + handler->ws_key_ = ""; + handler->parsing_value_ = false; + handler->current_header_ = ""; + + return 0; + } + + bool parsing_value_; + http_parser parser_; + http_parser_settings parser_settings; + std::vector events_; + std::string current_header_; + std::string ws_key_; + std::string path_; +}; + +} // namespace -static void handshake_complete(InspectorSocket* inspector) { - uv_read_stop(reinterpret_cast(&inspector->tcp)); - handshake_cb callback = inspector->http_parsing_state->callback; - inspector->ws_state = new ws_state_s(); - inspector->ws_mode = true; - callback(inspector, kInspectorHandshakeUpgraded, - inspector->http_parsing_state->path); +// Any protocol +ProtocolHandler::ProtocolHandler(InspectorSocket* inspector, + TcpHolder::Pointer tcp) + : inspector_(inspector), tcp_(std::move(tcp)) { + CHECK_NE(nullptr, tcp_); + tcp_->SetHandler(this); } -static void cleanup_http_parsing_state(InspectorSocket* inspector) { - delete inspector->http_parsing_state; - inspector->http_parsing_state = nullptr; +int ProtocolHandler::WriteRaw(const std::vector& buffer, + uv_write_cb write_cb) { + return tcp_->WriteRaw(buffer, write_cb); } -static void report_handshake_failure_cb(uv_handle_t* handle) { - dispose_inspector(handle); - InspectorSocket* inspector = inspector_from_stream(handle); - handshake_cb cb = inspector->http_parsing_state->callback; - cleanup_http_parsing_state(inspector); - cb(inspector, kInspectorHandshakeFailed, std::string()); +InspectorSocket::Delegate* ProtocolHandler::delegate() { + return tcp_->delegate(); } -static void close_and_report_handshake_failure(InspectorSocket* inspector) { - uv_handle_t* socket = reinterpret_cast(&inspector->tcp); - if (uv_is_closing(socket)) { - report_handshake_failure_cb(socket); +std::string ProtocolHandler::GetHost() { + char ip[INET6_ADDRSTRLEN]; + sockaddr_storage addr; + int len = sizeof(addr); + int err = uv_tcp_getsockname(tcp_->tcp(), + reinterpret_cast(&addr), + &len); + if (err != 0) + return ""; + if (addr.ss_family == AF_INET6) { + const sockaddr_in6* v6 = reinterpret_cast(&addr); + err = uv_ip6_name(v6, ip, sizeof(ip)); } else { - uv_read_stop(reinterpret_cast(socket)); - uv_close(socket, report_handshake_failure_cb); + const sockaddr_in* v4 = reinterpret_cast(&addr); + err = uv_ip4_name(v4, ip, sizeof(ip)); + } + if (err != 0) + return ""; + return ip; +} + +// RAII uv_tcp_t wrapper +TcpHolder::TcpHolder(InspectorSocket::DelegatePointer delegate) + : tcp_(), + delegate_(std::move(delegate)), + handler_(nullptr) { } + +// static +TcpHolder::Pointer TcpHolder::Accept( + uv_stream_t* server, + InspectorSocket::DelegatePointer delegate) { + TcpHolder* result = new TcpHolder(std::move(delegate)); + uv_stream_t* tcp = reinterpret_cast(&result->tcp_); + int err = uv_tcp_init(server->loop, &result->tcp_); + if (err == 0) { + err = uv_accept(server, tcp); + } + if (err == 0) { + err = uv_read_start(tcp, allocate_buffer, OnDataReceivedCb); + } + if (err == 0) { + return { result, DisconnectAndDispose }; + } else { + fprintf(stderr, "[%s:%d@%s]\n", __FILE__, __LINE__, __FUNCTION__); + + delete result; + return { nullptr, nullptr }; } } -static void then_close_and_report_failure(uv_write_t* req, int status) { - InspectorSocket* inspector = WriteRequest::from_write_req(req)->inspector; - write_request_cleanup(req, status); - close_and_report_handshake_failure(inspector); +void TcpHolder::SetHandler(ProtocolHandler* handler) { + handler_ = handler; } -static void handshake_failed(InspectorSocket* inspector) { - const char HANDSHAKE_FAILED_RESPONSE[] = - "HTTP/1.0 400 Bad Request\r\n" - "Content-Type: text/html; charset=UTF-8\r\n\r\n" - "WebSockets request was expected\r\n"; - write_to_client(inspector, HANDSHAKE_FAILED_RESPONSE, - sizeof(HANDSHAKE_FAILED_RESPONSE) - 1, - then_close_and_report_failure); +int TcpHolder::WriteRaw(const std::vector& buffer, uv_write_cb write_cb) { +#if DUMP_WRITES + printf("%s (%ld bytes):\n", __FUNCTION__, buffer.size()); + dump_hex(buffer.data(), buffer.size()); + printf("\n"); +#endif + + // Freed in write_request_cleanup + WriteRequest* wr = new WriteRequest(handler_, buffer); + uv_stream_t* stream = reinterpret_cast(&tcp_); + int err = uv_write(&wr->req, stream, &wr->buf, 1, write_cb); + if (err < 0) + delete wr; + return err < 0; } -// init_handshake references message_complete_cb -static void init_handshake(InspectorSocket* socket); - -static int message_complete_cb(http_parser* parser) { - InspectorSocket* inspector = static_cast(parser->data); - struct http_parsing_state_s* state = inspector->http_parsing_state; - if (parser->method != HTTP_GET) { - handshake_failed(inspector); - } else if (!parser->upgrade) { - if (state->callback(inspector, kInspectorHandshakeHttpGet, state->path)) { - init_handshake(inspector); - } else { - handshake_failed(inspector); - } - } else if (state->ws_key.empty()) { - handshake_failed(inspector); - } else if (state->callback(inspector, kInspectorHandshakeUpgrading, - state->path)) { - char accept_string[ACCEPT_KEY_LENGTH]; - generate_accept_string(state->ws_key, &accept_string); - const char accept_ws_prefix[] = "HTTP/1.1 101 Switching Protocols\r\n" - "Upgrade: websocket\r\n" - "Connection: Upgrade\r\n" - "Sec-WebSocket-Accept: "; - const char accept_ws_suffix[] = "\r\n\r\n"; - std::string reply(accept_ws_prefix, sizeof(accept_ws_prefix) - 1); - reply.append(accept_string, sizeof(accept_string)); - reply.append(accept_ws_suffix, sizeof(accept_ws_suffix) - 1); - if (write_to_client(inspector, &reply[0], reply.size()) >= 0) { - handshake_complete(inspector); - inspector->http_parsing_state->done = true; - } else { - close_and_report_handshake_failure(inspector); - } - } else { - handshake_failed(inspector); - } - return 0; +InspectorSocket::Delegate* TcpHolder::delegate() { + return delegate_.get(); +} + +// static +void TcpHolder::OnClosed(uv_handle_t* handle) { + delete From(handle); } -static void data_received_cb(uv_stream_t* tcp, ssize_t nread, - const uv_buf_t* buf) { +void TcpHolder::OnDataReceivedCb(uv_stream_t* tcp, ssize_t nread, + const uv_buf_t* buf) { #if DUMP_READS if (nread >= 0) { printf("%s (%ld bytes)\n", __FUNCTION__, nread); @@ -522,107 +667,65 @@ static void data_received_cb(uv_stream_t* tcp, ssize_t nread, printf("[%s:%d] %s\n", __FUNCTION__, __LINE__, uv_err_name(nread)); } #endif - InspectorSocket* inspector = inspector_from_stream(tcp); - reclaim_uv_buf(inspector, buf, nread); + TcpHolder* holder = From(tcp); + holder->ReclaimUvBuf(buf, nread); if (nread < 0 || nread == UV_EOF) { - close_and_report_handshake_failure(inspector); + holder->handler_->OnEof(); } else { - http_parsing_state_s* state = inspector->http_parsing_state; - http_parser* parser = &state->parser; - http_parser_execute(parser, &state->parser_settings, - inspector->buffer.data(), nread); - remove_from_beginning(&inspector->buffer, nread); - if (parser->http_errno != HPE_OK) { - handshake_failed(inspector); - } - if (inspector->http_parsing_state->done) { - cleanup_http_parsing_state(inspector); - } + holder->handler_->OnData(&holder->buffer); } } -static void init_handshake(InspectorSocket* socket) { - http_parsing_state_s* state = socket->http_parsing_state; - CHECK_NE(state, nullptr); - state->current_header.clear(); - state->ws_key.clear(); - state->path.clear(); - state->done = false; - http_parser_init(&state->parser, HTTP_REQUEST); - state->parser.data = socket; - http_parser_settings* settings = &state->parser_settings; - http_parser_settings_init(settings); - settings->on_header_field = header_field_cb; - settings->on_header_value = header_value_cb; - settings->on_message_complete = message_complete_cb; - settings->on_url = path_cb; +// static +void TcpHolder::DisconnectAndDispose(TcpHolder* holder) { + uv_handle_t* handle = reinterpret_cast(&holder->tcp_); + uv_close(handle, OnClosed); } -int inspector_accept(uv_stream_t* server, InspectorSocket* socket, - handshake_cb callback) { - CHECK_NE(callback, nullptr); - CHECK_EQ(socket->http_parsing_state, nullptr); - - socket->http_parsing_state = new http_parsing_state_s(); - uv_stream_t* tcp = reinterpret_cast(&socket->tcp); - int err = uv_tcp_init(server->loop, &socket->tcp); - - if (err == 0) { - err = uv_accept(server, tcp); - } - if (err == 0) { - init_handshake(socket); - socket->http_parsing_state->callback = callback; - err = uv_read_start(tcp, prepare_buffer, - data_received_cb); - } - if (err != 0) { - uv_close(reinterpret_cast(tcp), nullptr); +void TcpHolder::ReclaimUvBuf(const uv_buf_t* buf, ssize_t read) { + if (read > 0) { + buffer.insert(buffer.end(), buf->base, buf->base + read); } - return err; + delete[] buf->base; } -void inspector_write(InspectorSocket* inspector, const char* data, - size_t len) { - if (inspector->ws_mode) { - std::vector output = encode_frame_hybi17(data, len); - write_to_client(inspector, &output[0], output.size()); +// Public interface +InspectorSocket::InspectorSocket() + : protocol_handler_(nullptr, ProtocolHandler::Shutdown) { } + +InspectorSocket::~InspectorSocket() = default; + +// static +InspectorSocket::Pointer InspectorSocket::Accept(uv_stream_t* server, + DelegatePointer delegate) { + auto tcp = TcpHolder::Accept(server, std::move(delegate)); + if (tcp) { + InspectorSocket* inspector = new InspectorSocket(); + inspector->SwitchProtocol(new HttpHandler(inspector, std::move(tcp))); + return InspectorSocket::Pointer(inspector); } else { - write_to_client(inspector, data, len); + return InspectorSocket::Pointer(nullptr); } } -void inspector_close(InspectorSocket* inspector, - inspector_cb callback) { - // libuv throws assertions when closing stream that's already closed - we - // need to do the same. - CHECK(!uv_is_closing(reinterpret_cast(&inspector->tcp))); - CHECK(!inspector->shutting_down); - inspector->shutting_down = true; - inspector->ws_state->close_cb = callback; - if (inspector->connection_eof) { - close_connection(inspector); - } else { - inspector_read_stop(inspector); - write_to_client(inspector, CLOSE_FRAME, sizeof(CLOSE_FRAME), - on_close_frame_written); - inspector_read_start(inspector, nullptr, nullptr); - } +void InspectorSocket::AcceptUpgrade(const std::string& ws_key) { + protocol_handler_->AcceptUpgrade(ws_key); +} + +void InspectorSocket::CancelHandshake() { + protocol_handler_->CancelHandshake(); +} + +std::string InspectorSocket::GetHost() { + return protocol_handler_->GetHost(); } -bool inspector_is_active(const InspectorSocket* inspector) { - const uv_handle_t* tcp = - reinterpret_cast(&inspector->tcp); - return !inspector->shutting_down && !uv_is_closing(tcp); +void InspectorSocket::SwitchProtocol(ProtocolHandler* handler) { + protocol_handler_.reset(std::move(handler)); } -void InspectorSocket::reinit() { - http_parsing_state = nullptr; - ws_state = nullptr; - buffer.clear(); - ws_mode = false; - shutting_down = false; - connection_eof = false; +void InspectorSocket::Write(const char* data, size_t len) { + protocol_handler_->Write(std::vector(data, data + len)); } } // namespace inspector diff --git a/src/inspector_socket.h b/src/inspector_socket.h index f93150d6f9a..1a3411435ee 100644 --- a/src/inspector_socket.h +++ b/src/inspector_socket.h @@ -1,7 +1,6 @@ #ifndef SRC_INSPECTOR_SOCKET_H_ #define SRC_INSPECTOR_SOCKET_H_ -#include "http_parser.h" #include "util-inl.h" #include "uv.h" @@ -11,88 +10,41 @@ namespace node { namespace inspector { -enum inspector_handshake_event { - kInspectorHandshakeUpgrading, - kInspectorHandshakeUpgraded, - kInspectorHandshakeHttpGet, - kInspectorHandshakeFailed -}; - -class InspectorSocket; - -typedef void (*inspector_cb)(InspectorSocket*, int); -// Notifies as handshake is progressing. Returning false as a response to -// kInspectorHandshakeUpgrading or kInspectorHandshakeHttpGet event will abort -// the connection. inspector_write can be used from the callback. -typedef bool (*handshake_cb)(InspectorSocket*, - enum inspector_handshake_event state, - const std::string& path); - -struct http_parsing_state_s { - http_parser parser; - http_parser_settings parser_settings; - handshake_cb callback; - bool done; - bool parsing_value; - std::string ws_key; - std::string path; - std::string current_header; -}; - -struct ws_state_s { - uv_alloc_cb alloc_cb; - uv_read_cb read_cb; - inspector_cb close_cb; - bool close_sent; - bool received_close; -}; +class ProtocolHandler; // HTTP Wrapper around a uv_tcp_t class InspectorSocket { public: - InspectorSocket() : data(nullptr), http_parsing_state(nullptr), - ws_state(nullptr), buffer(0), ws_mode(false), - shutting_down(false), connection_eof(false) { } - void reinit(); - void* data; - struct http_parsing_state_s* http_parsing_state; - struct ws_state_s* ws_state; - std::vector buffer; - uv_tcp_t tcp; - bool ws_mode; - bool shutting_down; - bool connection_eof; + class Delegate { + public: + virtual void OnHttpGet(const std::string& path) = 0; + virtual void OnSocketUpgrade(const std::string& path, + const std::string& accept_key) = 0; + virtual void OnWsFrame(const std::vector& frame) = 0; + virtual ~Delegate() {} + }; - private: - DISALLOW_COPY_AND_ASSIGN(InspectorSocket); -}; + using DelegatePointer = std::unique_ptr; + using Pointer = std::unique_ptr; + + static Pointer Accept(uv_stream_t* server, DelegatePointer delegate); -int inspector_accept(uv_stream_t* server, InspectorSocket* inspector, - handshake_cb callback); + ~InspectorSocket(); -void inspector_close(InspectorSocket* inspector, - inspector_cb callback); + void AcceptUpgrade(const std::string& accept_key); + void CancelHandshake(); + void Write(const char* data, size_t len); + void SwitchProtocol(ProtocolHandler* handler); + std::string GetHost(); -// Callbacks will receive stream handles. Use inspector_from_stream to get -// InspectorSocket* from the stream handle. -int inspector_read_start(InspectorSocket* inspector, uv_alloc_cb, - uv_read_cb); -void inspector_read_stop(InspectorSocket* inspector); -void inspector_write(InspectorSocket* inspector, - const char* data, size_t len); -bool inspector_is_active(const InspectorSocket* inspector); + private: + InspectorSocket(); -inline InspectorSocket* inspector_from_stream(uv_tcp_t* stream) { - return node::ContainerOf(&InspectorSocket::tcp, stream); -} + std::unique_ptr protocol_handler_; -inline InspectorSocket* inspector_from_stream(uv_stream_t* stream) { - return inspector_from_stream(reinterpret_cast(stream)); -} + DISALLOW_COPY_AND_ASSIGN(InspectorSocket); +}; -inline InspectorSocket* inspector_from_stream(uv_handle_t* stream) { - return inspector_from_stream(reinterpret_cast(stream)); -} } // namespace inspector } // namespace node diff --git a/src/inspector_socket_server.cc b/src/inspector_socket_server.cc index 958c41a654a..bf114251cb1 100644 --- a/src/inspector_socket_server.cc +++ b/src/inspector_socket_server.cc @@ -33,7 +33,6 @@ std::string FormatWsAddress(const std::string& host, int port, return url.str(); } - namespace { static const uint8_t PROTOCOL_JSON[] = { @@ -114,8 +113,8 @@ void SendHttpResponse(InspectorSocket* socket, const std::string& response) { "\r\n"; char header[sizeof(HEADERS) + 20]; int header_len = snprintf(header, sizeof(header), HEADERS, response.size()); - inspector_write(socket, header, header_len); - inspector_write(socket, response.data(), response.size()); + socket->Write(header, header_len); + socket->Write(response.data(), response.size()); } void SendVersionResponse(InspectorSocket* socket) { @@ -145,28 +144,6 @@ void SendProtocolJson(InspectorSocket* socket) { CHECK_EQ(Z_OK, inflateEnd(&strm)); SendHttpResponse(socket, data); } - -int GetSocketHost(uv_tcp_t* socket, std::string* out_host) { - char ip[INET6_ADDRSTRLEN]; - sockaddr_storage addr; - int len = sizeof(addr); - int err = uv_tcp_getsockname(socket, - reinterpret_cast(&addr), - &len); - if (err != 0) - return err; - if (addr.ss_family == AF_INET6) { - const sockaddr_in6* v6 = reinterpret_cast(&addr); - err = uv_ip6_name(v6, ip, sizeof(ip)); - } else { - const sockaddr_in* v4 = reinterpret_cast(&addr); - err = uv_ip4_name(v4, ip, sizeof(ip)); - } - if (err != 0) - return err; - *out_host = ip; - return err; -} } // namespace @@ -209,46 +186,58 @@ class Closer { class SocketSession { public: - static int Accept(InspectorSocketServer* server, int server_port, - uv_stream_t* server_socket); + SocketSession(InspectorSocketServer* server, int id, int server_port); + void Close() { + ws_socket_.reset(); + } void Send(const std::string& message); - void Close(); - + void Own(InspectorSocket::Pointer ws_socket) { + ws_socket_ = std::move(ws_socket); + } int id() const { return id_; } - bool IsForTarget(const std::string& target_id) const { - return target_id_ == target_id; + int server_port() { + return server_port_; } - static int ServerPortForClient(InspectorSocket* client) { - return From(client)->server_port_; + InspectorSocket* ws_socket() { + return ws_socket_.get(); } - - private: - SocketSession(InspectorSocketServer* server, int server_port); - static SocketSession* From(InspectorSocket* socket) { - return node::ContainerOf(&SocketSession::socket_, socket); + void set_ws_key(const std::string& ws_key) { + ws_key_ = ws_key; + } + void Accept() { + ws_socket_->AcceptUpgrade(ws_key_); + } + void Decline() { + ws_socket_->CancelHandshake(); } - enum class State { kHttp, kWebSocket, kClosing, kEOF, kDeclined }; - static bool HandshakeCallback(InspectorSocket* socket, - enum inspector_handshake_event state, - const std::string& path); - static void ReadCallback(uv_stream_t* stream, ssize_t read, - const uv_buf_t* buf); - static void CloseCallback(InspectorSocket* socket, int code); + class Delegate : public InspectorSocket::Delegate { + public: + Delegate(InspectorSocketServer* server, int session_id) + : server_(server), session_id_(session_id) { } + ~Delegate() { + server_->SessionTerminated(session_id_); + } + void OnHttpGet(const std::string& path) override; + void OnSocketUpgrade(const std::string& path, + const std::string& ws_key) override; + void OnWsFrame(const std::vector& data) override; + + private: + SocketSession* Session() { + return server_->Session(session_id_); + } - void FrontendConnected(); - void SetDeclined() { state_ = State::kDeclined; } - void SetTargetId(const std::string& target_id) { - CHECK(target_id_.empty()); - target_id_ = target_id; - } + InspectorSocketServer* server_; + int session_id_; + }; + private: const int id_; - InspectorSocket socket_; + InspectorSocket::Pointer ws_socket_; InspectorSocketServer* server_; - std::string target_id_; - State state_; const int server_port_; + std::string ws_key_; }; class ServerSocket { @@ -269,7 +258,6 @@ class ServerSocket { return node::ContainerOf(&ServerSocket::tcp_socket_, reinterpret_cast(socket)); } - static void SocketConnectedCallback(uv_stream_t* tcp_socket, int status); static void SocketClosedCallback(uv_handle_t* tcp_socket); static void FreeOnCloseCallback(uv_handle_t* tcp_socket_) { @@ -296,41 +284,57 @@ InspectorSocketServer::InspectorSocketServer(SocketServerDelegate* delegate, state_ = ServerState::kNew; } -bool InspectorSocketServer::SessionStarted(SocketSession* session, - const std::string& id) { - if (TargetExists(id) && delegate_->StartSession(session->id(), id)) { - connected_sessions_[session->id()] = session; - return true; - } else { - return false; +InspectorSocketServer::~InspectorSocketServer() = default; + +SocketSession* InspectorSocketServer::Session(int session_id) { + auto it = connected_sessions_.find(session_id); + return it == connected_sessions_.end() ? nullptr : it->second.second.get(); +} + +void InspectorSocketServer::SessionStarted(int session_id, + const std::string& id, + const std::string& ws_key) { + SocketSession* session = Session(session_id); + if (!TargetExists(id)) { + Session(session_id)->Decline(); + return; } + connected_sessions_[session_id].first = id; + session->set_ws_key(ws_key); + delegate_->StartSession(session_id, id); } -void InspectorSocketServer::SessionTerminated(SocketSession* session) { - int id = session->id(); - if (connected_sessions_.erase(id) != 0) { - delegate_->EndSession(id); - if (connected_sessions_.empty()) { - if (state_ == ServerState::kRunning && !server_sockets_.empty()) { - PrintDebuggerReadyMessage(host_, server_sockets_[0]->port(), - delegate_->GetTargetIds(), out_); - } - if (state_ == ServerState::kStopped) { - delegate_->ServerDone(); - } +void InspectorSocketServer::SessionTerminated(int session_id) { + if (Session(session_id) == nullptr) { + return; + } + bool was_attached = connected_sessions_[session_id].first != ""; + if (was_attached) { + delegate_->EndSession(session_id); + } + connected_sessions_.erase(session_id); + if (connected_sessions_.empty()) { + if (was_attached && state_ == ServerState::kRunning + && !server_sockets_.empty()) { + PrintDebuggerReadyMessage(host_, server_sockets_[0]->port(), + delegate_->GetTargetIds(), out_); + } + if (state_ == ServerState::kStopped) { + delegate_->ServerDone(); } } - delete session; } -bool InspectorSocketServer::HandleGetRequest(InspectorSocket* socket, +bool InspectorSocketServer::HandleGetRequest(int session_id, const std::string& path) { + SocketSession* session = Session(session_id); + InspectorSocket* socket = session->ws_socket(); const char* command = MatchPathSegment(path.c_str(), "/json"); if (command == nullptr) return false; if (MatchPathSegment(command, "list") || command[0] == '\0') { - SendListResponse(socket); + SendListResponse(socket, session); return true; } else if (MatchPathSegment(command, "protocol")) { SendProtocolJson(socket); @@ -348,7 +352,8 @@ bool InspectorSocketServer::HandleGetRequest(InspectorSocket* socket, return false; } -void InspectorSocketServer::SendListResponse(InspectorSocket* socket) { +void InspectorSocketServer::SendListResponse(InspectorSocket* socket, + SocketSession* session) { std::vector> response; for (const std::string& id : delegate_->GetTargetIds()) { response.push_back(std::map()); @@ -366,15 +371,14 @@ void InspectorSocketServer::SendListResponse(InspectorSocket* socket) { bool connected = false; for (const auto& session : connected_sessions_) { - if (session.second->IsForTarget(id)) { + if (session.second.first == id) { connected = true; break; } } if (!connected) { - std::string host; - int port = SocketSession::ServerPortForClient(socket); - GetSocketHost(&socket->tcp, &host); + std::string host = socket->GetHost(); + int port = session->server_port(); std::ostringstream frontend_url; frontend_url << "chrome-devtools://devtools/bundled"; frontend_url << "/inspector.html?experiments=true&v8only=true&ws="; @@ -444,9 +448,8 @@ void InspectorSocketServer::Stop(ServerCallback cb) { } void InspectorSocketServer::TerminateConnections() { - for (const auto& session : connected_sessions_) { - session.second->Close(); - } + for (const auto& key_value : connected_sessions_) + key_value.second.second->Close(); } bool InspectorSocketServer::TargetExists(const std::string& id) { @@ -455,13 +458,6 @@ bool InspectorSocketServer::TargetExists(const std::string& id) { return found != target_ids.end(); } -void InspectorSocketServer::Send(int session_id, const std::string& message) { - auto session_iterator = connected_sessions_.find(session_id); - if (session_iterator != connected_sessions_.end()) { - session_iterator->second->Send(message); - } -} - void InspectorSocketServer::ServerSocketListening(ServerSocket* server_socket) { server_sockets_.push_back(server_socket); } @@ -491,92 +487,73 @@ int InspectorSocketServer::Port() const { return port_; } -// InspectorSession tracking -SocketSession::SocketSession(InspectorSocketServer* server, int server_port) - : id_(server->GenerateSessionId()), - server_(server), - state_(State::kHttp), - server_port_(server_port) { } +void InspectorSocketServer::Accept(int server_port, + uv_stream_t* server_socket) { + std::unique_ptr session( + new SocketSession(this, next_session_id_++, server_port)); + + InspectorSocket::DelegatePointer delegate = + InspectorSocket::DelegatePointer( + new SocketSession::Delegate(this, session->id())); -void SocketSession::Close() { - CHECK_NE(state_, State::kClosing); - state_ = State::kClosing; - inspector_close(&socket_, CloseCallback); + InspectorSocket::Pointer inspector = + InspectorSocket::Accept(server_socket, std::move(delegate)); + if (inspector) { + session->Own(std::move(inspector)); + connected_sessions_[session->id()].second = std::move(session); + } } -// static -int SocketSession::Accept(InspectorSocketServer* server, int server_port, - uv_stream_t* server_socket) { - // Memory is freed when the socket closes. - SocketSession* session = new SocketSession(server, server_port); - int err = inspector_accept(server_socket, &session->socket_, - HandshakeCallback); - if (err != 0) { - delete session; +void InspectorSocketServer::AcceptSession(int session_id) { + SocketSession* session = Session(session_id); + if (session == nullptr) { + delegate_->EndSession(session_id); + } else { + session->Accept(); } - return err; } -// static -bool SocketSession::HandshakeCallback(InspectorSocket* socket, - inspector_handshake_event event, - const std::string& path) { - SocketSession* session = SocketSession::From(socket); - InspectorSocketServer* server = session->server_; - const std::string& id = path.empty() ? path : path.substr(1); - switch (event) { - case kInspectorHandshakeHttpGet: - return server->HandleGetRequest(socket, path); - case kInspectorHandshakeUpgrading: - if (server->SessionStarted(session, id)) { - session->SetTargetId(id); - return true; - } else { - session->SetDeclined(); - return false; - } - case kInspectorHandshakeUpgraded: - session->FrontendConnected(); - return true; - case kInspectorHandshakeFailed: - server->SessionTerminated(session); - return false; - default: - UNREACHABLE(); - return false; +void InspectorSocketServer::DeclineSession(int session_id) { + auto it = connected_sessions_.find(session_id); + if (it != connected_sessions_.end()) { + it->second.first.clear(); + it->second.second->Decline(); } } -// static -void SocketSession::CloseCallback(InspectorSocket* socket, int code) { - SocketSession* session = SocketSession::From(socket); - CHECK_EQ(State::kClosing, session->state_); - session->server_->SessionTerminated(session); +void InspectorSocketServer::Send(int session_id, const std::string& message) { + SocketSession* session = Session(session_id); + if (session != nullptr) { + session->Send(message); + } +} + +// InspectorSession tracking +SocketSession::SocketSession(InspectorSocketServer* server, int id, + int server_port) + : id_(id), + server_(server), + server_port_(server_port) { } + + +void SocketSession::Send(const std::string& message) { + ws_socket_->Write(message.data(), message.length()); } -void SocketSession::FrontendConnected() { - CHECK_EQ(State::kHttp, state_); - state_ = State::kWebSocket; - inspector_read_start(&socket_, OnBufferAlloc, ReadCallback); +void SocketSession::Delegate::OnHttpGet(const std::string& path) { + if (!server_->HandleGetRequest(session_id_, path)) + Session()->ws_socket()->CancelHandshake(); } -// static -void SocketSession::ReadCallback(uv_stream_t* stream, ssize_t read, - const uv_buf_t* buf) { - InspectorSocket* socket = inspector_from_stream(stream); - SocketSession* session = SocketSession::From(socket); - if (read > 0) { - session->server_->MessageReceived(session->id_, - std::string(buf->base, read)); - } else { - session->Close(); - } - if (buf != nullptr && buf->base != nullptr) - delete[] buf->base; +void SocketSession::Delegate::OnSocketUpgrade(const std::string& path, + const std::string& ws_key) { + std::string id = path.empty() ? path : path.substr(1); + server_->SessionStarted(session_id_, id, ws_key); } -void SocketSession::Send(const std::string& message) { - inspector_write(&socket_, message.data(), message.length()); +void SocketSession::Delegate::OnWsFrame(const std::vector& data) { + server_->MessageReceived(session_id_, + std::string(data.data(), data.size())); } // ServerSocket implementation @@ -624,8 +601,7 @@ void ServerSocket::SocketConnectedCallback(uv_stream_t* tcp_socket, if (status == 0) { ServerSocket* server_socket = ServerSocket::FromTcpSocket(tcp_socket); // Memory is freed when the socket closes. - SocketSession::Accept(server_socket->server_, server_socket->port_, - tcp_socket); + server_socket->server_->Accept(server_socket->port_, tcp_socket); } } diff --git a/src/inspector_socket_server.h b/src/inspector_socket_server.h index 16b047da333..b193e33a46d 100644 --- a/src/inspector_socket_server.h +++ b/src/inspector_socket_server.h @@ -22,7 +22,7 @@ class ServerSocket; class SocketServerDelegate { public: - virtual bool StartSession(int session_id, const std::string& target_id) = 0; + virtual void StartSession(int session_id, const std::string& target_id) = 0; virtual void EndSession(int session_id) = 0; virtual void MessageReceived(int session_id, const std::string& message) = 0; virtual std::vector GetTargetIds() = 0; @@ -34,8 +34,6 @@ class SocketServerDelegate { // HTTP Server, writes messages requested as TransportActions, and responds // to HTTP requests and WS upgrades. - - class InspectorSocketServer { public: using ServerCallback = void (*)(InspectorSocketServer*); @@ -44,6 +42,8 @@ class InspectorSocketServer { const std::string& host, int port, FILE* out = stderr); + ~InspectorSocketServer(); + // Start listening on host/port bool Start(); @@ -54,6 +54,10 @@ class InspectorSocketServer { void Send(int session_id, const std::string& message); // kKill void TerminateConnections(); + // kAcceptSession + void AcceptSession(int session_id); + // kDeclineSession + void DeclineSession(int session_id); int Port() const; @@ -62,19 +66,18 @@ class InspectorSocketServer { void ServerSocketClosed(ServerSocket* server_socket); // Session connection lifecycle - bool HandleGetRequest(InspectorSocket* socket, const std::string& path); - bool SessionStarted(SocketSession* session, const std::string& id); - void SessionTerminated(SocketSession* session); + void Accept(int server_port, uv_stream_t* server_socket); + bool HandleGetRequest(int session_id, const std::string& path); + void SessionStarted(int session_id, const std::string& target_id, + const std::string& ws_id); + void SessionTerminated(int session_id); void MessageReceived(int session_id, const std::string& message) { delegate_->MessageReceived(session_id, message); } - - int GenerateSessionId() { - return next_session_id_++; - } + SocketSession* Session(int session_id); private: - void SendListResponse(InspectorSocket* socket); + void SendListResponse(InspectorSocket* socket, SocketSession* session); bool TargetExists(const std::string& id); enum class ServerState {kNew, kRunning, kStopping, kStopped}; @@ -85,7 +88,8 @@ class InspectorSocketServer { std::string path_; std::vector server_sockets_; Closer* closer_; - std::map connected_sessions_; + std::map>> + connected_sessions_; int next_session_id_; FILE* out_; ServerState state_; diff --git a/src/node.cc b/src/node.cc index ed2f0c14ff3..3af764d1fba 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2006,7 +2006,7 @@ static void InitGroups(const FunctionCallbackInfo& args) { static void WaitForInspectorDisconnect(Environment* env) { #if HAVE_INSPECTOR - if (env->inspector_agent()->IsConnected()) { + if (env->inspector_agent()->delegate() != nullptr) { // Restore signal dispositions, the app is done and is no longer // capable of handling signals. #if defined(__POSIX__) && !defined(NODE_SHARED_MODE) diff --git a/test/cctest/test_inspector_socket.cc b/test/cctest/test_inspector_socket.cc index d1f5b4a98ac..943109b8a59 100644 --- a/test/cctest/test_inspector_socket.cc +++ b/test/cctest/test_inspector_socket.cc @@ -1,57 +1,17 @@ #include "inspector_socket.h" #include "gtest/gtest.h" +#include + #define PORT 9444 namespace { using node::inspector::InspectorSocket; -using node::inspector::inspector_from_stream; -using node::inspector::inspector_handshake_event; -using node::inspector::kInspectorHandshakeFailed; -using node::inspector::kInspectorHandshakeHttpGet; -using node::inspector::kInspectorHandshakeUpgraded; -using node::inspector::kInspectorHandshakeUpgrading; static const int MAX_LOOP_ITERATIONS = 10000; -#define SPIN_WHILE(condition) \ - { \ - Timeout timeout(&loop); \ - while ((condition) && !timeout.timed_out) { \ - uv_run(&loop, UV_RUN_NOWAIT); \ - } \ - ASSERT_FALSE((condition)); \ - } - -static bool connected = false; -static bool inspector_ready = false; -static int handshake_events = 0; -static enum inspector_handshake_event last_event = kInspectorHandshakeHttpGet; static uv_loop_t loop; -static uv_tcp_t server, client_socket; -static InspectorSocket inspector; -static std::string last_path; // NOLINT(runtime/string) -static void (*handshake_delegate)(enum inspector_handshake_event state, - const std::string& path, - bool* should_continue); -static const char SERVER_CLOSE_FRAME[] = {'\x88', '\x00'}; - - -struct read_expects { - const char* expected; - size_t expected_len; - size_t pos; - bool read_expected; - bool callback_called; -}; - -static const char HANDSHAKE_REQ[] = "GET /ws/path HTTP/1.1\r\n" - "Host: localhost:9222\r\n" - "Upgrade: websocket\r\n" - "Connection: Upgrade\r\n" - "Sec-WebSocket-Key: aaa==\r\n" - "Sec-WebSocket-Version: 13\r\n\r\n"; class Timeout { public: @@ -86,16 +46,176 @@ class Timeout { uv_timer_t timer_; }; -static void stop_if_stop_path(enum inspector_handshake_event state, - const std::string& path, bool* cont) { - *cont = path.empty() || path != "/close"; +#define SPIN_WHILE(condition) \ + { \ + Timeout timeout(&loop); \ + while ((condition) && !timeout.timed_out) { \ + uv_run(&loop, UV_RUN_NOWAIT); \ + } \ + ASSERT_FALSE((condition)); \ + } + +enum inspector_handshake_event { + kInspectorHandshakeHttpGet, + kInspectorHandshakeUpgraded, + kInspectorHandshakeNoEvents +}; + +struct expectations { + std::string actual_data; + size_t actual_offset; + size_t actual_end; + int err_code; +}; + +static bool waiting_to_close = true; + +void handle_closed(uv_handle_t* handle) { + waiting_to_close = false; } -static bool connected_cb(InspectorSocket* socket, - enum inspector_handshake_event state, - const std::string& path) { - inspector_ready = state == kInspectorHandshakeUpgraded; - last_event = state; +static void really_close(uv_handle_t* handle) { + waiting_to_close = true; + if (!uv_is_closing(handle)) { + uv_close(handle, handle_closed); + SPIN_WHILE(waiting_to_close); + } +} + +static void buffer_alloc_cb(uv_handle_t* stream, size_t len, uv_buf_t* buf) { + buf->base = new char[len]; + buf->len = len; +} + +class TestInspectorDelegate; + +static TestInspectorDelegate* delegate = nullptr; + +// Gtest asserts can't be used in dtor directly. +static void assert_is_delegate(TestInspectorDelegate* d) { + GTEST_ASSERT_EQ(delegate, d); +} + +class TestInspectorDelegate : public InspectorSocket::Delegate { + public: + using delegate_fn = void(*)(inspector_handshake_event, const std::string&, + bool* should_continue); + + TestInspectorDelegate() : inspector_ready(false), + last_event(kInspectorHandshakeNoEvents), + handshake_events(0), + handshake_delegate_(stop_if_stop_path), + fail_on_ws_frame_(false) { } + + ~TestInspectorDelegate() { + assert_is_delegate(this); + delegate = nullptr; + } + + void OnHttpGet(const std::string& path) override { + process(kInspectorHandshakeHttpGet, path); + } + + void OnSocketUpgrade(const std::string& path, + const std::string& ws_key) override { + ws_key_ = ws_key; + process(kInspectorHandshakeUpgraded, path); + } + + void OnWsFrame(const std::vector& buffer) override { + ASSERT_FALSE(fail_on_ws_frame_); + frames.push(buffer); + } + + void SetDelegate(delegate_fn d) { + handshake_delegate_ = d; + } + + void SetInspector(InspectorSocket::Pointer inspector) { + socket_ = std::move(inspector); + } + + void Write(const char* buf, size_t len) { + socket_->Write(buf, len); + } + + void ExpectReadError() { + SPIN_WHILE(frames.empty() || !frames.back().empty()); + } + + void ExpectData(const char* data, size_t len) { + const char* cur = data; + const char* end = data + len; + while (cur < end) { + SPIN_WHILE(frames.empty()); + const std::vector& frame = frames.front(); + EXPECT_FALSE(frame.empty()); + auto c = frame.begin(); + for (; c < frame.end() && cur < end; c++) { + GTEST_ASSERT_EQ(*cur, *c) << "Character #" << cur - data; + cur = cur + 1; + } + EXPECT_EQ(c, frame.end()); + frames.pop(); + } + } + + void FailOnWsFrame() { + fail_on_ws_frame_ = true; + } + + void WaitForDispose() { + SPIN_WHILE(delegate != nullptr); + } + + void Close() { + socket_.reset(); + } + + bool inspector_ready; + std::string last_path; // NOLINT(runtime/string) + inspector_handshake_event last_event; + int handshake_events; + std::queue> frames; + + private: + static void stop_if_stop_path(enum inspector_handshake_event state, + const std::string& path, bool* cont) { + *cont = path.empty() || path != "/close"; + } + + void process(inspector_handshake_event event, const std::string& path); + + bool disposed_ = false; + delegate_fn handshake_delegate_; + InspectorSocket::Pointer socket_; + std::string ws_key_; + bool fail_on_ws_frame_; +}; + +static bool connected = false; +static uv_tcp_t server, client_socket; +static const char SERVER_CLOSE_FRAME[] = {'\x88', '\x00'}; + +struct read_expects { + const char* expected; + size_t expected_len; + size_t pos; + bool read_expected; + bool callback_called; +}; + +static const char HANDSHAKE_REQ[] = "GET /ws/path HTTP/1.1\r\n" + "Host: localhost:9222\r\n" + "Upgrade: websocket\r\n" + "Connection: Upgrade\r\n" + "Sec-WebSocket-Key: aaa==\r\n" + "Sec-WebSocket-Version: 13\r\n\r\n"; + +void TestInspectorDelegate::process(inspector_handshake_event event, + const std::string& path) { + inspector_ready = event == kInspectorHandshakeUpgraded; + last_event = event; if (path.empty()) { last_path = "@@@ Nothing received @@@"; } else { @@ -103,15 +223,23 @@ static bool connected_cb(InspectorSocket* socket, } handshake_events++; bool should_continue = true; - handshake_delegate(state, path, &should_continue); - return should_continue; + handshake_delegate_(event, path, &should_continue); + if (should_continue) { + if (inspector_ready) + socket_->AcceptUpgrade(ws_key_); + } else { + socket_->CancelHandshake(); + } } static void on_new_connection(uv_stream_t* server, int status) { GTEST_ASSERT_EQ(0, status); connected = true; - inspector_accept(server, static_cast(server->data), - connected_cb); + delegate = new TestInspectorDelegate(); + delegate->SetInspector( + InspectorSocket::Accept(server, + InspectorSocket::DelegatePointer(delegate))); + GTEST_ASSERT_NE(nullptr, delegate); } void write_done(uv_write_t* req, int status) { req->data = nullptr; } @@ -129,11 +257,6 @@ static void do_write(const char* data, int len) { SPIN_WHILE(req.data); } -static void buffer_alloc_cb(uv_handle_t* stream, size_t len, uv_buf_t* buf) { - buf->base = new char[len]; - buf->len = len; -} - static void check_data_cb(read_expects* expectation, ssize_t nread, const uv_buf_t* buf, bool* retval) { *retval = false; @@ -207,102 +330,6 @@ static void expect_on_client(const char* data, size_t len) { SPIN_WHILE(!expectation.read_expected); } -struct expectations { - std::string actual_data; - size_t actual_offset; - size_t actual_end; - int err_code; -}; - -static void grow_expects_buffer(uv_handle_t* stream, size_t size, uv_buf_t* b) { - expectations* expects = static_cast( - inspector_from_stream(stream)->data); - size_t end = expects->actual_end; - // Grow the buffer in chunks of 64k. - size_t new_length = (end + size + 65535) & ~((size_t) 0xFFFF); - expects->actual_data.resize(new_length); - *b = uv_buf_init(&expects->actual_data[end], new_length - end); -} - -// static void dump_hex(const char* buf, size_t len) { -// const char* ptr = buf; -// const char* end = ptr + len; -// const char* cptr; -// char c; -// int i; - -// while (ptr < end) { -// cptr = ptr; -// for (i = 0; i < 16 && ptr < end; i++) { -// printf("%2.2X ", *(ptr++)); -// } -// for (i = 72 - (i * 4); i > 0; i--) { -// printf(" "); -// } -// for (i = 0; i < 16 && cptr < end; i++) { -// c = *(cptr++); -// printf("%c", (c > 0x19) ? c : '.'); -// } -// printf("\n"); -// } -// printf("\n\n"); -// } - -static void save_read_data(uv_stream_t* stream, ssize_t nread, - const uv_buf_t* buf) { - expectations* expects = static_cast( - inspector_from_stream(stream)->data); - expects->err_code = nread < 0 ? nread : 0; - if (nread > 0) { - expects->actual_end += nread; - } -} - -static void setup_inspector_expecting() { - if (inspector.data) { - return; - } - expectations* expects = new expectations(); - inspector.data = expects; - inspector_read_start(&inspector, grow_expects_buffer, save_read_data); -} - -static void expect_on_server(const char* data, size_t len) { - setup_inspector_expecting(); - expectations* expects = static_cast(inspector.data); - for (size_t i = 0; i < len;) { - SPIN_WHILE(expects->actual_offset == expects->actual_end); - for (; i < len && expects->actual_offset < expects->actual_end; i++) { - char actual = expects->actual_data[expects->actual_offset++]; - char expected = data[i]; - if (expected != actual) { - fprintf(stderr, "Character %zu:\n", i); - GTEST_ASSERT_EQ(expected, actual); - } - } - } - expects->actual_end -= expects->actual_offset; - if (!expects->actual_end) { - memmove(&expects->actual_data[0], - &expects->actual_data[expects->actual_offset], - expects->actual_end); - } - expects->actual_offset = 0; -} - -static void inspector_record_error_code(uv_stream_t* stream, ssize_t nread, - const uv_buf_t* buf) { - InspectorSocket *inspector = inspector_from_stream(stream); - // Increment instead of assign is to ensure the function is only called once - *(static_cast(inspector->data)) += nread; -} - -static void expect_server_read_error() { - setup_inspector_expecting(); - expectations* expects = static_cast(inspector.data); - SPIN_WHILE(expects->err_code != UV_EPROTO); -} - static void expect_handshake() { const char UPGRADE_RESPONSE[] = "HTTP/1.1 101 Switching Protocols\r\n" @@ -320,35 +347,6 @@ static void expect_handshake_failure() { expect_on_client(UPGRADE_RESPONSE, sizeof(UPGRADE_RESPONSE) - 1); } -static bool waiting_to_close = true; - -void handle_closed(uv_handle_t* handle) { waiting_to_close = false; } - -static void really_close(uv_handle_t* handle) { - waiting_to_close = true; - if (!uv_is_closing(handle)) { - uv_close(handle, handle_closed); - SPIN_WHILE(waiting_to_close); - } -} - -// Called when the test leaves inspector socket in active state -static void manual_inspector_socket_cleanup() { - EXPECT_EQ(0, uv_is_active( - reinterpret_cast(&inspector.tcp))); - really_close(reinterpret_cast(&inspector.tcp)); - delete inspector.ws_state; - inspector.ws_state = nullptr; - delete inspector.http_parsing_state; - inspector.http_parsing_state = nullptr; - inspector.buffer.clear(); -} - -static void assert_both_sockets_closed() { - SPIN_WHILE(uv_is_active(reinterpret_cast(&client_socket))); - SPIN_WHILE(uv_is_active(reinterpret_cast(&inspector.tcp))); -} - static void on_connection(uv_connect_t* connect, int status) { GTEST_ASSERT_EQ(0, status); connect->data = connect; @@ -357,16 +355,10 @@ static void on_connection(uv_connect_t* connect, int status) { class InspectorSocketTest : public ::testing::Test { protected: virtual void SetUp() { - inspector.reinit(); - handshake_delegate = stop_if_stop_path; - handshake_events = 0; connected = false; - inspector_ready = false; - last_event = kInspectorHandshakeHttpGet; GTEST_ASSERT_EQ(0, uv_loop_init(&loop)); server = uv_tcp_t(); client_socket = uv_tcp_t(); - server.data = &inspector; sockaddr_in addr; uv_tcp_init(&loop, &server); uv_tcp_init(&loop, &client_socket); @@ -386,13 +378,7 @@ class InspectorSocketTest : public ::testing::Test { virtual void TearDown() { really_close(reinterpret_cast(&client_socket)); - EXPECT_TRUE(inspector.buffer.empty()); - expectations* expects = static_cast(inspector.data); - if (expects != nullptr) { - GTEST_ASSERT_EQ(expects->actual_end, expects->actual_offset); - delete expects; - inspector.data = nullptr; - } + SPIN_WHILE(delegate != nullptr); const int err = uv_loop_close(&loop); if (err != 0) { uv_print_all_handles(&loop, stderr); @@ -403,22 +389,22 @@ class InspectorSocketTest : public ::testing::Test { TEST_F(InspectorSocketTest, ReadsAndWritesInspectorMessage) { ASSERT_TRUE(connected); - ASSERT_FALSE(inspector_ready); + ASSERT_FALSE(delegate->inspector_ready); do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); - SPIN_WHILE(!inspector_ready); + SPIN_WHILE(!delegate->inspector_ready); expect_handshake(); // 2. Brief exchange const char SERVER_MESSAGE[] = "abcd"; const char CLIENT_FRAME[] = {'\x81', '\x04', 'a', 'b', 'c', 'd'}; - inspector_write(&inspector, SERVER_MESSAGE, sizeof(SERVER_MESSAGE) - 1); + delegate->Write(SERVER_MESSAGE, sizeof(SERVER_MESSAGE) - 1); expect_on_client(CLIENT_FRAME, sizeof(CLIENT_FRAME)); const char SERVER_FRAME[] = {'\x81', '\x84', '\x7F', '\xC2', '\x66', '\x31', '\x4E', '\xF0', '\x55', '\x05'}; const char CLIENT_MESSAGE[] = "1234"; do_write(SERVER_FRAME, sizeof(SERVER_FRAME)); - expect_on_server(CLIENT_MESSAGE, sizeof(CLIENT_MESSAGE) - 1); + delegate->ExpectData(CLIENT_MESSAGE, sizeof(CLIENT_MESSAGE) - 1); // 3. Close const char CLIENT_CLOSE_FRAME[] = {'\x88', '\x80', '\x2D', @@ -487,53 +473,34 @@ TEST_F(InspectorSocketTest, BufferEdgeCases) { "{\"id\":17,\"method\":\"Network.canEmulateNetworkConditions\"}"}; do_write(MULTIPLE_REQUESTS, sizeof(MULTIPLE_REQUESTS)); - expect_on_server(EXPECT, sizeof(EXPECT) - 1); - inspector_read_stop(&inspector); - manual_inspector_socket_cleanup(); + delegate->ExpectData(EXPECT, sizeof(EXPECT) - 1); } TEST_F(InspectorSocketTest, AcceptsRequestInSeveralWrites) { ASSERT_TRUE(connected); - ASSERT_FALSE(inspector_ready); + ASSERT_FALSE(delegate->inspector_ready); // Specifically, break up the request in the "Sec-WebSocket-Key" header name // and value const int write1 = 95; const int write2 = 5; const int write3 = sizeof(HANDSHAKE_REQ) - write1 - write2 - 1; do_write(const_cast(HANDSHAKE_REQ), write1); - ASSERT_FALSE(inspector_ready); + ASSERT_FALSE(delegate->inspector_ready); do_write(const_cast(HANDSHAKE_REQ) + write1, write2); - ASSERT_FALSE(inspector_ready); + ASSERT_FALSE(delegate->inspector_ready); do_write(const_cast(HANDSHAKE_REQ) + write1 + write2, write3); - SPIN_WHILE(!inspector_ready); + SPIN_WHILE(!delegate->inspector_ready); expect_handshake(); - inspector_read_stop(&inspector); GTEST_ASSERT_EQ(uv_is_active(reinterpret_cast(&client_socket)), 0); - manual_inspector_socket_cleanup(); } TEST_F(InspectorSocketTest, ExtraTextBeforeRequest) { - last_event = kInspectorHandshakeUpgraded; - char UNCOOL_BRO[] = "Uncool, bro: Text before the first req\r\n"; - do_write(const_cast(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1); - - ASSERT_FALSE(inspector_ready); - do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); - SPIN_WHILE(last_event != kInspectorHandshakeFailed); - expect_handshake_failure(); - assert_both_sockets_closed(); -} - -TEST_F(InspectorSocketTest, ExtraLettersBeforeRequest) { - char UNCOOL_BRO[] = "Uncool!!"; + delegate->last_event = kInspectorHandshakeUpgraded; + char UNCOOL_BRO[] = "Text before the first req, shouldn't be her\r\n"; do_write(const_cast(UNCOOL_BRO), sizeof(UNCOOL_BRO) - 1); - - ASSERT_FALSE(inspector_ready); - do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); - SPIN_WHILE(last_event != kInspectorHandshakeFailed); expect_handshake_failure(); - assert_both_sockets_closed(); + GTEST_ASSERT_EQ(nullptr, delegate); } TEST_F(InspectorSocketTest, RequestWithoutKey) { @@ -544,87 +511,65 @@ TEST_F(InspectorSocketTest, RequestWithoutKey) { "Sec-WebSocket-Version: 13\r\n\r\n"; do_write(const_cast(BROKEN_REQUEST), sizeof(BROKEN_REQUEST) - 1); - SPIN_WHILE(last_event != kInspectorHandshakeFailed); expect_handshake_failure(); - assert_both_sockets_closed(); } TEST_F(InspectorSocketTest, KillsConnectionOnProtocolViolation) { ASSERT_TRUE(connected); - ASSERT_FALSE(inspector_ready); + ASSERT_FALSE(delegate->inspector_ready); do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); - SPIN_WHILE(!inspector_ready); - ASSERT_TRUE(inspector_ready); + SPIN_WHILE(!delegate->inspector_ready); + ASSERT_TRUE(delegate->inspector_ready); expect_handshake(); const char SERVER_FRAME[] = "I'm not a good WS frame. Nope!"; do_write(SERVER_FRAME, sizeof(SERVER_FRAME)); - expect_server_read_error(); + SPIN_WHILE(delegate != nullptr); GTEST_ASSERT_EQ(uv_is_active(reinterpret_cast(&client_socket)), 0); } TEST_F(InspectorSocketTest, CanStopReadingFromInspector) { ASSERT_TRUE(connected); - ASSERT_FALSE(inspector_ready); + ASSERT_FALSE(delegate->inspector_ready); do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); expect_handshake(); - ASSERT_TRUE(inspector_ready); + ASSERT_TRUE(delegate->inspector_ready); // 2. Brief exchange const char SERVER_FRAME[] = {'\x81', '\x84', '\x7F', '\xC2', '\x66', '\x31', '\x4E', '\xF0', '\x55', '\x05'}; const char CLIENT_MESSAGE[] = "1234"; do_write(SERVER_FRAME, sizeof(SERVER_FRAME)); - expect_on_server(CLIENT_MESSAGE, sizeof(CLIENT_MESSAGE) - 1); + delegate->ExpectData(CLIENT_MESSAGE, sizeof(CLIENT_MESSAGE) - 1); - inspector_read_stop(&inspector); do_write(SERVER_FRAME, sizeof(SERVER_FRAME)); GTEST_ASSERT_EQ(uv_is_active( reinterpret_cast(&client_socket)), 0); - manual_inspector_socket_cleanup(); -} - -static int inspector_closed = 0; - -void inspector_closed_cb(InspectorSocket *inspector, int code) { - inspector_closed++; } TEST_F(InspectorSocketTest, CloseDoesNotNotifyReadCallback) { - inspector_closed = 0; do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); expect_handshake(); - int error_code = 0; - inspector.data = &error_code; - inspector_read_start(&inspector, buffer_alloc_cb, - inspector_record_error_code); - inspector_close(&inspector, inspector_closed_cb); + delegate->Close(); char CLOSE_FRAME[] = {'\x88', '\x00'}; expect_on_client(CLOSE_FRAME, sizeof(CLOSE_FRAME)); - EXPECT_EQ(0, inspector_closed); const char CLIENT_CLOSE_FRAME[] = {'\x88', '\x80', '\x2D', '\x0E', '\x1E', '\xFA'}; + delegate->FailOnWsFrame(); do_write(CLIENT_CLOSE_FRAME, sizeof(CLIENT_CLOSE_FRAME)); - EXPECT_NE(UV_EOF, error_code); - SPIN_WHILE(inspector_closed == 0); - EXPECT_EQ(1, inspector_closed); - inspector.data = nullptr; + SPIN_WHILE(delegate != nullptr); } TEST_F(InspectorSocketTest, CloseWorksWithoutReadEnabled) { - inspector_closed = 0; do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); expect_handshake(); - inspector_close(&inspector, inspector_closed_cb); + delegate->Close(); char CLOSE_FRAME[] = {'\x88', '\x00'}; expect_on_client(CLOSE_FRAME, sizeof(CLOSE_FRAME)); - EXPECT_EQ(0, inspector_closed); const char CLIENT_CLOSE_FRAME[] = {'\x88', '\x80', '\x2D', '\x0E', '\x1E', '\xFA'}; do_write(CLIENT_CLOSE_FRAME, sizeof(CLIENT_CLOSE_FRAME)); - SPIN_WHILE(inspector_closed == 0); - EXPECT_EQ(1, inspector_closed); } // Make sure buffering works @@ -641,26 +586,24 @@ static void send_in_chunks(const char* data, size_t len) { } static const char TEST_SUCCESS[] = "Test Success\n\n"; +static int ReportsHttpGet_eventsCount = 0; static void ReportsHttpGet_handshake(enum inspector_handshake_event state, const std::string& path, bool* cont) { *cont = true; enum inspector_handshake_event expected_state = kInspectorHandshakeHttpGet; std::string expected_path; - switch (handshake_events) { + switch (delegate->handshake_events) { case 1: expected_path = "/some/path"; break; case 2: expected_path = "/respond/withtext"; - inspector_write(&inspector, TEST_SUCCESS, sizeof(TEST_SUCCESS) - 1); + delegate->Write(TEST_SUCCESS, sizeof(TEST_SUCCESS) - 1); break; case 3: expected_path = "/some/path2"; break; - case 5: - expected_state = kInspectorHandshakeFailed; - break; case 4: expected_path = "/close"; *cont = false; @@ -670,10 +613,11 @@ static void ReportsHttpGet_handshake(enum inspector_handshake_event state, } EXPECT_EQ(expected_state, state); EXPECT_EQ(expected_path, path); + ReportsHttpGet_eventsCount = delegate->handshake_events; } TEST_F(InspectorSocketTest, ReportsHttpGet) { - handshake_delegate = ReportsHttpGet_handshake; + delegate->SetDelegate(ReportsHttpGet_handshake); const char GET_REQ[] = "GET /some/path HTTP/1.1\r\n" "Host: localhost:9222\r\n" @@ -688,7 +632,6 @@ TEST_F(InspectorSocketTest, ReportsHttpGet) { send_in_chunks(WRITE_REQUEST, sizeof(WRITE_REQUEST) - 1); expect_on_client(TEST_SUCCESS, sizeof(TEST_SUCCESS) - 1); - const char GET_REQS[] = "GET /some/path2 HTTP/1.1\r\n" "Host: localhost:9222\r\n" "Sec-WebSocket-Key: aaa==\r\n" @@ -698,53 +641,50 @@ TEST_F(InspectorSocketTest, ReportsHttpGet) { "Sec-WebSocket-Key: aaa==\r\n" "Sec-WebSocket-Version: 13\r\n\r\n"; send_in_chunks(GET_REQS, sizeof(GET_REQS) - 1); - expect_handshake_failure(); - EXPECT_EQ(5, handshake_events); + EXPECT_EQ(4, ReportsHttpGet_eventsCount); + EXPECT_EQ(nullptr, delegate); } -static void -HandshakeCanBeCanceled_handshake(enum inspector_handshake_event state, - const std::string& path, bool* cont) { - switch (handshake_events - 1) { +static int HandshakeCanBeCanceled_eventCount = 0; + +static +void HandshakeCanBeCanceled_handshake(enum inspector_handshake_event state, + const std::string& path, bool* cont) { + switch (delegate->handshake_events - 1) { case 0: - EXPECT_EQ(kInspectorHandshakeUpgrading, state); + EXPECT_EQ(kInspectorHandshakeUpgraded, state); EXPECT_EQ("/ws/path", path); break; - case 1: - EXPECT_EQ(kInspectorHandshakeFailed, state); - EXPECT_TRUE(path.empty()); - break; default: EXPECT_TRUE(false); break; } *cont = false; + HandshakeCanBeCanceled_eventCount = delegate->handshake_events; } TEST_F(InspectorSocketTest, HandshakeCanBeCanceled) { - handshake_delegate = HandshakeCanBeCanceled_handshake; + delegate->SetDelegate(HandshakeCanBeCanceled_handshake); do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); expect_handshake_failure(); - EXPECT_EQ(2, handshake_events); + EXPECT_EQ(1, HandshakeCanBeCanceled_eventCount); + EXPECT_EQ(nullptr, delegate); } static void GetThenHandshake_handshake(enum inspector_handshake_event state, const std::string& path, bool* cont) { *cont = true; std::string expected_path = "/ws/path"; - switch (handshake_events - 1) { + switch (delegate->handshake_events - 1) { case 0: EXPECT_EQ(kInspectorHandshakeHttpGet, state); expected_path = "/respond/withtext"; - inspector_write(&inspector, TEST_SUCCESS, sizeof(TEST_SUCCESS) - 1); + delegate->Write(TEST_SUCCESS, sizeof(TEST_SUCCESS) - 1); break; case 1: - EXPECT_EQ(kInspectorHandshakeUpgrading, state); - break; - case 2: EXPECT_EQ(kInspectorHandshakeUpgraded, state); break; default: @@ -755,7 +695,7 @@ static void GetThenHandshake_handshake(enum inspector_handshake_event state, } TEST_F(InspectorSocketTest, GetThenHandshake) { - handshake_delegate = GetThenHandshake_handshake; + delegate->SetDelegate(GetThenHandshake_handshake); const char WRITE_REQUEST[] = "GET /respond/withtext HTTP/1.1\r\n" "Host: localhost:9222\r\n\r\n"; send_in_chunks(WRITE_REQUEST, sizeof(WRITE_REQUEST) - 1); @@ -764,15 +704,7 @@ TEST_F(InspectorSocketTest, GetThenHandshake) { do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); expect_handshake(); - EXPECT_EQ(3, handshake_events); - manual_inspector_socket_cleanup(); -} - -static void WriteBeforeHandshake_inspector_delegate(inspector_handshake_event e, - const std::string& path, - bool* cont) { - if (e == kInspectorHandshakeFailed) - inspector_closed = 1; + EXPECT_EQ(2, delegate->handshake_events); } TEST_F(InspectorSocketTest, WriteBeforeHandshake) { @@ -780,51 +712,31 @@ TEST_F(InspectorSocketTest, WriteBeforeHandshake) { const char MESSAGE2[] = "Message 2"; const char EXPECTED[] = "Message 1Message 2"; - inspector_write(&inspector, MESSAGE1, sizeof(MESSAGE1) - 1); - inspector_write(&inspector, MESSAGE2, sizeof(MESSAGE2) - 1); + delegate->Write(MESSAGE1, sizeof(MESSAGE1) - 1); + delegate->Write(MESSAGE2, sizeof(MESSAGE2) - 1); expect_on_client(EXPECTED, sizeof(EXPECTED) - 1); - inspector_closed = 0; - handshake_delegate = WriteBeforeHandshake_inspector_delegate; really_close(reinterpret_cast(&client_socket)); - SPIN_WHILE(inspector_closed == 0); -} - -static void CleanupSocketAfterEOF_close_cb(InspectorSocket* inspector, - int status) { - *(static_cast(inspector->data)) = true; -} - -static void CleanupSocketAfterEOF_read_cb(uv_stream_t* stream, ssize_t nread, - const uv_buf_t* buf) { - EXPECT_EQ(UV_EOF, nread); - InspectorSocket* insp = inspector_from_stream(stream); - inspector_close(insp, CleanupSocketAfterEOF_close_cb); + SPIN_WHILE(delegate != nullptr); } TEST_F(InspectorSocketTest, CleanupSocketAfterEOF) { do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); expect_handshake(); - inspector_read_start(&inspector, buffer_alloc_cb, - CleanupSocketAfterEOF_read_cb); - for (int i = 0; i < MAX_LOOP_ITERATIONS; ++i) { uv_run(&loop, UV_RUN_NOWAIT); } uv_close(reinterpret_cast(&client_socket), nullptr); - bool flag = false; - inspector.data = &flag; - SPIN_WHILE(!flag); - inspector.data = nullptr; + SPIN_WHILE(delegate != nullptr); } TEST_F(InspectorSocketTest, EOFBeforeHandshake) { const char MESSAGE[] = "We'll send EOF afterwards"; - inspector_write(&inspector, MESSAGE, sizeof(MESSAGE) - 1); + delegate->Write(MESSAGE, sizeof(MESSAGE) - 1); expect_on_client(MESSAGE, sizeof(MESSAGE) - 1); uv_close(reinterpret_cast(&client_socket), nullptr); - SPIN_WHILE(last_event != kInspectorHandshakeFailed); + SPIN_WHILE(delegate != nullptr); } static void fill_message(std::string* buffer) { @@ -843,9 +755,9 @@ static void mask_message(const std::string& message, TEST_F(InspectorSocketTest, Send1Mb) { ASSERT_TRUE(connected); - ASSERT_FALSE(inspector_ready); + ASSERT_FALSE(delegate->inspector_ready); do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); - SPIN_WHILE(!inspector_ready); + SPIN_WHILE(!delegate->inspector_ready); expect_handshake(); // 2. Brief exchange @@ -860,7 +772,7 @@ TEST_F(InspectorSocketTest, Send1Mb) { std::string expected(EXPECTED_FRAME_HEADER, sizeof(EXPECTED_FRAME_HEADER)); expected.append(message); - inspector_write(&inspector, &message[0], message.size()); + delegate->Write(&message[0], message.size()); expect_on_client(&expected[0], expected.size()); char MASK[4] = {'W', 'h', 'O', 'a'}; @@ -874,9 +786,8 @@ TEST_F(InspectorSocketTest, Send1Mb) { outgoing.resize(outgoing.size() + message.size()); mask_message(message, &outgoing[sizeof(FRAME_TO_SERVER_HEADER)], MASK); - setup_inspector_expecting(); // Buffer on the client side. do_write(&outgoing[0], outgoing.size()); - expect_on_server(&message[0], message.size()); + delegate->ExpectData(&message[0], message.size()); // 3. Close const char CLIENT_CLOSE_FRAME[] = {'\x88', '\x80', '\x2D', @@ -887,53 +798,33 @@ TEST_F(InspectorSocketTest, Send1Mb) { reinterpret_cast(&client_socket))); } -static ssize_t err; - -void ErrorCleansUpTheSocket_cb(uv_stream_t* stream, ssize_t read, - const uv_buf_t* buf) { - err = read; -} - TEST_F(InspectorSocketTest, ErrorCleansUpTheSocket) { - inspector_closed = 0; do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); expect_handshake(); const char NOT_A_GOOD_FRAME[] = {'H', 'e', 'l', 'l', 'o'}; - err = 42; - inspector_read_start(&inspector, buffer_alloc_cb, - ErrorCleansUpTheSocket_cb); do_write(NOT_A_GOOD_FRAME, sizeof(NOT_A_GOOD_FRAME)); - SPIN_WHILE(err > 0); - EXPECT_EQ(UV_EPROTO, err); -} - -static void ServerClosedByClient_cb(InspectorSocket* socket, int code) { - *static_cast(socket->data) = true; + SPIN_WHILE(delegate != nullptr); } -TEST_F(InspectorSocketTest, NoCloseResponseFromClinet) { +TEST_F(InspectorSocketTest, NoCloseResponseFromClient) { ASSERT_TRUE(connected); - ASSERT_FALSE(inspector_ready); + ASSERT_FALSE(delegate->inspector_ready); do_write(const_cast(HANDSHAKE_REQ), sizeof(HANDSHAKE_REQ) - 1); - SPIN_WHILE(!inspector_ready); + SPIN_WHILE(!delegate->inspector_ready); expect_handshake(); // 2. Brief exchange const char SERVER_MESSAGE[] = "abcd"; const char CLIENT_FRAME[] = {'\x81', '\x04', 'a', 'b', 'c', 'd'}; - inspector_write(&inspector, SERVER_MESSAGE, sizeof(SERVER_MESSAGE) - 1); + delegate->Write(SERVER_MESSAGE, sizeof(SERVER_MESSAGE) - 1); expect_on_client(CLIENT_FRAME, sizeof(CLIENT_FRAME)); - bool closed = false; - - inspector.data = &closed; - inspector_close(&inspector, ServerClosedByClient_cb); + delegate->Close(); expect_on_client(SERVER_CLOSE_FRAME, sizeof(SERVER_CLOSE_FRAME)); uv_close(reinterpret_cast(&client_socket), nullptr); - SPIN_WHILE(!closed); - inspector.data = nullptr; GTEST_ASSERT_EQ(0, uv_is_active( - reinterpret_cast(&client_socket))); + reinterpret_cast(&client_socket))); + delegate->WaitForDispose(); } } // anonymous namespace diff --git a/test/cctest/test_inspector_socket_server.cc b/test/cctest/test_inspector_socket_server.cc index ab74917234e..49a3ca9f958 100644 --- a/test/cctest/test_inspector_socket_server.cc +++ b/test/cctest/test_inspector_socket_server.cc @@ -95,16 +95,17 @@ class TestInspectorServerDelegate : public SocketServerDelegate { server_ = server; } - bool StartSession(int session_id, const std::string& target_id) override { + void StartSession(int session_id, const std::string& target_id) override { buffer_.clear(); CHECK_NE(targets_.end(), std::find(targets_.begin(), targets_.end(), target_id)); if (target_id == UNCONNECTABLE_TARGET_ID) { - return false; + server_->DeclineSession(session_id); + return; } connected++; session_id_ = session_id; - return true; + server_->AcceptSession(session_id); } void MessageReceived(int session_id, const std::string& message) override { @@ -350,12 +351,13 @@ class ServerHolder { class ServerDelegateNoTargets : public SocketServerDelegate { public: + ServerDelegateNoTargets() : server_(nullptr) { } void Connect(InspectorSocketServer* server) { } void MessageReceived(int session_id, const std::string& message) override { } void EndSession(int session_id) override { } - bool StartSession(int session_id, const std::string& target_id) override { - return false; + void StartSession(int session_id, const std::string& target_id) override { + server_->DeclineSession(session_id); } std::vector GetTargetIds() override { @@ -375,6 +377,9 @@ class ServerDelegateNoTargets : public SocketServerDelegate { } bool done = false; + + private: + InspectorSocketServer* server_; }; static void TestHttpRequest(int port, const std::string& path, @@ -407,7 +412,6 @@ TEST_F(InspectorSocketServerTest, InspectorSessions) { well_behaved_socket.Write(WsHandshakeRequest(MAIN_TARGET_ID)); well_behaved_socket.Expect(WS_HANDSHAKE_RESPONSE); - EXPECT_EQ(1, delegate.connected); well_behaved_socket.Write("\x81\x84\x7F\xC2\x66\x31\x4E\xF0\x55\x05"); @@ -416,7 +420,6 @@ TEST_F(InspectorSocketServerTest, InspectorSessions) { delegate.Write("5678"); well_behaved_socket.Expect("\x81\x4" "5678"); - well_behaved_socket.Write(CLIENT_CLOSE_FRAME); well_behaved_socket.Expect(SERVER_CLOSE_FRAME); diff --git a/test/common/inspector-helper.js b/test/common/inspector-helper.js index 8fc778555d0..1f1738e31b7 100644 --- a/test/common/inspector-helper.js +++ b/test/common/inspector-helper.js @@ -369,28 +369,43 @@ class NodeInstance { }); } - wsHandshake(devtoolsUrl) { - return this.portPromise.then((port) => new Promise((resolve) => { - http.get({ - port, - path: url.parse(devtoolsUrl).path, - headers: { - 'Connection': 'Upgrade', - 'Upgrade': 'websocket', - 'Sec-WebSocket-Version': 13, - 'Sec-WebSocket-Key': 'key==' - } - }).on('upgrade', (message, socket) => { - resolve(new InspectorSession(socket, this)); - }).on('response', common.mustNotCall('Upgrade was not received')); - })); + async sendUpgradeRequest() { + const response = await this.httpGet(null, '/json/list'); + const devtoolsUrl = response[0]['webSocketDebuggerUrl']; + const port = await this.portPromise; + return http.get({ + port, + path: url.parse(devtoolsUrl).path, + headers: { + 'Connection': 'Upgrade', + 'Upgrade': 'websocket', + 'Sec-WebSocket-Version': 13, + 'Sec-WebSocket-Key': 'key==' + } + }); } async connectInspectorSession() { console.log('[test]', 'Connecting to a child Node process'); - const response = await this.httpGet(null, '/json/list'); - const url = response[0]['webSocketDebuggerUrl']; - return this.wsHandshake(url); + const upgradeRequest = await this.sendUpgradeRequest(); + return new Promise((resolve, reject) => { + upgradeRequest + .on('upgrade', + (message, socket) => resolve(new InspectorSession(socket, this))) + .on('response', common.mustNotCall('Upgrade was not received')); + }); + } + + async expectConnectionDeclined() { + console.log('[test]', 'Checking upgrade is not possible'); + const upgradeRequest = await this.sendUpgradeRequest(); + return new Promise((resolve, reject) => { + upgradeRequest + .on('upgrade', common.mustNotCall('Upgrade was received')) + .on('response', (response) => + response.on('data', () => {}) + .on('end', () => resolve(response.statusCode))); + }); } expectShutdown() { @@ -403,6 +418,10 @@ class NodeInstance { return new Promise((resolve) => this._stderrLineCallback = resolve); } + write(message) { + this._process.stdin.write(message); + } + kill() { this._process.kill(); } diff --git a/test/parallel/test-inspector-no-crash-ws-after-bindings.js b/test/parallel/test-inspector-no-crash-ws-after-bindings.js new file mode 100644 index 00000000000..286373068e8 --- /dev/null +++ b/test/parallel/test-inspector-no-crash-ws-after-bindings.js @@ -0,0 +1,30 @@ +'use strict'; +const common = require('../common'); +common.skipIfInspectorDisabled(); +common.crashOnUnhandledRejection(); +const { NodeInstance } = require('../common/inspector-helper.js'); +const assert = require('assert'); + +const expected = 'Can connect now!'; + +const script = ` + 'use strict'; + const { Session } = require('inspector'); + + const s = new Session(); + s.connect(); + console.error('${expected}'); + process.stdin.on('data', () => process.exit(0)); +`; + +async function runTests() { + const instance = new NodeInstance(['--inspect=0', '--expose-internals'], + script); + while (await instance.nextStderrString() !== expected); + assert.strictEqual(400, await instance.expectConnectionDeclined()); + instance.write('Stop!\n'); + assert.deepStrictEqual({ exitCode: 0, signal: null }, + await instance.expectShutdown()); +} + +runTests(); From ec6c063279b4cd4d85dcc0ac7b223c10982b6e77 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Sat, 9 Dec 2017 13:27:31 -0500 Subject: [PATCH 07/29] doc: fix modules.md export example Arrow functions cannot be called with the new keyword, convert to ES6 classes instead. PR-URL: https://github.com/nodejs/node/pull/17579 Refs: https://github.com/nodejs/node/pull/17364 Reviewed-By: Rich Trott --- doc/api/modules.md | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/doc/api/modules.md b/doc/api/modules.md index 4ecdf826a23..cdb591563ce 100644 --- a/doc/api/modules.md +++ b/doc/api/modules.md @@ -38,7 +38,7 @@ In this example, the variable `PI` is private to `circle.js`. The `module.exports` property can be assigned a new value (such as a function or object). -Below, `bar.js` makes use of the `square` module, which exports a constructor: +Below, `bar.js` makes use of the `square` module, which exports a Square class: ```js const Square = require('./square.js'); @@ -50,10 +50,14 @@ The `square` module is defined in `square.js`: ```js // assigning to exports will not modify module, must use module.exports -module.exports = (width) => { - return { - area: () => width ** 2 - }; +module.exports = class Square { + constructor(width) { + this.width = width; + } + + area() { + return this.width ** 2; + } }; ``` From c84ca17305964905c99d16066cb56d6950e30256 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 9 Dec 2017 05:56:22 +0100 Subject: [PATCH 08/29] test: refactor test-http-default-port - Remove redundant `hasCrypto` checks - Use `common.mustCall()` - Use arrow functions - Deduplicate HTTP & HTTPS code PR-URL: https://github.com/nodejs/node/pull/17562 Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig Reviewed-By: Jon Moss Reviewed-By: Anatoli Papirovski --- test/parallel/test-http-default-port.js | 55 +++++++------------------ 1 file changed, 14 insertions(+), 41 deletions(-) diff --git a/test/parallel/test-http-default-port.js b/test/parallel/test-http-default-port.js index 0b3530e76c3..a5af439a8ec 100644 --- a/test/parallel/test-http-default-port.js +++ b/test/parallel/test-http-default-port.js @@ -34,54 +34,27 @@ const options = { key: fixtures.readKey('agent1-key.pem'), cert: fixtures.readKey('agent1-cert.pem') }; -let gotHttpsResp = false; -let gotHttpResp = false; -process.on('exit', function() { - if (common.hasCrypto) { - assert(gotHttpsResp); - } - assert(gotHttpResp); - console.log('ok'); -}); - -http.createServer(function(req, res) { - assert.strictEqual(req.headers.host, hostExpect); - assert.strictEqual(req.headers['x-port'], this.address().port.toString()); - res.writeHead(200); - res.end('ok'); - this.close(); -}).listen(0, function() { - http.globalAgent.defaultPort = this.address().port; - http.get({ - host: 'localhost', - headers: { - 'x-port': this.address().port - } - }, function(res) { - gotHttpResp = true; - res.resume(); - }); -}); - -if (common.hasCrypto) { - https.createServer(options, function(req, res) { +for (const { mod, createServer } of [ + { mod: http, createServer: http.createServer }, + { mod: https, createServer: https.createServer.bind(null, options) } +]) { + const server = createServer(common.mustCall((req, res) => { assert.strictEqual(req.headers.host, hostExpect); - assert.strictEqual(req.headers['x-port'], this.address().port.toString()); + assert.strictEqual(req.headers['x-port'], `${server.address().port}`); res.writeHead(200); res.end('ok'); - this.close(); - }).listen(0, function() { - https.globalAgent.defaultPort = this.address().port; - https.get({ + server.close(); + })).listen(0, common.mustCall(() => { + mod.globalAgent.defaultPort = server.address().port; + mod.get({ host: 'localhost', rejectUnauthorized: false, headers: { - 'x-port': this.address().port + 'x-port': server.address().port } - }, function(res) { - gotHttpsResp = true; + }, common.mustCall((res) => { res.resume(); - }); - }); + })); + })); } From e4a71098efae67f7edf40962b684c816a7f59e68 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sat, 9 Dec 2017 09:38:14 -0500 Subject: [PATCH 09/29] tools: simplify prefer-common-mustnotcall rule PR-URL: https://github.com/nodejs/node/pull/17572 Reviewed-By: Anatoli Papirovski --- .../eslint-rules/prefer-common-mustnotcall.js | 35 +++++++------------ 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/tools/eslint-rules/prefer-common-mustnotcall.js b/tools/eslint-rules/prefer-common-mustnotcall.js index 6bc428ed59a..ef3c5fb729f 100644 --- a/tools/eslint-rules/prefer-common-mustnotcall.js +++ b/tools/eslint-rules/prefer-common-mustnotcall.js @@ -10,30 +10,21 @@ const msg = 'Please use common.mustNotCall(msg) instead of ' + 'common.mustCall(fn, 0) or common.mustCall(0).'; - -function isCommonMustCall(node) { - return node && - node.callee && - node.callee.object && - node.callee.object.name === 'common' && - node.callee.property && - node.callee.property.name === 'mustCall'; -} - -function isArgZero(argument) { - return argument && - typeof argument.value === 'number' && - argument.value === 0; -} +const mustCallSelector = 'CallExpression[callee.object.name="common"]' + + '[callee.property.name="mustCall"]'; +const arg0Selector = `${mustCallSelector}[arguments.0.value=0]`; +const arg1Selector = `${mustCallSelector}[arguments.1.value=0]`; module.exports = function(context) { + function report(node) { + context.report(node, msg); + } + return { - CallExpression(node) { - if (isCommonMustCall(node) && - (isArgZero(node.arguments[0]) || // catch common.mustCall(0) - isArgZero(node.arguments[1]))) { // catch common.mustCall(fn, 0) - context.report(node, msg); - } - } + // Catch common.mustCall(0) + [arg0Selector]: report, + + // Catch common.mustCall(fn, 0) + [arg1Selector]: report }; }; From 66d3e6b2f4c4ca8944f199139e11436144aa79b1 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sat, 9 Dec 2017 09:54:33 -0500 Subject: [PATCH 10/29] tools: simplify prefer-assert-methods rule PR-URL: https://github.com/nodejs/node/pull/17572 Reviewed-By: Anatoli Papirovski --- tools/eslint-rules/prefer-assert-methods.js | 27 ++++++--------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/tools/eslint-rules/prefer-assert-methods.js b/tools/eslint-rules/prefer-assert-methods.js index fa345eb7c3f..0604fd3ed99 100644 --- a/tools/eslint-rules/prefer-assert-methods.js +++ b/tools/eslint-rules/prefer-assert-methods.js @@ -1,15 +1,8 @@ 'use strict'; -function isAssert(node) { - return node.expression && - node.expression.type === 'CallExpression' && - node.expression.callee && - node.expression.callee.name === 'assert'; -} - -function getFirstArg(expression) { - return expression.arguments && expression.arguments[0]; -} +const astSelector = 'ExpressionStatement[expression.type="CallExpression"]' + + '[expression.callee.name="assert"]' + + '[expression.arguments.0.type="BinaryExpression"]'; function parseError(method, op) { return `'assert.${method}' should be used instead of '${op}'`; @@ -24,15 +17,11 @@ const preferedAssertMethod = { module.exports = function(context) { return { - ExpressionStatement(node) { - if (isAssert(node)) { - const arg = getFirstArg(node.expression); - if (arg && arg.type === 'BinaryExpression') { - const assertMethod = preferedAssertMethod[arg.operator]; - if (assertMethod) { - context.report(node, parseError(assertMethod, arg.operator)); - } - } + [astSelector]: function(node) { + const arg = node.expression.arguments[0]; + const assertMethod = preferedAssertMethod[arg.operator]; + if (assertMethod) { + context.report(node, parseError(assertMethod, arg.operator)); } } }; From 0c1dee5089b3c0569f959baeec21207036a011e3 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sat, 9 Dec 2017 10:06:57 -0500 Subject: [PATCH 11/29] tools: simplify buffer-constructor rule PR-URL: https://github.com/nodejs/node/pull/17572 Reviewed-By: Anatoli Papirovski --- tools/eslint-rules/buffer-constructor.js | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/tools/eslint-rules/buffer-constructor.js b/tools/eslint-rules/buffer-constructor.js index 938598e8dbf..412fdaa9346 100644 --- a/tools/eslint-rules/buffer-constructor.js +++ b/tools/eslint-rules/buffer-constructor.js @@ -10,16 +10,11 @@ const msg = 'Use of the Buffer() constructor has been deprecated. ' + 'Please use either Buffer.alloc(), Buffer.allocUnsafe(), ' + 'or Buffer.from()'; - -function test(context, node) { - if (node.callee.name === 'Buffer') { - context.report(node, msg); - } -} +const astSelector = 'NewExpression[callee.name="Buffer"],' + + 'CallExpression[callee.name="Buffer"]'; module.exports = function(context) { return { - 'NewExpression': (node) => test(context, node), - 'CallExpression': (node) => test(context, node) + [astSelector]: (node) => context.report(node, msg) }; }; From 746534b973c62afed90f00fbd8983adec1509b58 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sat, 9 Dec 2017 10:21:23 -0500 Subject: [PATCH 12/29] tools: simplify no-let-in-for-declaration rule PR-URL: https://github.com/nodejs/node/pull/17572 Reviewed-By: Anatoli Papirovski --- .../eslint-rules/no-let-in-for-declaration.js | 46 ++++++------------- 1 file changed, 14 insertions(+), 32 deletions(-) diff --git a/tools/eslint-rules/no-let-in-for-declaration.js b/tools/eslint-rules/no-let-in-for-declaration.js index 34ad2d5761f..1ae49a48dee 100644 --- a/tools/eslint-rules/no-let-in-for-declaration.js +++ b/tools/eslint-rules/no-let-in-for-declaration.js @@ -11,46 +11,28 @@ //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ +const message = 'Use of `let` as the loop variable in a for-loop is ' + + 'not recommended. Please use `var` instead.'; +const forSelector = 'ForStatement[init.kind="let"]'; +const forInOfSelector = 'ForOfStatement[left.kind="let"],' + + 'ForInStatement[left.kind="let"]'; module.exports = { create(context) { const sourceCode = context.getSourceCode(); - const msg = 'Use of `let` as the loop variable in a for-loop is ' + - 'not recommended. Please use `var` instead.'; - - /** - * Report function to test if the for-loop is declared using `let`. - */ - function testForLoop(node) { - if (node.init && node.init.kind === 'let') { - context.report({ - node: node.init, - message: msg, - fix: (fixer) => - fixer.replaceText(sourceCode.getFirstToken(node.init), 'var') - }); - } - } - /** - * Report function to test if the for-in or for-of loop - * is declared using `let`. - */ - function testForInOfLoop(node) { - if (node.left && node.left.kind === 'let') { - context.report({ - node: node.left, - message: msg, - fix: (fixer) => - fixer.replaceText(sourceCode.getFirstToken(node.left), 'var') - }); - } + function report(node) { + context.report({ + node, + message, + fix: (fixer) => + fixer.replaceText(sourceCode.getFirstToken(node), 'var') + }); } return { - 'ForStatement': testForLoop, - 'ForInStatement': testForInOfLoop, - 'ForOfStatement': testForInOfLoop + [forSelector]: (node) => report(node.init), + [forInOfSelector]: (node) => report(node.left), }; } }; From 3674bee884bd86a7eb778345a410b2d8ab473a84 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Fri, 8 Dec 2017 18:03:30 -0800 Subject: [PATCH 13/29] test: simplify common.PORT code common.PORT is no longer used in parallelized tests and should not be. Remove code that accommodates parallelized tests. PR-URL: https://github.com/nodejs/node/pull/17559 Reviewed-By: Luigi Pinca Reviewed-By: Gibson Fahnestock --- test/common/index.js | 4 ++-- test/testpy/__init__.py | 5 +---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/test/common/index.js b/test/common/index.js index 1ed9952a514..7489b8faef0 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -39,8 +39,9 @@ const noop = () => {}; // Using a `.` prefixed name, which is the convention for "hidden" on POSIX, // gets tools to ignore it by default or by simple rules, especially eslint. let tmpDirName = '.tmp'; -// PORT should match the definition in test/testpy/__init__.py. + exports.PORT = +process.env.NODE_COMMON_PORT || 12346; + exports.isWindows = process.platform === 'win32'; exports.isWOW64 = exports.isWindows && (process.env.PROCESSOR_ARCHITEW6432 !== undefined); @@ -162,7 +163,6 @@ exports.refreshTmpDir = function() { }; if (process.env.TEST_THREAD_ID) { - exports.PORT += process.env.TEST_THREAD_ID * 100; tmpDirName += `.${process.env.TEST_THREAD_ID}`; } exports.tmpDir = path.join(testRoot, tmpDirName); diff --git a/test/testpy/__init__.py b/test/testpy/__init__.py index f113c1253a4..455d57f4d91 100644 --- a/test/testpy/__init__.py +++ b/test/testpy/__init__.py @@ -61,10 +61,7 @@ def GetCommand(self): source = open(self.file).read() flags_match = FLAGS_PATTERN.search(source) if flags_match: - # PORT should match the definition in test/common/index.js. - env = { 'PORT': int(os.getenv('NODE_COMMON_PORT', '12346')) } - env['PORT'] += self.thread_id * 100 - flag = flags_match.group(1).strip().format(**env).split() + flag = flags_match.group(1).strip().split() # The following block reads config.gypi to extract the v8_enable_inspector # value. This is done to check if the inspector is disabled in which case # the '--inspect' flag cannot be passed to the node process as it will From 800ce94e5cdaa8dba7cf69b4c560a66a51ae0ec8 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Fri, 8 Dec 2017 18:33:36 -0800 Subject: [PATCH 14/29] tools,test: throw if common.PORT used in parallel tests common.PORT should not be used in parallelized tests. (There can be a port collision if another tests requests an arbitrary open port from the operating system and ends up getting common.PORT before a test that uses common.PORT uses the port.) In such a situation, throw an error. PR-URL: https://github.com/nodejs/node/pull/17559 Reviewed-By: Luigi Pinca Reviewed-By: Gibson Fahnestock --- test/common/index.js | 11 ++++++++++- tools/test.py | 3 ++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/test/common/index.js b/test/common/index.js index 7489b8faef0..81e76be0f31 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -40,7 +40,16 @@ const noop = () => {}; // gets tools to ignore it by default or by simple rules, especially eslint. let tmpDirName = '.tmp'; -exports.PORT = +process.env.NODE_COMMON_PORT || 12346; +Object.defineProperty(exports, 'PORT', { + get: () => { + if (+process.env.TEST_PARALLEL) { + throw new Error('common.PORT cannot be used in a parallelized test'); + } + return +process.env.NODE_COMMON_PORT || 12346; + }, + enumerable: true +}); + exports.isWindows = process.platform === 'win32'; exports.isWOW64 = exports.isWindows && diff --git a/tools/test.py b/tools/test.py index ccc25f2a883..230774b2561 100755 --- a/tools/test.py +++ b/tools/test.py @@ -532,7 +532,8 @@ def Run(self): try: result = self.RunCommand(self.GetCommand(), { - "TEST_THREAD_ID": "%d" % self.thread_id + "TEST_THREAD_ID": "%d" % self.thread_id, + "TEST_PARALLEL" : "%d" % self.parallel }) finally: # Tests can leave the tty in non-blocking mode. If the test runner From ba9801a8d26853e791fd3d16c2dc39c07c790119 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Fri, 8 Dec 2017 21:23:29 -0800 Subject: [PATCH 15/29] test: remove unnecessary use of common.PORT in addons test Using port 0 to request an open port from the operating system is sufficient in openssl-client-cert-engine/test.js. PR-URL: https://github.com/nodejs/node/pull/17563 Reviewed-By: Luigi Pinca Reviewed-By: Richard Lau Reviewed-By: Jon Moss Reviewed-By: Colin Ihrig --- test/addons/openssl-client-cert-engine/test.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/addons/openssl-client-cert-engine/test.js b/test/addons/openssl-client-cert-engine/test.js index d07b9c6a1c2..1c0e4564a5c 100644 --- a/test/addons/openssl-client-cert-engine/test.js +++ b/test/addons/openssl-client-cert-engine/test.js @@ -21,8 +21,6 @@ const agentKey = fs.readFileSync(fixture.path('/keys/agent1-key.pem')); const agentCert = fs.readFileSync(fixture.path('/keys/agent1-cert.pem')); const agentCa = fs.readFileSync(fixture.path('/keys/ca1-cert.pem')); -const port = common.PORT; - const serverOptions = { key: agentKey, cert: agentCert, @@ -34,11 +32,11 @@ const serverOptions = { const server = https.createServer(serverOptions, (req, res) => { res.writeHead(200); res.end('hello world'); -}).listen(port, common.localhostIPv4, () => { +}).listen(0, common.localhostIPv4, () => { const clientOptions = { method: 'GET', host: common.localhostIPv4, - port: port, + port: server.address().port, path: '/test', clientCertEngine: engine, // engine will provide key+cert rejectUnauthorized: false, // prevent failing on self-signed certificates From ac25cee2e22ac4c64e4a92b33fe3784648b97072 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 10 Dec 2017 15:55:00 -0800 Subject: [PATCH 16/29] test: refactor test-child-process-pass-fd Add a comment explaining the test (especailly why it forks 80 processes. Use destructuring and an arrow function callback. PR-URL: https://github.com/nodejs/node/pull/17596 Reviewed-By: Khaidi Chu Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig Reviewed-By: Minwoo Jung --- test/sequential/test-child-process-pass-fd.js | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/test/sequential/test-child-process-pass-fd.js b/test/sequential/test-child-process-pass-fd.js index 8cc11f6c11f..73e469cdede 100644 --- a/test/sequential/test-child-process-pass-fd.js +++ b/test/sequential/test-child-process-pass-fd.js @@ -1,11 +1,20 @@ 'use strict'; const common = require('../common'); + +// On some OS X versions, when passing fd's between processes: +// When the handle associated to a specific file descriptor is closed by the +// sender process before it's received in the destination, the handle is indeed +// closed while it should remain opened. In order to fix this behavior, don't +// close the handle until the `NODE_HANDLE_ACK` is received by the sender. +// This test is basically `test-cluster-net-send` but creating lots of workers +// so the issue reproduces on OS X consistently. + if ((process.config.variables.arm_version === '6') || (process.config.variables.arm_version === '7')) common.skip('Too slow for armv6 and armv7 bots'); const assert = require('assert'); -const fork = require('child_process').fork; +const { fork } = require('child_process'); const net = require('net'); const N = 80; @@ -46,14 +55,14 @@ if (process.argv[2] !== 'child') { process.on('message', common.mustCall()); const server = net.createServer((c) => { - process.once('message', function(msg) { + process.once('message', (msg) => { assert.strictEqual(msg, 'got'); c.end('hello'); }); socketConnected(); }).unref(); server.listen(0, common.localhostIPv4, () => { - const port = server.address().port; + const { port } = server.address(); socket = net.connect(port, common.localhostIPv4, socketConnected).unref(); }); } From b961d9fd83c963657c2305ed13ff447573eac852 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Benno=20F=C3=BCnfst=C3=BCck?= Date: Mon, 16 Oct 2017 15:36:32 +0200 Subject: [PATCH 17/29] http: disallow two-byte characters in URL path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit changes node's handling of two-byte characters in the path component of an http URL. Previously, node would just strip the higher byte when generating the request. So this code: ``` http.request({host: "example.com", port: "80", "/Ｎ"}) ``` would request `http://example.com/.` (`.` is the character for the byte `0x2e`). This is not useful and can in some cases lead to filter evasion. With this change, the code generates `ERR_UNESCAPED_CHARACTERS`, just like space and control characters already did. PR-URL: https://github.com/nodejs/node/pull/16237 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Anatoli Papirovski Reviewed-By: Ruben Bridgewater Reviewed-By: Timothy Gu --- lib/_http_client.js | 36 ++----------------- .../parallel/test-http-client-invalid-path.js | 12 +++++++ 2 files changed, 14 insertions(+), 34 deletions(-) create mode 100644 test/parallel/test-http-client-invalid-path.js diff --git a/lib/_http_client.js b/lib/_http_client.js index 5b568628006..bdda708493a 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -41,33 +41,7 @@ const { outHeadersKey } = require('internal/http'); const { nextTick } = require('internal/process/next_tick'); const errors = require('internal/errors'); -// The actual list of disallowed characters in regexp form is more like: -// /[^A-Za-z0-9\-._~!$&'()*+,;=/:@]/ -// with an additional rule for ignoring percentage-escaped characters, but -// that's a) hard to capture in a regular expression that performs well, and -// b) possibly too restrictive for real-world usage. So instead we restrict the -// filter to just control characters and spaces. -// -// This function is used in the case of small paths, where manual character code -// checks can greatly outperform the equivalent regexp (tested in V8 5.4). -function isInvalidPath(s) { - var i = 0; - if (s.charCodeAt(0) <= 32) return true; - if (++i >= s.length) return false; - if (s.charCodeAt(1) <= 32) return true; - if (++i >= s.length) return false; - if (s.charCodeAt(2) <= 32) return true; - if (++i >= s.length) return false; - if (s.charCodeAt(3) <= 32) return true; - if (++i >= s.length) return false; - if (s.charCodeAt(4) <= 32) return true; - if (++i >= s.length) return false; - if (s.charCodeAt(5) <= 32) return true; - ++i; - for (; i < s.length; ++i) - if (s.charCodeAt(i) <= 32) return true; - return false; -} +const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/; function validateHost(host, name) { if (host != null && typeof host !== 'string') { @@ -117,13 +91,7 @@ function ClientRequest(options, cb) { var path; if (options.path) { path = String(options.path); - var invalidPath; - if (path.length <= 39) { // Determined experimentally in V8 5.4 - invalidPath = isInvalidPath(path); - } else { - invalidPath = /[\u0000-\u0020]/.test(path); - } - if (invalidPath) + if (INVALID_PATH_REGEX.test(path)) throw new errors.TypeError('ERR_UNESCAPED_CHARACTERS', 'Request path'); } diff --git a/test/parallel/test-http-client-invalid-path.js b/test/parallel/test-http-client-invalid-path.js new file mode 100644 index 00000000000..c042d61eda0 --- /dev/null +++ b/test/parallel/test-http-client-invalid-path.js @@ -0,0 +1,12 @@ +'use strict'; +const common = require('../common'); +const http = require('http'); + +common.expectsError(() => { + http.request({ + path: '/thisisinvalid\uffe2' + }).end(); +}, { + code: 'ERR_UNESCAPED_CHARACTERS', + type: TypeError +}); From e17dba7a4553081a4ae84e4f27bf89e589e27fed Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 6 Dec 2017 14:48:37 -0200 Subject: [PATCH 18/29] net: remove Socket.prototype.listen The function was never documented and now throws a TypeError if used. PR-URL: https://github.com/nodejs/node/pull/13735 Reviewed-By: Colin Ihrig Reviewed-By: Ben Noordhuis Reviewed-By: Anatoli Papirovski --- lib/net.js | 9 --------- 1 file changed, 9 deletions(-) diff --git a/lib/net.js b/lib/net.js index eec7619bc74..f962a848fad 100644 --- a/lib/net.js +++ b/lib/net.js @@ -380,15 +380,6 @@ Socket.prototype.read = function(n) { return this.read(n); }; - -// FIXME(joyeecheung): this method is neither documented nor tested -Socket.prototype.listen = function() { - debug('socket.listen'); - this.on('connection', arguments[0]); - listenInCluster(this, null, null, null); -}; - - Socket.prototype.setTimeout = function(msecs, callback) { if (msecs === 0) { timers.unenroll(this); From 8d5b3de8cb3467ad9f29232f0ecbbd2fd6f99438 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 11 Dec 2017 06:34:57 -0200 Subject: [PATCH 19/29] test: simplify common.expectsError The mustCall is actually only necessary in case it is used as callback. Otherwise it works as a must call on its own. PR-URL: https://github.com/nodejs/node/pull/17616 Reviewed-By: Anatoli Papirovski Reviewed-By: Colin Ihrig Reviewed-By: Jon Moss --- test/common/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/common/index.js b/test/common/index.js index 81e76be0f31..6a22849843a 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -735,7 +735,7 @@ exports.expectsError = function expectsError(fn, settings, exact) { settings = fn; fn = undefined; } - const innerFn = exports.mustCall(function(error) { + function innerFn(error) { assert.strictEqual(error.code, settings.code); if ('type' in settings) { const type = settings.type; @@ -768,12 +768,12 @@ exports.expectsError = function expectsError(fn, settings, exact) { }); } return true; - }, exact); + } if (fn) { assert.throws(fn, innerFn); return; } - return innerFn; + return exports.mustCall(innerFn, exact); }; exports.skipIfInspectorDisabled = function skipIfInspectorDisabled() { From dd2200ecf831cae5f57b69e3ee8a934efbe20af8 Mon Sep 17 00:00:00 2001 From: Jamie Davis Date: Thu, 7 Dec 2017 12:44:42 -0500 Subject: [PATCH 20/29] test: add test description to fs.readFile tests PR-URL: https://github.com/nodejs/node/pull/17610 Refs: https://github.com/nodejs/node/pull/17054#discussion_r155406755 Reviewed-By: Anna Henningsen Reviewed-By: Evan Lucas Reviewed-By: Colin Ihrig Reviewed-By: Jeremiah Senkpiel Reviewed-By: Jon Moss Reviewed-By: Ruben Bridgewater --- test/parallel/test-fs-readfile-empty.js | 3 +++ test/parallel/test-fs-readfile-error.js | 3 +++ test/parallel/test-fs-readfile-fd.js | 3 +++ test/parallel/test-fs-readfile-unlink.js | 3 +++ test/parallel/test-fs-readfile-zero-byte-liar.js | 3 +++ 5 files changed, 15 insertions(+) diff --git a/test/parallel/test-fs-readfile-empty.js b/test/parallel/test-fs-readfile-empty.js index bbc2e9c81d6..21f99fc6be2 100644 --- a/test/parallel/test-fs-readfile-empty.js +++ b/test/parallel/test-fs-readfile-empty.js @@ -21,6 +21,9 @@ 'use strict'; require('../common'); + +// Trivial test of fs.readFile on an empty file. + const assert = require('assert'); const fs = require('fs'); const fixtures = require('../common/fixtures'); diff --git a/test/parallel/test-fs-readfile-error.js b/test/parallel/test-fs-readfile-error.js index e7c52f19a83..616760b0669 100644 --- a/test/parallel/test-fs-readfile-error.js +++ b/test/parallel/test-fs-readfile-error.js @@ -21,6 +21,9 @@ 'use strict'; const common = require('../common'); + +// Test that fs.readFile fails correctly on a non-existent file. + // `fs.readFile('/')` does not fail on FreeBSD, because you can open and read // the directory there. if (common.isFreeBSD) diff --git a/test/parallel/test-fs-readfile-fd.js b/test/parallel/test-fs-readfile-fd.js index 7881666cadb..7458af7b2e5 100644 --- a/test/parallel/test-fs-readfile-fd.js +++ b/test/parallel/test-fs-readfile-fd.js @@ -1,5 +1,8 @@ 'use strict'; require('../common'); + +// Test fs.readFile using a file descriptor. + const fixtures = require('../common/fixtures'); const assert = require('assert'); const fs = require('fs'); diff --git a/test/parallel/test-fs-readfile-unlink.js b/test/parallel/test-fs-readfile-unlink.js index abcbed7ad5d..9ec2e849bee 100644 --- a/test/parallel/test-fs-readfile-unlink.js +++ b/test/parallel/test-fs-readfile-unlink.js @@ -21,6 +21,9 @@ 'use strict'; const common = require('../common'); + +// Test that unlink succeeds immediately after readFile completes. + const assert = require('assert'); const fs = require('fs'); const path = require('path'); diff --git a/test/parallel/test-fs-readfile-zero-byte-liar.js b/test/parallel/test-fs-readfile-zero-byte-liar.js index 625438e7c68..ec0b9cf6922 100644 --- a/test/parallel/test-fs-readfile-zero-byte-liar.js +++ b/test/parallel/test-fs-readfile-zero-byte-liar.js @@ -21,6 +21,9 @@ 'use strict'; const common = require('../common'); + +// Test that readFile works even when stat returns size 0. + const assert = require('assert'); const fs = require('fs'); From 93656f4366bcb6084cae63aeb97a63d3634bac2b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 11 Dec 2017 04:37:47 +0100 Subject: [PATCH 21/29] doc,test: mention Duplex support for TLS Document and test the existing support for generic Duplex streams in the TLS module. PR-URL: https://github.com/nodejs/node/pull/17599 Reviewed-By: Rich Trott Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig Reviewed-By: Luigi Pinca --- doc/api/tls.md | 15 ++++++---- test/parallel/test-tls-generic-stream.js | 38 ++++++++++++++++++++++++ tools/doc/type-parser.js | 1 + 3 files changed, 49 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-tls-generic-stream.js diff --git a/doc/api/tls.md b/doc/api/tls.md index 38e35fb5d2d..13af0b2aa2a 100644 --- a/doc/api/tls.md +++ b/doc/api/tls.md @@ -455,7 +455,10 @@ changes: description: ALPN options are supported now. --> -* `socket` {net.Socket} An instance of [`net.Socket`][] +* `socket` {net.Socket|stream.Duplex} + On the server side, any `Duplex` stream. On the client side, any + instance of [`net.Socket`][] (for generic `Duplex` stream support + on the client side, [`tls.connect()`][] must be used). * `options` {Object} * `isServer`: The SSL/TLS protocol is asymmetrical, TLSSockets must know if they are to behave as a server or a client. If `true` the TLS socket will be @@ -815,10 +818,12 @@ changes: * `port` {number} Port the client should connect to. * `path` {string} Creates unix socket connection to path. If this option is specified, `host` and `port` are ignored. - * `socket` {net.Socket} Establish secure connection on a given socket rather - than creating a new socket. If this option is specified, `path`, `host` and - `port` are ignored. Usually, a socket is already connected when passed to - `tls.connect()`, but it can be connected later. Note that + * `socket` {stream.Duplex} Establish secure connection on a given socket + rather than creating a new socket. Typically, this is an instance of + [`net.Socket`][], but any `Duplex` stream is allowed. + If this option is specified, `path`, `host` and `port` are ignored, + except for certificate validation. Usually, a socket is already connected + when passed to `tls.connect()`, but it can be connected later. Note that connection/disconnection/destruction of `socket` is the user's responsibility, calling `tls.connect()` will not cause `net.connect()` to be called. diff --git a/test/parallel/test-tls-generic-stream.js b/test/parallel/test-tls-generic-stream.js new file mode 100644 index 00000000000..d4e5427acae --- /dev/null +++ b/test/parallel/test-tls-generic-stream.js @@ -0,0 +1,38 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const fixtures = require('../common/fixtures'); +const makeDuplexPair = require('../common/duplexpair'); +const assert = require('assert'); +const { TLSSocket, connect } = require('tls'); + +const key = fixtures.readKey('agent1-key.pem'); +const cert = fixtures.readKey('agent1-cert.pem'); +const ca = fixtures.readKey('ca1-cert.pem'); + +const { clientSide, serverSide } = makeDuplexPair(); + +const clientTLS = connect({ + socket: clientSide, + ca, + host: 'agent1' // Hostname from certificate +}); +const serverTLS = new TLSSocket(serverSide, { + isServer: true, + key, + cert, + ca +}); + +assert.strictEqual(clientTLS.connecting, false); +assert.strictEqual(serverTLS.connecting, false); + +clientTLS.on('secureConnect', common.mustCall(() => { + clientTLS.write('foobar', common.mustCall(() => { + assert.strictEqual(serverTLS.read().toString(), 'foobar'); + assert.strictEqual(clientTLS._handle.writeQueueSize, 0); + })); + assert.ok(clientTLS._handle.writeQueueSize > 0); +})); diff --git a/tools/doc/type-parser.js b/tools/doc/type-parser.js index 43af1cb9781..3bcdd817a2f 100644 --- a/tools/doc/type-parser.js +++ b/tools/doc/type-parser.js @@ -49,6 +49,7 @@ const typeMap = { 'Stream': 'stream.html#stream_stream', 'stream.Readable': 'stream.html#stream_class_stream_readable', 'stream.Writable': 'stream.html#stream_class_stream_writable', + 'stream.Duplex': 'stream.html#stream_class_stream_duplex', 'tls.TLSSocket': 'tls.html#tls_class_tls_tlssocket', From 88a9e2df859c9de8a3b2d0cca9862ee00478c863 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 8 Dec 2017 15:33:41 +0100 Subject: [PATCH 22/29] src: fix inspector nullptr deref on abrupt exit Fix a nullptr dereference on abrupt termination when async call stack recording is enabled. Bug discovered while trying to write a regression test for the bug fix in commit df79b7d821 ("src: fix missing handlescope bug in inspector".) PR-URL: https://github.com/nodejs/node/pull/17577 Reviewed-By: Anna Henningsen Reviewed-By: Timothy Gu Reviewed-By: Colin Ihrig Reviewed-By: Refael Ackermann Reviewed-By: Eugene Ostroukhov --- src/inspector_agent.cc | 10 +++--- .../test-inspector-async-call-stack-abort.js | 34 +++++++++++++++++++ 2 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 test/sequential/test-inspector-async-call-stack-abort.js diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 216b43ca6c3..95e1d642915 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -325,10 +325,12 @@ class NodeInspectorClient : public V8InspectorClient { } void maxAsyncCallStackDepthChanged(int depth) override { - if (depth == 0) { - env_->inspector_agent()->DisableAsyncHook(); - } else { - env_->inspector_agent()->EnableAsyncHook(); + if (auto agent = env_->inspector_agent()) { + if (depth == 0) { + agent->DisableAsyncHook(); + } else { + agent->EnableAsyncHook(); + } } } diff --git a/test/sequential/test-inspector-async-call-stack-abort.js b/test/sequential/test-inspector-async-call-stack-abort.js new file mode 100644 index 00000000000..1ec46ab3cfe --- /dev/null +++ b/test/sequential/test-inspector-async-call-stack-abort.js @@ -0,0 +1,34 @@ +// Check that abrupt termination when async call stack recording is enabled +// does not segfault the process. +'use strict'; +const common = require('../common'); +common.skipIfInspectorDisabled(); +common.skipIf32Bits(); + +const { strictEqual } = require('assert'); +const eyecatcher = 'nou, houdoe he?'; + +if (process.argv[2] === 'child') { + const { Session } = require('inspector'); + const { promisify } = require('util'); + const { registerAsyncHook } = process.binding('inspector'); + (async () => { + let enabled = 0; + registerAsyncHook(() => ++enabled, () => {}); + const session = new Session(); + session.connect(); + session.post = promisify(session.post); + await session.post('Debugger.enable'); + strictEqual(enabled, 0); + await session.post('Debugger.setAsyncCallStackDepth', { maxDepth: 42 }); + strictEqual(enabled, 1); + throw new Error(eyecatcher); + })(); +} else { + const { spawnSync } = require('child_process'); + const options = { encoding: 'utf8' }; + const proc = spawnSync(process.execPath, [__filename, 'child'], options); + strictEqual(proc.status, 0); + strictEqual(proc.signal, null); + strictEqual(proc.stderr.includes(eyecatcher), true); +} From 727339e9c276748d7b4efa066b72e05a1d60a4b9 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 9 Dec 2017 12:43:40 +0100 Subject: [PATCH 23/29] repl: fix util.inspect() coloring regression The `Object.assign()` calls introduced in commit 90a4390 ("repl: show proxies as Proxy objects") mutated their first argument, causing the `{ colors: true }` option from the REPL to leak over into the global `util.inspect.defaultOptions` object. Refs: https://github.com/nodejs/node/pull/16485#issuecomment-350428638 PR-URL: https://github.com/nodejs/node/pull/17565 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: Ruben Bridgewater Reviewed-By: Timothy Gu Reviewed-By: Jeremiah Senkpiel Reviewed-By: Luigi Pinca --- lib/repl.js | 4 ++-- test/parallel/test-repl-colors.js | 31 +++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-repl-colors.js diff --git a/lib/repl.js b/lib/repl.js index e2e716dd66f..da3ed78e9eb 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -103,7 +103,7 @@ function hasOwnProperty(obj, prop) { // This is the default "writer" value if none is passed in the REPL options. const writer = exports.writer = (obj) => util.inspect(obj, writer.options); writer.options = - Object.assign(util.inspect.defaultOptions, { showProxy: true }); + Object.assign({}, util.inspect.defaultOptions, { showProxy: true }); exports._builtinLibs = internalModule.builtinLibs; @@ -470,7 +470,7 @@ function REPLServer(prompt, if (self.useColors && self.writer === writer) { // Turn on ANSI coloring. self.writer = (obj) => util.inspect(obj, self.writer.options); - self.writer.options = Object.assign(writer.options, { colors: true }); + self.writer.options = Object.assign({}, writer.options, { colors: true }); } function filterInternalStackFrames(error, structuredStack) { diff --git a/test/parallel/test-repl-colors.js b/test/parallel/test-repl-colors.js new file mode 100644 index 00000000000..dfcc020d899 --- /dev/null +++ b/test/parallel/test-repl-colors.js @@ -0,0 +1,31 @@ +/* eslint-disable quotes */ +'use strict'; +require('../common'); +const { Duplex } = require('stream'); +const { inspect } = require('util'); +const { strictEqual } = require('assert'); +const { REPLServer } = require('repl'); + +let output = ''; + +const inout = new Duplex({ decodeStrings: false }); +inout._read = function() { + this.push('util.inspect("string")\n'); + this.push(null); +}; +inout._write = function(s, _, cb) { + output += s; + cb(); +}; + +const repl = new REPLServer({ input: inout, output: inout, useColors: true }); + +process.on('exit', function() { + // https://github.com/nodejs/node/pull/16485#issuecomment-350428638 + // The color setting of the REPL should not have leaked over into + // the color setting of `util.inspect.defaultOptions`. + strictEqual(output.includes(`'\\'string\\''`), true); + strictEqual(output.includes(`'\u001b[32m\\'string\\'\u001b[39m'`), false); + strictEqual(inspect.defaultOptions.colors, false); + strictEqual(repl.writer.options.colors, true); +}); From d11f7d54f35af093efbee208b7920c158cfc2c72 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 9 Dec 2017 05:41:22 +0100 Subject: [PATCH 24/29] tls: use correct class name in deprecation message `tls.Socket` does not exist, and the deprecation message should refer to `tls.TLSSocket` (like the documentation for the deprecation message already does). PR-URL: https://github.com/nodejs/node/pull/17561 Reviewed-By: Rich Trott Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig Reviewed-By: Jon Moss Reviewed-By: Timothy Gu Reviewed-By: Anatoli Papirovski Reviewed-By: Ruben Bridgewater --- lib/_tls_legacy.js | 2 +- test/parallel/test-tls-legacy-deprecated.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/_tls_legacy.js b/lib/_tls_legacy.js index 07b95546b33..d65da58e8cf 100644 --- a/lib/_tls_legacy.js +++ b/lib/_tls_legacy.js @@ -949,6 +949,6 @@ module.exports = { createSecurePair: internalUtil.deprecate(createSecurePair, 'tls.createSecurePair() is deprecated. Please use ' + - 'tls.Socket instead.', 'DEP0064'), + 'tls.TLSSocket instead.', 'DEP0064'), pipe }; diff --git a/test/parallel/test-tls-legacy-deprecated.js b/test/parallel/test-tls-legacy-deprecated.js index 4a6120c71b1..3b1919010e4 100644 --- a/test/parallel/test-tls-legacy-deprecated.js +++ b/test/parallel/test-tls-legacy-deprecated.js @@ -9,7 +9,7 @@ const tls = require('tls'); common.expectWarning( 'DeprecationWarning', - 'tls.createSecurePair() is deprecated. Please use tls.Socket instead.' + 'tls.createSecurePair() is deprecated. Please use tls.TLSSocket instead.' ); assert.doesNotThrow(() => tls.createSecurePair()); From 2050bab09c7b3917caa92133299da0f7c07bd38d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 5 Dec 2017 13:23:31 +0100 Subject: [PATCH 25/29] src: minor cleanups to node_url.cc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove pointless pointers - Make `WriteHost` take a const argument so that it’s functionality is clear from the signature - Make `FindLongestZeroSequence` templated to accommodate the constness in `WriteHost` and because using `uint16_t` is an articifial, unnecessary restriction - Remove string copying when no copies are needed - Make `PercentDecode` just return its return value - Make `ParseHost` (string-only version) take its constant argument as a constant reference PR-URL: https://github.com/nodejs/node/pull/17470 Reviewed-By: Timothy Gu --- src/node_url.cc | 61 +++++++++++++++++++++++-------------------------- 1 file changed, 29 insertions(+), 32 deletions(-) diff --git a/src/node_url.cc b/src/node_url.cc index e1ef9273ae9..b4b16399aab 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -506,12 +506,11 @@ static inline unsigned hex2bin(const T ch) { return static_cast(-1); } -static inline void PercentDecode(const char* input, - size_t len, - std::string* dest) { +inline std::string PercentDecode(const char* input, size_t len) { + std::string dest; if (len == 0) - return; - dest->reserve(len); + return dest; + dest.reserve(len); const char* pointer = input; const char* end = input + len; @@ -522,17 +521,18 @@ static inline void PercentDecode(const char* input, (ch == '%' && (!IsASCIIHexDigit(pointer[1]) || !IsASCIIHexDigit(pointer[2])))) { - *dest += ch; + dest += ch; pointer++; continue; } else { unsigned a = hex2bin(pointer[1]); unsigned b = hex2bin(pointer[2]); char c = static_cast(a * 16 + b); - *dest += c; + dest += c; pointer += 3; } } + return dest; } #define SPECIALS(XX) \ @@ -860,7 +860,7 @@ static url_host_type ParseHost(url_host* host, return ParseOpaqueHost(host, input, length); // First, we have to percent decode - PercentDecode(input, length, &decoded); + decoded = PercentDecode(input, length); // Then we have to punycode toASCII if (!ToASCII(decoded, &decoded)) @@ -894,13 +894,13 @@ static url_host_type ParseHost(url_host* host, // Locates the longest sequence of 0 segments in an IPv6 address // in order to use the :: compression when serializing -static inline uint16_t* FindLongestZeroSequence(uint16_t* values, - size_t len) { - uint16_t* start = values; - uint16_t* end = start + len; - uint16_t* result = nullptr; +template +static inline T* FindLongestZeroSequence(T* values, size_t len) { + T* start = values; + T* end = start + len; + T* result = nullptr; - uint16_t* current = nullptr; + T* current = nullptr; unsigned counter = 0, longest = 1; while (start < end) { @@ -923,7 +923,7 @@ static inline uint16_t* FindLongestZeroSequence(uint16_t* values, return result; } -static url_host_type WriteHost(url_host* host, std::string* dest) { +static url_host_type WriteHost(const url_host* host, std::string* dest) { dest->clear(); switch (host->type) { case HOST_TYPE_DOMAIN: @@ -934,8 +934,7 @@ static url_host_type WriteHost(url_host* host, std::string* dest) { uint32_t value = host->value.ipv4; for (int n = 0; n < 4; n++) { char buf[4]; - char* buffer = buf; - snprintf(buffer, sizeof(buf), "%d", value % 256); + snprintf(buf, sizeof(buf), "%d", value % 256); dest->insert(0, buf); if (n < 3) dest->insert(0, 1, '.'); @@ -946,12 +945,12 @@ static url_host_type WriteHost(url_host* host, std::string* dest) { case HOST_TYPE_IPV6: { dest->reserve(41); *dest+= '['; - uint16_t* start = &host->value.ipv6[0]; - uint16_t* compress_pointer = + const uint16_t* start = &host->value.ipv6[0]; + const uint16_t* compress_pointer = FindLongestZeroSequence(start, 8); bool ignore0 = false; for (int n = 0; n <= 7; n++) { - uint16_t* piece = &host->value.ipv6[n]; + const uint16_t* piece = &host->value.ipv6[n]; if (ignore0 && *piece == 0) continue; else if (ignore0) @@ -962,8 +961,7 @@ static url_host_type WriteHost(url_host* host, std::string* dest) { continue; } char buf[5]; - char* buffer = buf; - snprintf(buffer, sizeof(buf), "%x", *piece); + snprintf(buf, sizeof(buf), "%x", *piece); *dest += buf; if (n < 7) *dest += ':'; @@ -980,16 +978,16 @@ static url_host_type WriteHost(url_host* host, std::string* dest) { return host->type; } -static bool ParseHost(std::string* input, +static bool ParseHost(const std::string& input, std::string* output, bool is_special, bool unicode = false) { - if (input->length() == 0) { + if (input.length() == 0) { output->clear(); return true; } url_host host{{""}, HOST_TYPE_DOMAIN}; - ParseHost(&host, input->c_str(), input->length(), is_special, unicode); + ParseHost(&host, input.c_str(), input.length(), is_special, unicode); if (host.type == HOST_TYPE_FAILED) return false; WriteHost(&host, output); @@ -1092,7 +1090,7 @@ static inline void HarvestContext(Environment* env, } // Single dot segment can be ".", "%2e", or "%2E" -static inline bool IsSingleDotSegment(std::string str) { +static inline bool IsSingleDotSegment(const std::string& str) { switch (str.size()) { case 1: return str == "."; @@ -1108,7 +1106,7 @@ static inline bool IsSingleDotSegment(std::string str) { // Double dot segment can be: // "..", ".%2e", ".%2E", "%2e.", "%2E.", // "%2e%2e", "%2E%2E", "%2e%2E", or "%2E%2e" -static inline bool IsDoubleDotSegment(std::string str) { +static inline bool IsDoubleDotSegment(const std::string& str) { switch (str.size()) { case 2: return str == ".."; @@ -1542,7 +1540,7 @@ void URL::Parse(const char* input, return; } url->flags |= URL_FLAGS_HAS_HOST; - if (!ParseHost(&buffer, &url->host, special)) { + if (!ParseHost(buffer, &url->host, special)) { url->flags |= URL_FLAGS_FAILED; return; } @@ -1569,7 +1567,7 @@ void URL::Parse(const char* input, return; } url->flags |= URL_FLAGS_HAS_HOST; - if (!ParseHost(&buffer, &url->host, special)) { + if (!ParseHost(buffer, &url->host, special)) { url->flags |= URL_FLAGS_FAILED; return; } @@ -1741,7 +1739,7 @@ void URL::Parse(const char* input, state = kPathStart; } else { std::string host; - if (!ParseHost(&buffer, &host, special)) { + if (!ParseHost(buffer, &host, special)) { url->flags |= URL_FLAGS_FAILED; return; } @@ -2104,8 +2102,7 @@ std::string URL::ToFilePath() const { #endif std::string decoded_path; for (const std::string& part : context_.path) { - std::string decoded; - PercentDecode(part.c_str(), part.length(), &decoded); + std::string decoded = PercentDecode(part.c_str(), part.length()); for (char& ch : decoded) { if (is_slash(ch)) { return ""; From 89b374623f969897a2e2499f646fa69c2fc1119c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 5 Dec 2017 13:37:38 +0100 Subject: [PATCH 26/29] src: move url internals into anonymous namespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This helps because `static` doesn’t work for C++ classes, but refactoring `url_host` into a proper C++ class seems the most reasonable soluation for the memory leak fixed by the next commit. PR-URL: https://github.com/nodejs/node/pull/17470 Reviewed-By: Timothy Gu --- src/node_url.cc | 117 ++++++++++++++++++++++++------------------------ 1 file changed, 58 insertions(+), 59 deletions(-) diff --git a/src/node_url.cc b/src/node_url.cc index b4b16399aab..21d8c810cb8 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -46,11 +46,13 @@ using v8::Value; namespace url { +namespace { + // https://url.spec.whatwg.org/#eof-code-point -static const char kEOL = -1; +const char kEOL = -1; // Used in ToUSVString(). -static const char16_t kUnicodeReplacementCharacter = 0xFFFD; +const char16_t kUnicodeReplacementCharacter = 0xFFFD; // https://url.spec.whatwg.org/#concept-host union url_host_value { @@ -103,7 +105,7 @@ enum url_error_cb_args { #define CHAR_TEST(bits, name, expr) \ template \ - static inline bool name(const T ch) { \ + inline bool name(const T ch) { \ static_assert(sizeof(ch) >= (bits) / 8, \ "Character must be wider than " #bits " bits"); \ return (expr); \ @@ -111,13 +113,13 @@ enum url_error_cb_args { #define TWO_CHAR_STRING_TEST(bits, name, expr) \ template \ - static inline bool name(const T ch1, const T ch2) { \ + inline bool name(const T ch1, const T ch2) { \ static_assert(sizeof(ch1) >= (bits) / 8, \ "Character must be wider than " #bits " bits"); \ return (expr); \ } \ template \ - static inline bool name(const std::basic_string& str) { \ + inline bool name(const std::basic_string& str) { \ static_assert(sizeof(str[0]) >= (bits) / 8, \ "Character must be wider than " #bits " bits"); \ return str.length() >= 2 && name(str[0], str[1]); \ @@ -146,7 +148,7 @@ CHAR_TEST(8, IsASCIIAlphanumeric, (IsASCIIDigit(ch) || IsASCIIAlpha(ch))) // https://infra.spec.whatwg.org/#ascii-lowercase template -static inline T ASCIILowercase(T ch) { +inline T ASCIILowercase(T ch) { return IsASCIIAlpha(ch) ? (ch | 0x20) : ch; } @@ -177,7 +179,7 @@ CHAR_TEST(16, IsUnicodeSurrogateTrail, (ch & 0x400) != 0) #undef CHAR_TEST #undef TWO_CHAR_STRING_TEST -static const char* hex[256] = { +const char* hex[256] = { "%00", "%01", "%02", "%03", "%04", "%05", "%06", "%07", "%08", "%09", "%0A", "%0B", "%0C", "%0D", "%0E", "%0F", "%10", "%11", "%12", "%13", "%14", "%15", "%16", "%17", @@ -212,7 +214,7 @@ static const char* hex[256] = { "%F8", "%F9", "%FA", "%FB", "%FC", "%FD", "%FE", "%FF" }; -static const uint8_t C0_CONTROL_ENCODE_SET[32] = { +const uint8_t C0_CONTROL_ENCODE_SET[32] = { // 00 01 02 03 04 05 06 07 0x01 | 0x02 | 0x04 | 0x08 | 0x10 | 0x20 | 0x40 | 0x80, // 08 09 0A 0B 0C 0D 0E 0F @@ -279,7 +281,7 @@ static const uint8_t C0_CONTROL_ENCODE_SET[32] = { 0x01 | 0x02 | 0x04 | 0x08 | 0x10 | 0x20 | 0x40 | 0x80 }; -static const uint8_t PATH_ENCODE_SET[32] = { +const uint8_t PATH_ENCODE_SET[32] = { // 00 01 02 03 04 05 06 07 0x01 | 0x02 | 0x04 | 0x08 | 0x10 | 0x20 | 0x40 | 0x80, // 08 09 0A 0B 0C 0D 0E 0F @@ -346,7 +348,7 @@ static const uint8_t PATH_ENCODE_SET[32] = { 0x01 | 0x02 | 0x04 | 0x08 | 0x10 | 0x20 | 0x40 | 0x80 }; -static const uint8_t USERINFO_ENCODE_SET[32] = { +const uint8_t USERINFO_ENCODE_SET[32] = { // 00 01 02 03 04 05 06 07 0x01 | 0x02 | 0x04 | 0x08 | 0x10 | 0x20 | 0x40 | 0x80, // 08 09 0A 0B 0C 0D 0E 0F @@ -413,7 +415,7 @@ static const uint8_t USERINFO_ENCODE_SET[32] = { 0x01 | 0x02 | 0x04 | 0x08 | 0x10 | 0x20 | 0x40 | 0x80 }; -static const uint8_t QUERY_ENCODE_SET[32] = { +const uint8_t QUERY_ENCODE_SET[32] = { // 00 01 02 03 04 05 06 07 0x01 | 0x02 | 0x04 | 0x08 | 0x10 | 0x20 | 0x40 | 0x80, // 08 09 0A 0B 0C 0D 0E 0F @@ -480,15 +482,15 @@ static const uint8_t QUERY_ENCODE_SET[32] = { 0x01 | 0x02 | 0x04 | 0x08 | 0x10 | 0x20 | 0x40 | 0x80 }; -static inline bool BitAt(const uint8_t a[], const uint8_t i) { +inline bool BitAt(const uint8_t a[], const uint8_t i) { return !!(a[i >> 3] & (1 << (i & 7))); } // Appends ch to str. If ch position in encode_set is set, the ch will // be percent-encoded then appended. -static inline void AppendOrEscape(std::string* str, - const unsigned char ch, - const uint8_t encode_set[]) { +inline void AppendOrEscape(std::string* str, + const unsigned char ch, + const uint8_t encode_set[]) { if (BitAt(encode_set, ch)) *str += hex[ch]; else @@ -496,7 +498,7 @@ static inline void AppendOrEscape(std::string* str, } template -static inline unsigned hex2bin(const T ch) { +inline unsigned hex2bin(const T ch) { if (ch >= '0' && ch <= '9') return ch - '0'; if (ch >= 'A' && ch <= 'F') @@ -544,7 +546,7 @@ inline std::string PercentDecode(const char* input, size_t len) { XX("ws:", 80) \ XX("wss:", 443) -static inline bool IsSpecial(std::string scheme) { +inline bool IsSpecial(std::string scheme) { #define XX(name, _) if (scheme == name) return true; SPECIALS(XX); #undef XX @@ -552,8 +554,7 @@ static inline bool IsSpecial(std::string scheme) { } // https://url.spec.whatwg.org/#start-with-a-windows-drive-letter -static inline bool StartsWithWindowsDriveLetter(const char* p, - const char* end) { +inline bool StartsWithWindowsDriveLetter(const char* p, const char* end) { const size_t length = end - p; return length >= 2 && IsWindowsDriveLetter(p[0], p[1]) && @@ -564,7 +565,7 @@ static inline bool StartsWithWindowsDriveLetter(const char* p, p[2] == '#'); } -static inline int NormalizePort(std::string scheme, int p) { +inline int NormalizePort(std::string scheme, int p) { #define XX(name, port) if (scheme == name && p == port) return -1; SPECIALS(XX); #undef XX @@ -572,7 +573,7 @@ static inline int NormalizePort(std::string scheme, int p) { } #if defined(NODE_HAVE_I18N_SUPPORT) -static inline bool ToUnicode(const std::string& input, std::string* output) { +inline bool ToUnicode(const std::string& input, std::string* output) { MaybeStackBuffer buf; if (i18n::ToUnicode(&buf, input.c_str(), input.length()) < 0) return false; @@ -580,7 +581,7 @@ static inline bool ToUnicode(const std::string& input, std::string* output) { return true; } -static inline bool ToASCII(const std::string& input, std::string* output) { +inline bool ToASCII(const std::string& input, std::string* output) { MaybeStackBuffer buf; if (i18n::ToASCII(&buf, input.c_str(), input.length()) < 0) return false; @@ -589,20 +590,18 @@ static inline bool ToASCII(const std::string& input, std::string* output) { } #else // Intentional non-ops if ICU is not present. -static inline bool ToUnicode(const std::string& input, std::string* output) { +inline bool ToUnicode(const std::string& input, std::string* output) { *output = input; return true; } -static inline bool ToASCII(const std::string& input, std::string* output) { +inline bool ToASCII(const std::string& input, std::string* output) { *output = input; return true; } #endif -static url_host_type ParseIPv6Host(url_host* host, - const char* input, - size_t length) { +url_host_type ParseIPv6Host(url_host* host, const char* input, size_t length) { url_host_type type = HOST_TYPE_FAILED; for (unsigned n = 0; n < 8; n++) host->value.ipv6[n] = 0; @@ -720,7 +719,7 @@ static url_host_type ParseIPv6Host(url_host* host, return type; } -static inline int64_t ParseNumber(const char* start, const char* end) { +inline int64_t ParseNumber(const char* start, const char* end) { unsigned R = 10; if (end - start >= 2 && start[0] == '0' && (start[1] | 0x20) == 'x') { start += 2; @@ -755,9 +754,7 @@ static inline int64_t ParseNumber(const char* start, const char* end) { return strtoll(start, nullptr, R); } -static url_host_type ParseIPv4Host(url_host* host, - const char* input, - size_t length) { +url_host_type ParseIPv4Host(url_host* host, const char* input, size_t length) { url_host_type type = HOST_TYPE_DOMAIN; const char* pointer = input; const char* mark = input; @@ -816,9 +813,9 @@ static url_host_type ParseIPv4Host(url_host* host, return type; } -static url_host_type ParseOpaqueHost(url_host* host, - const char* input, - size_t length) { +url_host_type ParseOpaqueHost(url_host* host, + const char* input, + size_t length) { url_host_type type = HOST_TYPE_OPAQUE; std::string output; output.reserve(length * 3); @@ -838,11 +835,11 @@ static url_host_type ParseOpaqueHost(url_host* host, return type; } -static url_host_type ParseHost(url_host* host, - const char* input, - size_t length, - bool is_special, - bool unicode = false) { +url_host_type ParseHost(url_host* host, + const char* input, + size_t length, + bool is_special, + bool unicode = false) { url_host_type type = HOST_TYPE_FAILED; const char* pointer = input; std::string decoded; @@ -895,7 +892,7 @@ static url_host_type ParseHost(url_host* host, // Locates the longest sequence of 0 segments in an IPv6 address // in order to use the :: compression when serializing template -static inline T* FindLongestZeroSequence(T* values, size_t len) { +inline T* FindLongestZeroSequence(T* values, size_t len) { T* start = values; T* end = start + len; T* result = nullptr; @@ -923,7 +920,7 @@ static inline T* FindLongestZeroSequence(T* values, size_t len) { return result; } -static url_host_type WriteHost(const url_host* host, std::string* dest) { +url_host_type WriteHost(const url_host* host, std::string* dest) { dest->clear(); switch (host->type) { case HOST_TYPE_DOMAIN: @@ -978,10 +975,10 @@ static url_host_type WriteHost(const url_host* host, std::string* dest) { return host->type; } -static bool ParseHost(const std::string& input, - std::string* output, - bool is_special, - bool unicode = false) { +bool ParseHost(const std::string& input, + std::string* output, + bool is_special, + bool unicode = false) { if (input.length() == 0) { output->clear(); return true; @@ -994,9 +991,9 @@ static bool ParseHost(const std::string& input, return true; } -static inline void Copy(Environment* env, - Local ary, - std::vector* vec) { +inline void Copy(Environment* env, + Local ary, + std::vector* vec) { const int32_t len = ary->Length(); if (len == 0) return; // nothing to copy @@ -1010,8 +1007,8 @@ static inline void Copy(Environment* env, } } -static inline Local Copy(Environment* env, - const std::vector& vec) { +inline Local Copy(Environment* env, + const std::vector& vec) { Isolate* isolate = env->isolate(); Local ary = Array::New(isolate, vec.size()); for (size_t n = 0; n < vec.size(); n++) @@ -1019,9 +1016,9 @@ static inline Local Copy(Environment* env, return ary; } -static inline void HarvestBase(Environment* env, - struct url_data* base, - Local base_obj) { +inline void HarvestBase(Environment* env, + struct url_data* base, + Local base_obj) { Local context = env->context(); Local flags = GET(env, base_obj, "flags"); if (flags->IsInt32()) @@ -1045,9 +1042,9 @@ static inline void HarvestBase(Environment* env, } } -static inline void HarvestContext(Environment* env, - struct url_data* context, - Local context_obj) { +inline void HarvestContext(Environment* env, + struct url_data* context, + Local context_obj) { Local flags = GET(env, context_obj, "flags"); if (flags->IsInt32()) { int32_t _flags = flags->Int32Value(env->context()).FromJust(); @@ -1090,7 +1087,7 @@ static inline void HarvestContext(Environment* env, } // Single dot segment can be ".", "%2e", or "%2E" -static inline bool IsSingleDotSegment(const std::string& str) { +inline bool IsSingleDotSegment(const std::string& str) { switch (str.size()) { case 1: return str == "."; @@ -1106,7 +1103,7 @@ static inline bool IsSingleDotSegment(const std::string& str) { // Double dot segment can be: // "..", ".%2e", ".%2E", "%2e.", "%2E.", // "%2e%2e", "%2E%2E", "%2e%2E", or "%2E%2e" -static inline bool IsDoubleDotSegment(const std::string& str) { +inline bool IsDoubleDotSegment(const std::string& str) { switch (str.size()) { case 2: return str == ".."; @@ -1133,13 +1130,15 @@ static inline bool IsDoubleDotSegment(const std::string& str) { } } -static inline void ShortenUrlPath(struct url_data* url) { +inline void ShortenUrlPath(struct url_data* url) { if (url->path.empty()) return; if (url->path.size() == 1 && url->scheme == "file:" && IsNormalizedWindowsDriveLetter(url->path[0])) return; url->path.pop_back(); } +} // anonymous namespace + void URL::Parse(const char* input, size_t len, enum url_parse_state state_override, From 9236dfe1ef3becfc02a18770ec5e74bf3b01bd99 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 5 Dec 2017 14:23:38 +0100 Subject: [PATCH 27/29] src: make url host a proper C++ class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Gives `URLHost` a proper destructor that clears memory depending on the type of the host (This fixes a memory leak) - Hide the host type enums and class layout as implementation details - Make the `Parse` methods members of `URLHost` - Turn `WriteHost` into a `ToString()` method on the `URLHost` class - Verify that at the beginning of a parse attempt, the type is set to “failed” - Remove a lot of `goto`s from the source code 🐢🚀 PR-URL: https://github.com/nodejs/node/pull/17470 Fixes: https://github.com/nodejs/node/issues/17448 Reviewed-By: Timothy Gu --- src/node_url.cc | 269 ++++++++++++++++++++++++++---------------------- 1 file changed, 147 insertions(+), 122 deletions(-) diff --git a/src/node_url.cc b/src/node_url.cc index 21d8c810cb8..1b90df3b92a 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -55,27 +55,71 @@ const char kEOL = -1; const char16_t kUnicodeReplacementCharacter = 0xFFFD; // https://url.spec.whatwg.org/#concept-host -union url_host_value { - std::string domain; - uint32_t ipv4; - uint16_t ipv6[8]; - std::string opaque; - ~url_host_value() {} -}; +class URLHost { + public: + ~URLHost(); + + void ParseIPv4Host(const char* input, size_t length, bool* is_ipv4); + void ParseIPv6Host(const char* input, size_t length); + void ParseOpaqueHost(const char* input, size_t length); + void ParseHost(const char* input, + size_t length, + bool is_special, + bool unicode = false); + + inline bool ParsingFailed() const { return type_ == HostType::H_FAILED; } + std::string ToString() const; + + private: + enum class HostType { + H_FAILED, + H_DOMAIN, + H_IPV4, + H_IPV6, + H_OPAQUE, + }; -enum url_host_type { - HOST_TYPE_FAILED = -1, - HOST_TYPE_DOMAIN = 0, - HOST_TYPE_IPV4 = 1, - HOST_TYPE_IPV6 = 2, - HOST_TYPE_OPAQUE = 3, -}; + union Value { + std::string domain; + uint32_t ipv4; + uint16_t ipv6[8]; + std::string opaque; + + ~Value() {} + Value() : ipv4(0) {} + }; -struct url_host { - url_host_value value; - enum url_host_type type; + Value value_; + HostType type_ = HostType::H_FAILED; + + // Setting the string members of the union with = is brittle because + // it relies on them being initialized to a state that requires no + // destruction of old data. + // For a long time, that worked well enough because ParseIPv6Host() happens + // to zero-fill `value_`, but that really is relying on standard library + // internals too much. + // These helpers are the easiest solution but we might want to consider + // just not forcing strings into an union. + inline void SetOpaque(std::string&& string) { + type_ = HostType::H_OPAQUE; + new(&value_.opaque) std::string(std::move(string)); + } + + inline void SetDomain(std::string&& string) { + type_ = HostType::H_DOMAIN; + new(&value_.domain) std::string(std::move(string)); + } }; +URLHost::~URLHost() { + using string = std::string; + switch (type_) { + case HostType::H_DOMAIN: value_.domain.~string(); break; + case HostType::H_OPAQUE: value_.opaque.~string(); break; + default: break; + } +} + #define ARGS(XX) \ XX(ARG_FLAGS) \ XX(ARG_PROTOCOL) \ @@ -601,11 +645,11 @@ inline bool ToASCII(const std::string& input, std::string* output) { } #endif -url_host_type ParseIPv6Host(url_host* host, const char* input, size_t length) { - url_host_type type = HOST_TYPE_FAILED; +void URLHost::ParseIPv6Host(const char* input, size_t length) { + CHECK_EQ(type_, HostType::H_FAILED); for (unsigned n = 0; n < 8; n++) - host->value.ipv6[n] = 0; - uint16_t* piece_pointer = &host->value.ipv6[0]; + value_.ipv6[n] = 0; + uint16_t* piece_pointer = &value_.ipv6[0]; uint16_t* last_piece = piece_pointer + 8; uint16_t* compress_pointer = nullptr; const char* pointer = input; @@ -614,7 +658,7 @@ url_host_type ParseIPv6Host(url_host* host, const char* input, size_t length) { char ch = pointer < end ? pointer[0] : kEOL; if (ch == ':') { if (length < 2 || pointer[1] != ':') - goto end; + return; pointer += 2; ch = pointer < end ? pointer[0] : kEOL; piece_pointer++; @@ -622,10 +666,10 @@ url_host_type ParseIPv6Host(url_host* host, const char* input, size_t length) { } while (ch != kEOL) { if (piece_pointer > last_piece) - goto end; + return; if (ch == ':') { if (compress_pointer != nullptr) - goto end; + return; pointer++; ch = pointer < end ? pointer[0] : kEOL; piece_pointer++; @@ -643,11 +687,11 @@ url_host_type ParseIPv6Host(url_host* host, const char* input, size_t length) { switch (ch) { case '.': if (len == 0) - goto end; + return; pointer -= len; ch = pointer < end ? pointer[0] : kEOL; if (piece_pointer > last_piece - 2) - goto end; + return; numbers_seen = 0; while (ch != kEOL) { value = 0xffffffff; @@ -656,22 +700,22 @@ url_host_type ParseIPv6Host(url_host* host, const char* input, size_t length) { pointer++; ch = pointer < end ? pointer[0] : kEOL; } else { - goto end; + return; } } if (!IsASCIIDigit(ch)) - goto end; + return; while (IsASCIIDigit(ch)) { unsigned number = ch - '0'; if (value == 0xffffffff) { value = number; } else if (value == 0) { - goto end; + return; } else { value = value * 10 + number; } if (value > 255) - goto end; + return; pointer++; ch = pointer < end ? pointer[0] : kEOL; } @@ -681,18 +725,18 @@ url_host_type ParseIPv6Host(url_host* host, const char* input, size_t length) { piece_pointer++; } if (numbers_seen != 4) - goto end; + return; continue; case ':': pointer++; ch = pointer < end ? pointer[0] : kEOL; if (ch == kEOL) - goto end; + return; break; case kEOL: break; default: - goto end; + return; } *piece_pointer = value; piece_pointer++; @@ -701,7 +745,7 @@ url_host_type ParseIPv6Host(url_host* host, const char* input, size_t length) { if (compress_pointer != nullptr) { swaps = piece_pointer - compress_pointer; piece_pointer = last_piece - 1; - while (piece_pointer != &host->value.ipv6[0] && swaps > 0) { + while (piece_pointer != &value_.ipv6[0] && swaps > 0) { uint16_t temp = *piece_pointer; uint16_t* swap_piece = compress_pointer + swaps - 1; *piece_pointer = *swap_piece; @@ -711,12 +755,9 @@ url_host_type ParseIPv6Host(url_host* host, const char* input, size_t length) { } } else if (compress_pointer == nullptr && piece_pointer != last_piece) { - goto end; + return; } - type = HOST_TYPE_IPV6; - end: - host->type = type; - return type; + type_ = HostType::H_IPV6; } inline int64_t ParseNumber(const char* start, const char* end) { @@ -754,8 +795,9 @@ inline int64_t ParseNumber(const char* start, const char* end) { return strtoll(start, nullptr, R); } -url_host_type ParseIPv4Host(url_host* host, const char* input, size_t length) { - url_host_type type = HOST_TYPE_DOMAIN; +void URLHost::ParseIPv4Host(const char* input, size_t length, bool* is_ipv4) { + CHECK_EQ(type_, HostType::H_FAILED); + *is_ipv4 = false; const char* pointer = input; const char* mark = input; const char* end = pointer + length; @@ -764,19 +806,19 @@ url_host_type ParseIPv4Host(url_host* host, const char* input, size_t length) { uint64_t numbers[4]; int tooBigNumbers = 0; if (length == 0) - goto end; + return; while (pointer <= end) { const char ch = pointer < end ? pointer[0] : kEOL; const int remaining = end - pointer - 1; if (ch == '.' || ch == kEOL) { if (++parts > 4) - goto end; + return; if (pointer == mark) - goto end; + return; int64_t n = ParseNumber(mark, pointer); if (n < 0) - goto end; + return; if (n > 255) { tooBigNumbers++; @@ -789,6 +831,7 @@ url_host_type ParseIPv4Host(url_host* host, const char* input, size_t length) { pointer++; } CHECK_GT(parts, 0); + *is_ipv4 = true; // If any but the last item in numbers is greater than 255, return failure. // If the last item in numbers is greater than or equal to @@ -796,97 +839,81 @@ url_host_type ParseIPv4Host(url_host* host, const char* input, size_t length) { if (tooBigNumbers > 1 || (tooBigNumbers == 1 && numbers[parts - 1] <= 255) || numbers[parts - 1] >= pow(256, static_cast(5 - parts))) { - type = HOST_TYPE_FAILED; - goto end; + return; } - type = HOST_TYPE_IPV4; + type_ = HostType::H_IPV4; val = numbers[parts - 1]; for (int n = 0; n < parts - 1; n++) { double b = 3 - n; val += numbers[n] * pow(256, b); } - host->value.ipv4 = val; - end: - host->type = type; - return type; + value_.ipv4 = val; } -url_host_type ParseOpaqueHost(url_host* host, - const char* input, - size_t length) { - url_host_type type = HOST_TYPE_OPAQUE; +void URLHost::ParseOpaqueHost(const char* input, size_t length) { + CHECK_EQ(type_, HostType::H_FAILED); std::string output; output.reserve(length * 3); for (size_t i = 0; i < length; i++) { const char ch = input[i]; if (ch != '%' && IsForbiddenHostCodePoint(ch)) { - type = HOST_TYPE_FAILED; - goto end; + return; } else { AppendOrEscape(&output, ch, C0_CONTROL_ENCODE_SET); } } - host->value.opaque = output; - end: - host->type = type; - return type; + SetOpaque(std::move(output)); } -url_host_type ParseHost(url_host* host, - const char* input, +void URLHost::ParseHost(const char* input, size_t length, bool is_special, - bool unicode = false) { - url_host_type type = HOST_TYPE_FAILED; + bool unicode) { + CHECK_EQ(type_, HostType::H_FAILED); const char* pointer = input; - std::string decoded; if (length == 0) - goto end; + return; if (pointer[0] == '[') { if (pointer[length - 1] != ']') - goto end; - return ParseIPv6Host(host, ++pointer, length - 2); + return; + return ParseIPv6Host(++pointer, length - 2); } if (!is_special) - return ParseOpaqueHost(host, input, length); + return ParseOpaqueHost(input, length); // First, we have to percent decode - decoded = PercentDecode(input, length); + std::string decoded = PercentDecode(input, length); // Then we have to punycode toASCII if (!ToASCII(decoded, &decoded)) - goto end; + return; // If any of the following characters are still present, we have to fail for (size_t n = 0; n < decoded.size(); n++) { const char ch = decoded[n]; if (IsForbiddenHostCodePoint(ch)) { - goto end; + return; } } // Check to see if it's an IPv4 IP address - type = ParseIPv4Host(host, decoded.c_str(), decoded.length()); - if (type == HOST_TYPE_IPV4 || type == HOST_TYPE_FAILED) - goto end; + bool is_ipv4; + ParseIPv4Host(decoded.c_str(), decoded.length(), &is_ipv4); + if (is_ipv4) + return; // If the unicode flag is set, run the result through punycode ToUnicode if (unicode && !ToUnicode(decoded, &decoded)) - goto end; + return; // It's not an IPv4 or IPv6 address, it must be a domain - type = HOST_TYPE_DOMAIN; - host->value.domain = decoded; - - end: - host->type = type; - return type; + SetDomain(std::move(decoded)); } // Locates the longest sequence of 0 segments in an IPv6 address @@ -920,59 +947,59 @@ inline T* FindLongestZeroSequence(T* values, size_t len) { return result; } -url_host_type WriteHost(const url_host* host, std::string* dest) { - dest->clear(); - switch (host->type) { - case HOST_TYPE_DOMAIN: - *dest = host->value.domain; +std::string URLHost::ToString() const { + std::string dest; + switch (type_) { + case HostType::H_DOMAIN: + return value_.domain; + break; + case HostType::H_OPAQUE: + return value_.opaque; break; - case HOST_TYPE_IPV4: { - dest->reserve(15); - uint32_t value = host->value.ipv4; + case HostType::H_IPV4: { + dest.reserve(15); + uint32_t value = value_.ipv4; for (int n = 0; n < 4; n++) { char buf[4]; snprintf(buf, sizeof(buf), "%d", value % 256); - dest->insert(0, buf); + dest.insert(0, buf); if (n < 3) - dest->insert(0, 1, '.'); + dest.insert(0, 1, '.'); value /= 256; } break; } - case HOST_TYPE_IPV6: { - dest->reserve(41); - *dest+= '['; - const uint16_t* start = &host->value.ipv6[0]; + case HostType::H_IPV6: { + dest.reserve(41); + dest += '['; + const uint16_t* start = &value_.ipv6[0]; const uint16_t* compress_pointer = FindLongestZeroSequence(start, 8); bool ignore0 = false; for (int n = 0; n <= 7; n++) { - const uint16_t* piece = &host->value.ipv6[n]; + const uint16_t* piece = &value_.ipv6[n]; if (ignore0 && *piece == 0) continue; else if (ignore0) ignore0 = false; if (compress_pointer == piece) { - *dest += n == 0 ? "::" : ":"; + dest += n == 0 ? "::" : ":"; ignore0 = true; continue; } char buf[5]; snprintf(buf, sizeof(buf), "%x", *piece); - *dest += buf; + dest += buf; if (n < 7) - *dest += ':'; + dest += ':'; } - *dest += ']'; + dest += ']'; break; } - case HOST_TYPE_OPAQUE: - *dest = host->value.opaque; - break; - case HOST_TYPE_FAILED: + case HostType::H_FAILED: break; } - return host->type; + return dest; } bool ParseHost(const std::string& input, @@ -983,11 +1010,11 @@ bool ParseHost(const std::string& input, output->clear(); return true; } - url_host host{{""}, HOST_TYPE_DOMAIN}; - ParseHost(&host, input.c_str(), input.length(), is_special, unicode); - if (host.type == HOST_TYPE_FAILED) + URLHost host; + host.ParseHost(input.c_str(), input.length(), is_special, unicode); + if (host.ParsingFailed()) return false; - WriteHost(&host, output); + *output = host.ToString(); return true; } @@ -2043,15 +2070,14 @@ static void DomainToASCII(const FunctionCallbackInfo& args) { CHECK(args[0]->IsString()); Utf8Value value(env->isolate(), args[0]); - url_host host{{""}, HOST_TYPE_DOMAIN}; + URLHost host; // Assuming the host is used for a special scheme. - ParseHost(&host, *value, value.length(), true); - if (host.type == HOST_TYPE_FAILED) { + host.ParseHost(*value, value.length(), true); + if (host.ParsingFailed()) { args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(env->isolate(), "")); return; } - std::string out; - WriteHost(&host, &out); + std::string out = host.ToString(); args.GetReturnValue().Set( String::NewFromUtf8(env->isolate(), out.c_str(), @@ -2064,15 +2090,14 @@ static void DomainToUnicode(const FunctionCallbackInfo& args) { CHECK(args[0]->IsString()); Utf8Value value(env->isolate(), args[0]); - url_host host{{""}, HOST_TYPE_DOMAIN}; + URLHost host; // Assuming the host is used for a special scheme. - ParseHost(&host, *value, value.length(), true, true); - if (host.type == HOST_TYPE_FAILED) { + host.ParseHost(*value, value.length(), true, true); + if (host.ParsingFailed()) { args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(env->isolate(), "")); return; } - std::string out; - WriteHost(&host, &out); + std::string out = host.ToString(); args.GetReturnValue().Set( String::NewFromUtf8(env->isolate(), out.c_str(), From e8a5f7bfb3a4b30b6313247fddd816cc92138193 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 5 Dec 2017 19:40:16 +0100 Subject: [PATCH 28/29] src: use correct OOB check for IPv6 parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `last_piece` pointed to the end of the 8×16 bit array, so `piece_pointer == last_piece` already means that the pointer is not writable any longer. Previously, this still worked most of the time but could result in an out-of-bounds-write. Also, rename `last_piece` to `buffer_end` to avoid this pitfall. PR-URL: https://github.com/nodejs/node/pull/17470 Reviewed-By: Timothy Gu --- src/node_url.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/node_url.cc b/src/node_url.cc index 1b90df3b92a..578c73c7f03 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -650,7 +650,7 @@ void URLHost::ParseIPv6Host(const char* input, size_t length) { for (unsigned n = 0; n < 8; n++) value_.ipv6[n] = 0; uint16_t* piece_pointer = &value_.ipv6[0]; - uint16_t* last_piece = piece_pointer + 8; + uint16_t* const buffer_end = piece_pointer + 8; uint16_t* compress_pointer = nullptr; const char* pointer = input; const char* end = pointer + length; @@ -665,7 +665,7 @@ void URLHost::ParseIPv6Host(const char* input, size_t length) { compress_pointer = piece_pointer; } while (ch != kEOL) { - if (piece_pointer > last_piece) + if (piece_pointer >= buffer_end) return; if (ch == ':') { if (compress_pointer != nullptr) @@ -690,7 +690,7 @@ void URLHost::ParseIPv6Host(const char* input, size_t length) { return; pointer -= len; ch = pointer < end ? pointer[0] : kEOL; - if (piece_pointer > last_piece - 2) + if (piece_pointer > buffer_end - 2) return; numbers_seen = 0; while (ch != kEOL) { @@ -744,7 +744,7 @@ void URLHost::ParseIPv6Host(const char* input, size_t length) { if (compress_pointer != nullptr) { swaps = piece_pointer - compress_pointer; - piece_pointer = last_piece - 1; + piece_pointer = buffer_end - 1; while (piece_pointer != &value_.ipv6[0] && swaps > 0) { uint16_t temp = *piece_pointer; uint16_t* swap_piece = compress_pointer + swaps - 1; @@ -754,7 +754,7 @@ void URLHost::ParseIPv6Host(const char* input, size_t length) { swaps--; } } else if (compress_pointer == nullptr && - piece_pointer != last_piece) { + piece_pointer != buffer_end) { return; } type_ = HostType::H_IPV6; From 7a055f1d399a74dc6ed59a6f3f70da2cf6a14847 Mon Sep 17 00:00:00 2001 From: Leko Date: Sat, 9 Dec 2017 02:29:34 +0900 Subject: [PATCH 29/29] test: improve crypto/random.js coverage - Call randomBytes with a non-function callback - Call randomFill with a non-function callback PR-URL: https://github.com/nodejs/node/pull/17555 Reviewed-By: Anatoli Papirovski Reviewed-By: Colin Ihrig Reviewed-By: Luigi Pinca --- test/parallel/test-crypto-random.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/parallel/test-crypto-random.js b/test/parallel/test-crypto-random.js index 320a4358ff3..3926eb38504 100644 --- a/test/parallel/test-crypto-random.js +++ b/test/parallel/test-crypto-random.js @@ -488,6 +488,7 @@ common.expectsError( ); [1, true, NaN, null, undefined, {}, []].forEach((i) => { + const buf = Buffer.alloc(10); common.expectsError( () => crypto.randomFillSync(i), { @@ -502,4 +503,22 @@ common.expectsError( type: TypeError } ); + common.expectsError( + () => crypto.randomFill(buf, 0, 10, i), + { + code: 'ERR_INVALID_CALLBACK', + type: TypeError, + message: 'Callback must be a function', + }); +}); + +[1, true, NaN, null, {}, []].forEach((i) => { + common.expectsError( + () => crypto.randomBytes(1, i), + { + code: 'ERR_INVALID_CALLBACK', + type: TypeError, + message: 'Callback must be a function', + } + ); });