From 271c54cf24771fbffc9077f68f3aaa672e678905 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 20 Aug 2019 16:23:25 +0200 Subject: [PATCH 1/3] http: emit ERR_STREAM_DESTROYED if the socket is destroyed The pipeline utility assumes that a stream is going to error if it could not be written to it. If an http.OutgoingMessage was piped after the underlining socket had been destroyed, the whole pipeline would not be teared down, resulting in a file descriptor and memory leak. --- lib/_http_outgoing.js | 35 ++++++-- .../test-http-destroyed-socket-write2.js | 9 +- .../test-http-outgoing-socket-destroyed.js | 90 +++++++++++++++++++ 3 files changed, 128 insertions(+), 6 deletions(-) create mode 100644 test/parallel/test-http-outgoing-socket-destroyed.js diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 32a51d120bad2b..4aa1ef6c364016 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -47,6 +47,7 @@ const { ERR_METHOD_NOT_IMPLEMENTED, ERR_STREAM_CANNOT_PIPE, ERR_STREAM_ALREADY_FINISHED, + ERR_STREAM_DESTROYED, ERR_STREAM_WRITE_AFTER_END }, hideStackFrames @@ -57,6 +58,7 @@ const HIGH_WATER_MARK = getDefaultHighWaterMark(); const { CRLF, debug } = common; const kIsCorked = Symbol('isCorked'); +const kErrorEmitted = Symbol('errorEmitted'); const RE_CONN_CLOSE = /(?:^|\W)close(?:$|\W)/i; const RE_TE_CHUNKED = common.chunkExpression; @@ -284,17 +286,40 @@ OutgoingMessage.prototype._send = function _send(data, encoding, callback) { OutgoingMessage.prototype._writeRaw = _writeRaw; function _writeRaw(data, encoding, callback) { const conn = this.socket; - if (conn && conn.destroyed) { - // The socket was destroyed. If we're still trying to write to it, - // then we haven't gotten the 'close' event yet. - return false; - } if (typeof encoding === 'function') { callback = encoding; encoding = null; } + if (conn && conn.destroyed) { + const err = new ERR_STREAM_DESTROYED('write'); + if (callback) { + callback(err); + } + + // We check if there is a listener for 'error' as a backward-compatible fix. + // TODO(mcollina): remove in a follow-up, semver-major PR. + if (this.listenerCount('error') > 0 && + this[kErrorEmitted] === undefined && + !conn._writableState.errorEmitted && + !conn._hadError) { // _hadError could be set by ClientRequest. + + // TODO(mcollina): clean up kErrorEmitted and _hadError + // they can likely be the same state variable. + this[kErrorEmitted] = true; + + // If the message is a ClientRequest, we avoid emitting 'error' more + // than once. + conn._hadError = true; + + process.nextTick(writeAfterEndNT, this, err); + } + // The socket was destroyed. If we're still trying to write to it, + // then we haven't gotten the 'close' event yet. + return false; + } + if (conn && conn._httpMessage === this && conn.writable) { // There might be pending data in the this.output buffer. if (this.outputData.length) { diff --git a/test/parallel/test-http-destroyed-socket-write2.js b/test/parallel/test-http-destroyed-socket-write2.js index 551cea19829d93..3cb2c793b23463 100644 --- a/test/parallel/test-http-destroyed-socket-write2.js +++ b/test/parallel/test-http-destroyed-socket-write2.js @@ -41,7 +41,10 @@ server.listen(0, function() { }); function write() { - req.write('hello', function() { + req.write('hello', function(err) { + if (err) { + return; + } setImmediate(write); }); } @@ -52,6 +55,10 @@ server.listen(0, function() { case 'ECONNRESET': break; + // This is also ok, depends on the operating system + case 'ERR_STREAM_DESTROYED': + break; + // On Windows, this sometimes manifests as ECONNABORTED case 'ECONNABORTED': break; diff --git a/test/parallel/test-http-outgoing-socket-destroyed.js b/test/parallel/test-http-outgoing-socket-destroyed.js new file mode 100644 index 00000000000000..61ede110a52e4f --- /dev/null +++ b/test/parallel/test-http-outgoing-socket-destroyed.js @@ -0,0 +1,90 @@ +'use strict'; + +const common = require('../common'); +const { createServer, request } = require('http'); + +{ + const server = createServer((req, res) => { + server.close(); + + req.socket.destroy(); + + res.write('hello', common.expectsError({ + code: 'ERR_STREAM_DESTROYED' + })); + }); + + server.listen(0, common.mustCall(() => { + const req = request({ + host: 'localhost', + port: server.address().port + }); + + req.on('response', common.mustNotCall()); + req.on('error', common.expectsError({ + code: 'ECONNRESET' + })); + + req.end(); + })); +} + +{ + const server = createServer((req, res) => { + server.close(); + + req.socket.destroy(); + + const onError = common.expectsError({ + code: 'ERR_STREAM_DESTROYED' + }); + + res.on('error', (err) => onError(err)); + res.write('hello'); + }); + + server.listen(0, common.mustCall(() => { + const req = request({ + host: 'localhost', + port: server.address().port + }); + + req.on('response', common.mustNotCall()); + req.on('error', common.mustCall()); + + req.end(); + })); +} + +{ + const server = createServer((req, res) => { + res.write('hello'); + req.resume(); + + const onError = common.expectsError({ + code: 'ERR_STREAM_DESTROYED' + }); + + res.on('close', () => { + res.write('world'); + }); + + res.on('error', (err) => { + onError(err); + server.close(); + }); + }); + + server.listen(0, common.mustCall(() => { + const req = request({ + host: 'localhost', + port: server.address().port + }); + + req.on('response', common.mustCall(() => { + req.socket.destroy(); + })); + + req.end(); + })); +} From 64fd3834cb5f4a5694cbe96bb49cc0dfb2507bc7 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 23 Aug 2019 11:29:30 +0200 Subject: [PATCH 2/3] http: make writeContinue() emit ERR_STREAM_WRITE_AFTER_END --- lib/_http_server.js | 25 +++++++++++++++-- ...est-http-write-continue-write-after-end.js | 28 +++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-http-write-continue-write-after-end.js diff --git a/lib/_http_server.js b/lib/_http_server.js index 6dc29c855e414c..af0ec345acebda 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -46,14 +46,16 @@ const { } = require('internal/http'); const { defaultTriggerAsyncIdScope, - getOrSetAsyncId + getOrSetAsyncId, + symbols: { async_id_symbol } } = require('internal/async_hooks'); const { IncomingMessage } = require('_http_incoming'); const { ERR_HTTP_HEADERS_SENT, ERR_HTTP_INVALID_STATUS_CODE, ERR_INVALID_ARG_TYPE, - ERR_INVALID_CHAR + ERR_INVALID_CHAR, + ERR_STREAM_WRITE_AFTER_END } = require('internal/errors').codes; const Buffer = require('buffer').Buffer; const { @@ -216,10 +218,29 @@ ServerResponse.prototype.detachSocket = function detachSocket(socket) { }; ServerResponse.prototype.writeContinue = function writeContinue(cb) { + if (this.finished) { + const err = new ERR_STREAM_WRITE_AFTER_END(); + const triggerAsyncId = this.socket ? + this.socket[async_id_symbol] : undefined; + defaultTriggerAsyncIdScope(triggerAsyncId, + process.nextTick, + writeAfterEndNT, + this, + err, + cb); + + return true; + } + this._writeRaw(`HTTP/1.1 100 Continue${CRLF}${CRLF}`, 'ascii', cb); this._sent100 = true; }; +function writeAfterEndNT(msg, err, callback) { + msg.emit('error', err); + if (callback) callback(err); +} + ServerResponse.prototype.writeProcessing = function writeProcessing(cb) { this._writeRaw(`HTTP/1.1 102 Processing${CRLF}${CRLF}`, 'ascii', cb); }; diff --git a/test/parallel/test-http-write-continue-write-after-end.js b/test/parallel/test-http-write-continue-write-after-end.js new file mode 100644 index 00000000000000..77d9351f57974f --- /dev/null +++ b/test/parallel/test-http-write-continue-write-after-end.js @@ -0,0 +1,28 @@ +'use strict'; + +const common = require('../common'); +const { createServer, request } = require('http'); + +{ + const server = createServer((req, res) => { + server.close(); + + res.end(); + res.writeContinue(); + + res.on('error', common.expectsError({ + code: 'ERR_STREAM_WRITE_AFTER_END' + })); + }); + + server.listen(0, common.mustCall(() => { + const req = request({ + host: 'localhost', + port: server.address().port + }); + + req.on('response', common.mustCall((res) => res.resume())); + + req.end(); + })); +} From 2816b2db482fcfa7cced44cda8d51af2c4856853 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 26 Aug 2019 15:13:25 +0200 Subject: [PATCH 3/3] http: make writeProcessing() emit ERR_STREAM_WRITE_AFTER_END --- lib/_http_server.js | 14 ++++++++++ ...t-http-write-processing-write-after-end.js | 28 +++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 test/parallel/test-http-write-processing-write-after-end.js diff --git a/lib/_http_server.js b/lib/_http_server.js index af0ec345acebda..41b287745c1148 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -242,6 +242,20 @@ function writeAfterEndNT(msg, err, callback) { } ServerResponse.prototype.writeProcessing = function writeProcessing(cb) { + if (this.finished) { + const err = new ERR_STREAM_WRITE_AFTER_END(); + const triggerAsyncId = this.socket ? + this.socket[async_id_symbol] : undefined; + defaultTriggerAsyncIdScope(triggerAsyncId, + process.nextTick, + writeAfterEndNT, + this, + err, + cb); + + return true; + } + this._writeRaw(`HTTP/1.1 102 Processing${CRLF}${CRLF}`, 'ascii', cb); }; diff --git a/test/parallel/test-http-write-processing-write-after-end.js b/test/parallel/test-http-write-processing-write-after-end.js new file mode 100644 index 00000000000000..013268fdca4eae --- /dev/null +++ b/test/parallel/test-http-write-processing-write-after-end.js @@ -0,0 +1,28 @@ +'use strict'; + +const common = require('../common'); +const { createServer, request } = require('http'); + +{ + const server = createServer((req, res) => { + server.close(); + + res.end(); + res.writeProcessing(); + + res.on('error', common.expectsError({ + code: 'ERR_STREAM_WRITE_AFTER_END' + })); + }); + + server.listen(0, common.mustCall(() => { + const req = request({ + host: 'localhost', + port: server.address().port + }); + + req.on('response', common.mustCall((res) => res.resume())); + + req.end(); + })); +}