From dd2e3edc1fe226b36722a76c2457e4de12dd3f55 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Fri, 20 Nov 2020 22:48:19 +0100 Subject: [PATCH 1/9] net: require two events to destroy a socket Closing a TCP connection requires 2 FIN/ACK exchanges, so we should wait for both events before destroying a socket Fixes: https://github.com/nodejs/node/issues/36180 --- lib/net.js | 14 ++++++++++++-- test/parallel/test-http-should-keep-alive.js | 2 +- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/net.js b/lib/net.js index 4b3b96c8cd2736..6c523fcd214708 100644 --- a/lib/net.js +++ b/lib/net.js @@ -631,10 +631,20 @@ Socket.prototype.destroySoon = function() { if (this.writable) this.end(); - if (this.writableFinished) + if (this.writableFinished && this.readableEnded) this.destroy(); else - this.once('finish', this.destroy); + this.once('finish', () => { + // If we destroy the socket before we have received an incoming EOF + // the remote might still be sending data that will trigger a RST + // from this host once it is received + // Closing a TCP socket requires two FIN/ACK exchanges, one in every + // direction - this means two events + if (this.readableEnded) + this.destroy(); + else + this.once('end', this.destroy); + }); }; diff --git a/test/parallel/test-http-should-keep-alive.js b/test/parallel/test-http-should-keep-alive.js index 038af47ee4db97..33fa46187040d6 100644 --- a/test/parallel/test-http-should-keep-alive.js +++ b/test/parallel/test-http-should-keep-alive.js @@ -49,7 +49,7 @@ const countdown = new Countdown(SHOULD_KEEP_ALIVE.length, () => server.close()); const getCountdownIndex = () => SERVER_RESPONSES.length - countdown.remaining; const server = net.createServer(function(socket) { - socket.write(SERVER_RESPONSES[getCountdownIndex()]); + socket.end(SERVER_RESPONSES[getCountdownIndex()]); }).listen(0, function() { function makeRequest() { const req = http.get({ port: server.address().port }, function(res) { From e6699c64343ae2f52f3611c8e7d973ace6ee00dd Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Sun, 22 Nov 2020 00:05:00 +0100 Subject: [PATCH 2/9] net: replace destroySoon with socket.end() As per @ronag suggestion, destroySoon is considered obsolete, all of its functionnality having been implemented by Writeable.end() Refs: https://github.com/nodejs/node/pull/36205 --- lib/_http_client.js | 6 +----- lib/_http_server.js | 6 +----- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 885016bf4856db..ab1d31afd2bc23 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -709,11 +709,7 @@ function responseOnEnd() { if (!req.shouldKeepAlive) { if (socket.writable) { - debug('AGENT socket.destroySoon()'); - if (typeof socket.destroySoon === 'function') - socket.destroySoon(); - else - socket.end(); + socket.end(); } assert(!socket.writable); } else if (req.finished && !this.aborted) { diff --git a/lib/_http_server.js b/lib/_http_server.js index 7343d5f52791b9..ebb685bfae9579 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -814,11 +814,7 @@ function resOnFinish(req, res, socket, state, server) { process.nextTick(emitCloseNT, res); if (res._last) { - if (typeof socket.destroySoon === 'function') { - socket.destroySoon(); - } else { - socket.end(); - } + socket.end(); } else if (state.outgoing.length === 0) { if (server.keepAliveTimeout && typeof socket.setTimeout === 'function') { socket.setTimeout(server.keepAliveTimeout); From 8f0e26c42f7a2193ddf35f1c052ca5a8ca4bd941 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Sun, 22 Nov 2020 18:40:29 +0100 Subject: [PATCH 3/9] net: close http connections with stream.end() Adopt suggestions by @mcollina and @ronag Revert destroySoon to its original state, pending deprecation Remove useless condition on stream Refs: https://github.com/nodejs/node/pull/36205 --- lib/_http_client.js | 4 +--- lib/net.js | 16 +++------------- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index ab1d31afd2bc23..11e70b8e54341f 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -708,9 +708,7 @@ function responseOnEnd() { req._ended = true; if (!req.shouldKeepAlive) { - if (socket.writable) { - socket.end(); - } + socket.end(); assert(!socket.writable); } else if (req.finished && !this.aborted) { // We can assume `req.finished` means all data has been written since: diff --git a/lib/net.js b/lib/net.js index 6c523fcd214708..c2255eff816700 100644 --- a/lib/net.js +++ b/lib/net.js @@ -627,24 +627,14 @@ function onReadableStreamEnd() { } -Socket.prototype.destroySoon = function() { +Socket.prototype.destroySoon = function () { if (this.writable) this.end(); - if (this.writableFinished && this.readableEnded) + if (this.writableFinished) this.destroy(); else - this.once('finish', () => { - // If we destroy the socket before we have received an incoming EOF - // the remote might still be sending data that will trigger a RST - // from this host once it is received - // Closing a TCP socket requires two FIN/ACK exchanges, one in every - // direction - this means two events - if (this.readableEnded) - this.destroy(); - else - this.once('end', this.destroy); - }); + this.once('finish', this.destroy); }; From f780542261f4fa9bcb933d318ff924058fb7030b Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Sun, 22 Nov 2020 19:33:36 +0100 Subject: [PATCH 4/9] http: lint the PR eslint net.js Refs: https://github.com/nodejs/node/pull/36205 --- lib/net.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/net.js b/lib/net.js index c2255eff816700..4b3b96c8cd2736 100644 --- a/lib/net.js +++ b/lib/net.js @@ -627,7 +627,7 @@ function onReadableStreamEnd() { } -Socket.prototype.destroySoon = function () { +Socket.prototype.destroySoon = function() { if (this.writable) this.end(); From 3e1ba08b6b83463fd2e888b090f824a1e5fafcfa Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Mon, 23 Nov 2020 11:00:35 +0100 Subject: [PATCH 5/9] net: add a unit test for res.end This unit test exploits the fact that the server will stay in half-closed state after `res.end()` Thus, the parser will receive the second request and will parse it, triggering another call of the connection handler - but it won't be able to respond since the connection has been closed in this direction The bugged version will force the socket closing and won't receive the second request Refs: https://github.com/nodejs/node/pull/36205 --- test/parallel/test-https-end-rst.js | 31 +++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 test/parallel/test-https-end-rst.js diff --git a/test/parallel/test-https-end-rst.js b/test/parallel/test-https-end-rst.js new file mode 100644 index 00000000000000..5d5f566693de28 --- /dev/null +++ b/test/parallel/test-https-end-rst.js @@ -0,0 +1,31 @@ +'use strict'; +const common = require('../common'); + +const http = require('http'); +const net = require('net'); + +const data = 'hello'; +const server = http.createServer({}, common.mustCallAtLeast((req, res) => { + res.setHeader('content-length', data.length); + res.end(data); +}, 2)); + +server.listen(0, function() { + const options = { + host: '127.0.0.1', + port: server.address().port, + allowHalfOpen: true + }; + const socket = net.connect(options, common.mustCall(() => { + socket.on('ready', common.mustCall(() => { + socket.write('GET /\n\n'); + setTimeout(() => { + socket.write('GET /\n\n'); + setTimeout(() => { + socket.destroy(); + server.close(); + }, common.platformTimeout(10)); + }, common.platformTimeout(10)); + })); + })); +}); From 1559f5ca312c5c232b18362da53f82a93ecfdef2 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Mon, 23 Nov 2020 11:07:33 +0100 Subject: [PATCH 6/9] http: replace destroySoon with socket.end Use events in the unit test --- test/parallel/test-https-end-rst.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-https-end-rst.js b/test/parallel/test-https-end-rst.js index 5d5f566693de28..ab3c49dee6ced2 100644 --- a/test/parallel/test-https-end-rst.js +++ b/test/parallel/test-https-end-rst.js @@ -19,13 +19,13 @@ server.listen(0, function() { const socket = net.connect(options, common.mustCall(() => { socket.on('ready', common.mustCall(() => { socket.write('GET /\n\n'); - setTimeout(() => { + socket.once('data', common.mustCall(() => { socket.write('GET /\n\n'); - setTimeout(() => { + setTimeout(common.mustCall(() => { socket.destroy(); server.close(); - }, common.platformTimeout(10)); - }, common.platformTimeout(10)); + }), common.platformTimeout(10)); + })); })); })); }); From a1b606c68dfcc9f791f96421ac07655aa26bc50c Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Wed, 9 Dec 2020 12:46:50 +0100 Subject: [PATCH 7/9] tls: support active closing for TLS sockets After discussions with @ronag and @lpinca, the best solution to the TLS RST problem is the one that limits the new behaviour to TLSSockets This is complementary to @vtjnash solution which allows the client to ignore this (somewhat normal) RST Both are needed, since both ends of Node.js TLS code must interact with other TLS implementations Refs: https://github.com/nodejs/node/pull/36205 --- lib/_http_server.js | 6 +++++- lib/_tls_wrap.js | 21 ++++++++++++++++++ test/parallel/test-https-end-rst.js | 33 ++++++++++++++++++----------- 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index ebb685bfae9579..7343d5f52791b9 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -814,7 +814,11 @@ function resOnFinish(req, res, socket, state, server) { process.nextTick(emitCloseNT, res); if (res._last) { - socket.end(); + if (typeof socket.destroySoon === 'function') { + socket.destroySoon(); + } else { + socket.end(); + } } else if (state.outgoing.length === 0) { if (server.keepAliveTimeout && typeof socket.setTimeout === 'function') { socket.setTimeout(server.keepAliveTimeout); diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 929114d74ad6b6..91b08aaeb11801 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -571,6 +571,27 @@ tls_wrap.TLSWrap.prototype.close = function close(cb) { return this._parent.close(done); }; +TLSSocket.prototype.destroySoon = function() { + if (this.writable) + this.end(); + + if (this.writableFinished && this.readableEnded) + this.destroy(); + else + this.once('finish', () => { + // TLS needs special handling when the server initiates the closing + // of the connection because a TLS-compliant client will send more data + // after receiving the server FIN + // If the server immediately destroys its socket, this data will trigger a + // RST packet in response + // https://github.com/nodejs/node/issues/36180 + if (this.readableEnded) + this.destroy(); + else + this.once('end', this.destroy); + }); +}; + TLSSocket.prototype.disableRenegotiation = function disableRenegotiation() { this[kDisableRenegotiation] = true; }; diff --git a/test/parallel/test-https-end-rst.js b/test/parallel/test-https-end-rst.js index ab3c49dee6ced2..6249290b7b38e7 100644 --- a/test/parallel/test-https-end-rst.js +++ b/test/parallel/test-https-end-rst.js @@ -1,11 +1,20 @@ 'use strict'; const common = require('../common'); -const http = require('http'); -const net = require('net'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const https = require('https'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); + +const opt = { + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem') +}; const data = 'hello'; -const server = http.createServer({}, common.mustCallAtLeast((req, res) => { +const server = https.createServer(opt, common.mustCallAtLeast((req, res) => { res.setHeader('content-length', data.length); res.end(data); }, 2)); @@ -14,18 +23,18 @@ server.listen(0, function() { const options = { host: '127.0.0.1', port: server.address().port, + rejectUnauthorized: false, + ALPNProtocols: ['http/1.1'], allowHalfOpen: true }; - const socket = net.connect(options, common.mustCall(() => { - socket.on('ready', common.mustCall(() => { + const socket = tls.connect(options, common.mustCall(() => { + socket.write('GET /\n\n'); + socket.once('data', common.mustCall(() => { socket.write('GET /\n\n'); - socket.once('data', common.mustCall(() => { - socket.write('GET /\n\n'); - setTimeout(common.mustCall(() => { - socket.destroy(); - server.close(); - }), common.platformTimeout(10)); - })); + setTimeout(common.mustCall(() => { + socket.destroy(); + server.close(); + }), common.platformTimeout(10)); })); })); }); From 6038c95194fd361d0e91e8b1df1f9e1c989f95cc Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Wed, 9 Dec 2020 12:52:50 +0100 Subject: [PATCH 8/9] remove useless change --- lib/_http_client.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 11e70b8e54341f..885016bf4856db 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -708,7 +708,13 @@ function responseOnEnd() { req._ended = true; if (!req.shouldKeepAlive) { - socket.end(); + if (socket.writable) { + debug('AGENT socket.destroySoon()'); + if (typeof socket.destroySoon === 'function') + socket.destroySoon(); + else + socket.end(); + } assert(!socket.writable); } else if (req.finished && !this.aborted) { // We can assume `req.finished` means all data has been written since: From 0dfd1f2cf9ff64253132c2c93be8235f48af6619 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Wed, 9 Dec 2020 13:10:55 +0100 Subject: [PATCH 9/9] tls: use Writable.end (@ronag) --- lib/_tls_wrap.js | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 91b08aaeb11801..85f071aaf9bd4e 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -46,6 +46,7 @@ const tls = require('tls'); const common = require('_tls_common'); const JSStreamSocket = require('internal/js_stream_socket'); const { Buffer } = require('buffer'); +const Writable = require('internal/streams/writable'); let debug = require('internal/util/debuglog').debuglog('tls', (fn) => { debug = fn; }); @@ -571,26 +572,13 @@ tls_wrap.TLSWrap.prototype.close = function close(cb) { return this._parent.close(done); }; -TLSSocket.prototype.destroySoon = function() { - if (this.writable) - this.end(); - - if (this.writableFinished && this.readableEnded) - this.destroy(); - else - this.once('finish', () => { - // TLS needs special handling when the server initiates the closing - // of the connection because a TLS-compliant client will send more data - // after receiving the server FIN - // If the server immediately destroys its socket, this data will trigger a - // RST packet in response - // https://github.com/nodejs/node/issues/36180 - if (this.readableEnded) - this.destroy(); - else - this.once('end', this.destroy); - }); -}; +// TLS needs special handling when the server initiates the closing +// of the connection because a TLS-compliant client will send more data +// after receiving the server FIN +// If the server immediately destroys its socket, this data will trigger a +// RST packet in response +// https://github.com/nodejs/node/issues/36180 +TLSSocket.prototype.destroySoon = Writable.prototype.end; TLSSocket.prototype.disableRenegotiation = function disableRenegotiation() { this[kDisableRenegotiation] = true;