From 56693fef4a6cbfa70bae78ffc579133cc15427a0 Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Wed, 22 Feb 2023 09:11:12 +0100 Subject: [PATCH 1/5] http: use listenerCount when adding noop event --- lib/_http_server.js | 5 +- .../test-http-socket-error-listeners.js | 84 +++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-http-socket-error-listeners.js diff --git a/lib/_http_server.js b/lib/_http_server.js index 9f396b440ae6e2..6ced6c84d55d72 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -825,7 +825,10 @@ const requestHeaderFieldsTooLargeResponse = Buffer.from( function socketOnError(e) { // Ignore further errors this.removeListener('error', socketOnError); - this.on('error', noop); + + if (this.listenerCount('error', noop) === 0) { + this.on('error', noop); + } if (!this.server.emit('clientError', e, this)) { // Caution must be taken to avoid corrupting the remote peer. diff --git a/test/parallel/test-http-socket-error-listeners.js b/test/parallel/test-http-socket-error-listeners.js new file mode 100644 index 00000000000000..5e712aa98ffc19 --- /dev/null +++ b/test/parallel/test-http-socket-error-listeners.js @@ -0,0 +1,84 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); +const net = require('net'); + +// This test sends an invalid character to a HTTP server and purposely +// does not handle clientError (even if it sets an event handler). +// +// The idea is to let the server emit multiple errors on the socket, +// mostly due to parsing error, and make sure they don't result +// in leaking event listeners. + +{ + let i = 0; + let socket; + const mustNotCall = common.mustNotCall.bind(null); + process.on('warning', mustNotCall); + + const server = http.createServer(common.mustNotCall()); + + server.on('clientError', common.mustCallAtLeast((err) => { + assert.strictEqual(err.code, 'HPE_INVALID_METHOD'); + assert.strictEqual(err.rawPacket.toString(), '*'); + + if (i === 20) { + socket.end(); + } else { + socket.write('*'); + i++; + } + }, 1)); + + server.listen(0, () => { + socket = net.createConnection({ port: server.address().port }); + + socket.on('connect', () => { + socket.write('*'); + }); + + socket.on('close', () => { + server.close(); + process.removeListener('warning', mustNotCall); + }); + }); +} + +{ + const valid = 'GET / HTTP/1.1\r\nHost:a\r\nContent-Length:1\r\n\r\n1\r\n'; + + const server = http.createServer((req, res) => { + const handler = common.mustCall(function handler(error) { + req.socket.removeListener('error', handler); + }, 2); + + req.on('end', handler); + req.socket.on('error', handler); + + res.removeHeader('date'); + res.writeHead(204); + res.end(); + }); + + server.listen(0, () => { + const client = net.createConnection({ host: '127.0.0.1', port: server.address().port }); + let data = Buffer.alloc(0); + + client.on('data', (chunk) => { + data = Buffer.concat([data, chunk]); + }); + + client.on('end', () => { + assert.strictEqual( + data.toString('utf-8'), + 'HTTP/1.1 204 No Content\r\nConnection: keep-alive\r\nKeep-Alive: timeout=5\r\n\r\n' + + 'HTTP/1.1 400 Bad Request\r\nConnection: close\r\n\r\n', + ); + server.close(); + }); + + client.write(valid + 'INVALID'); + }); +} From ff212f73c88fabb8dd2515edb83930f17327260e Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Thu, 23 Feb 2023 12:37:29 +0100 Subject: [PATCH 2/5] http: also warn the user when preventing the leak --- lib/_http_server.js | 15 +++++++++++++++ test/parallel/test-http-socket-error-listeners.js | 6 +++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index 6ced6c84d55d72..281ec46e9dc347 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -822,12 +822,27 @@ const requestHeaderFieldsTooLargeResponse = Buffer.from( `HTTP/1.1 431 ${STATUS_CODES[431]}\r\n` + 'Connection: close\r\n\r\n', 'ascii', ); + +function warnUnclosedSocket() { + if (warnUnclosedSocket.emitted) { + return; + } + + warnUnclosedSocket.emitted = true; + process.emitWarning( + 'An error event has already been emitted on the request. ' + + 'Please use the destroy method on the socket while handling a clientError event.', + ); +} + function socketOnError(e) { // Ignore further errors this.removeListener('error', socketOnError); if (this.listenerCount('error', noop) === 0) { this.on('error', noop); + } else { + warnUnclosedSocket(); } if (!this.server.emit('clientError', e, this)) { diff --git a/test/parallel/test-http-socket-error-listeners.js b/test/parallel/test-http-socket-error-listeners.js index 5e712aa98ffc19..1108ac7ee27f1d 100644 --- a/test/parallel/test-http-socket-error-listeners.js +++ b/test/parallel/test-http-socket-error-listeners.js @@ -1,3 +1,5 @@ +// Flags: --no-warnings + 'use strict'; const common = require('../common'); @@ -15,8 +17,7 @@ const net = require('net'); { let i = 0; let socket; - const mustNotCall = common.mustNotCall.bind(null); - process.on('warning', mustNotCall); + process.on('warning', common.mustCall()); const server = http.createServer(common.mustNotCall()); @@ -41,7 +42,6 @@ const net = require('net'); socket.on('close', () => { server.close(); - process.removeListener('warning', mustNotCall); }); }); } From 196ec78b4fdced94b32686a89746b2a31aab38b3 Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Mon, 27 Feb 2023 09:53:53 +0100 Subject: [PATCH 3/5] net: fix error message --- lib/_http_server.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index 281ec46e9dc347..b43101215e9330 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -830,7 +830,7 @@ function warnUnclosedSocket() { warnUnclosedSocket.emitted = true; process.emitWarning( - 'An error event has already been emitted on the request. ' + + 'An error event has already been emitted on the socket. ' + 'Please use the destroy method on the socket while handling a clientError event.', ); } From de99a80bc161f1279f989d4cf31fabb995db88cb Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Sun, 5 Mar 2023 22:42:58 +0100 Subject: [PATCH 4/5] http: remove useless test --- .../test-http-socket-error-listeners.js | 36 ------------------- 1 file changed, 36 deletions(-) diff --git a/test/parallel/test-http-socket-error-listeners.js b/test/parallel/test-http-socket-error-listeners.js index 1108ac7ee27f1d..26db347c6377f2 100644 --- a/test/parallel/test-http-socket-error-listeners.js +++ b/test/parallel/test-http-socket-error-listeners.js @@ -46,39 +46,3 @@ const net = require('net'); }); } -{ - const valid = 'GET / HTTP/1.1\r\nHost:a\r\nContent-Length:1\r\n\r\n1\r\n'; - - const server = http.createServer((req, res) => { - const handler = common.mustCall(function handler(error) { - req.socket.removeListener('error', handler); - }, 2); - - req.on('end', handler); - req.socket.on('error', handler); - - res.removeHeader('date'); - res.writeHead(204); - res.end(); - }); - - server.listen(0, () => { - const client = net.createConnection({ host: '127.0.0.1', port: server.address().port }); - let data = Buffer.alloc(0); - - client.on('data', (chunk) => { - data = Buffer.concat([data, chunk]); - }); - - client.on('end', () => { - assert.strictEqual( - data.toString('utf-8'), - 'HTTP/1.1 204 No Content\r\nConnection: keep-alive\r\nKeep-Alive: timeout=5\r\n\r\n' + - 'HTTP/1.1 400 Bad Request\r\nConnection: close\r\n\r\n', - ); - server.close(); - }); - - client.write(valid + 'INVALID'); - }); -} From e9b3a8dc23c14c2961f2297d9cad06a3bf764736 Mon Sep 17 00:00:00 2001 From: Paolo Insogna Date: Tue, 7 Mar 2023 21:44:11 +0100 Subject: [PATCH 5/5] http: linted code --- lib/_http_server.js | 3 ++- test/parallel/test-http-socket-error-listeners.js | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index b43101215e9330..838a1f13eac533 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -831,7 +831,8 @@ function warnUnclosedSocket() { warnUnclosedSocket.emitted = true; process.emitWarning( 'An error event has already been emitted on the socket. ' + - 'Please use the destroy method on the socket while handling a clientError event.', + 'Please use the destroy method on the socket while handling ' + + "a 'clientError' event.", ); } diff --git a/test/parallel/test-http-socket-error-listeners.js b/test/parallel/test-http-socket-error-listeners.js index 26db347c6377f2..f0d220a5d9337a 100644 --- a/test/parallel/test-http-socket-error-listeners.js +++ b/test/parallel/test-http-socket-error-listeners.js @@ -45,4 +45,3 @@ const net = require('net'); }); }); } -