From 4fe853348257b4ff8a1ab7e389981e3e68b59acb Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 16 Jan 2021 18:17:57 +0100 Subject: [PATCH 1/7] http: fix ClientRequest unhandled errors ClientRequest could someone cause an unhandled error from socket. Fixes: https://github.com/nodejs/node/issues/36931 --- lib/_http_agent.js | 8 +++++--- lib/_http_client.js | 2 +- test/parallel/test-http-client-abort3.js | 12 ++++++++++++ 3 files changed, 18 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-http-client-abort3.js diff --git a/lib/_http_agent.js b/lib/_http_agent.js index ba165b42b1dc3a..483da6b53e7802 100644 --- a/lib/_http_agent.js +++ b/lib/_http_agent.js @@ -57,6 +57,8 @@ const { const { once } = require('internal/util'); const { validateNumber, validateOneOf } = require('internal/validators'); +function nop() {} + const kOnKeylog = Symbol('onkeylog'); const kRequestOptions = Symbol('requestOptions'); const kRequestAsyncResource = Symbol('requestAsyncResource'); @@ -131,7 +133,7 @@ function Agent(options) { // and could cause unhandled exception. if (!socket.writable) { - socket.destroy(); + socket.on('error', nop).destroy(); return; } @@ -159,7 +161,7 @@ function Agent(options) { // the freeSockets pool, but only if we're allowed to do so. const req = socket._httpMessage; if (!req || !req.shouldKeepAlive || !this.keepAlive) { - socket.destroy(); + socket.on('error', nop).destroy(); return; } @@ -173,7 +175,7 @@ function Agent(options) { count > this.maxSockets || freeLen >= this.maxFreeSockets || !this.keepSocketAlive(socket)) { - socket.destroy(); + socket.on('error', nop).destroy(); return; } diff --git a/lib/_http_client.js b/lib/_http_client.js index 93dbf9d63e9f4b..35f559b0456ef2 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -801,7 +801,7 @@ function onSocketNT(req, socket, err) { req.emit('close'); } - if (!err && req.agent) { + if (!err && req.agent && !socket.destroyed) { socket?.emit('free'); } else if (socket) { finished(socket.destroy(err || req[kError]), (er) => { diff --git a/test/parallel/test-http-client-abort3.js b/test/parallel/test-http-client-abort3.js new file mode 100644 index 00000000000000..f84384ef8d9a62 --- /dev/null +++ b/test/parallel/test-http-client-abort3.js @@ -0,0 +1,12 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +const req = http.get('http://[2604:1380:45f1:3f00::1]:4002'); + +req.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'EHOSTUNREACH'); +})); +req.abort(); From 96e1c4fe9310fa3caef3a0e03d64062222cca033 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 16 Jan 2021 18:20:22 +0100 Subject: [PATCH 2/7] fixup --- lib/_http_client.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 35f559b0456ef2..1c4cda9e585952 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -801,13 +801,15 @@ function onSocketNT(req, socket, err) { req.emit('close'); } - if (!err && req.agent && !socket.destroyed) { - socket?.emit('free'); - } else if (socket) { - finished(socket.destroy(err || req[kError]), (er) => { - _destroy(req, er || err); - }); - return; + if (socket) { + if (!err && req.agent && !socket.destroyed) { + socket.emit('free'); + } else { + finished(socket.destroy(err || req[kError]), (er) => { + _destroy(req, er || err); + }); + return; + } } _destroy(req, err || req[kError]); From a077054985e661a1e552f9322ea0880522484809 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 16 Jan 2021 20:35:43 +0100 Subject: [PATCH 3/7] fixup --- lib/_http_agent.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/_http_agent.js b/lib/_http_agent.js index 483da6b53e7802..ba165b42b1dc3a 100644 --- a/lib/_http_agent.js +++ b/lib/_http_agent.js @@ -57,8 +57,6 @@ const { const { once } = require('internal/util'); const { validateNumber, validateOneOf } = require('internal/validators'); -function nop() {} - const kOnKeylog = Symbol('onkeylog'); const kRequestOptions = Symbol('requestOptions'); const kRequestAsyncResource = Symbol('requestAsyncResource'); @@ -133,7 +131,7 @@ function Agent(options) { // and could cause unhandled exception. if (!socket.writable) { - socket.on('error', nop).destroy(); + socket.destroy(); return; } @@ -161,7 +159,7 @@ function Agent(options) { // the freeSockets pool, but only if we're allowed to do so. const req = socket._httpMessage; if (!req || !req.shouldKeepAlive || !this.keepAlive) { - socket.on('error', nop).destroy(); + socket.destroy(); return; } @@ -175,7 +173,7 @@ function Agent(options) { count > this.maxSockets || freeLen >= this.maxFreeSockets || !this.keepSocketAlive(socket)) { - socket.on('error', nop).destroy(); + socket.destroy(); return; } From 44b35f3ab45a0a88bb0cba21615037851f768e86 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 3 Feb 2021 11:52:57 +0100 Subject: [PATCH 4/7] fixup --- test/parallel/test-http-client-abort3.js | 42 ++++++++++++++++++++---- 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/test/parallel/test-http-client-abort3.js b/test/parallel/test-http-client-abort3.js index f84384ef8d9a62..5e69802f8c0fd3 100644 --- a/test/parallel/test-http-client-abort3.js +++ b/test/parallel/test-http-client-abort3.js @@ -1,12 +1,42 @@ 'use strict'; const common = require('../common'); -const assert = require('assert'); const http = require('http'); +const net = require('net'); +const assert = require('assert'); + +{ + const req = http.get('http://[2604:1380:45f1:3f00::1]:4002'); + + req.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'EHOSTUNREACH'); + })); + req.abort(); +} + +function createConnection() { + const socket = new net.Socket(); + + process.nextTick(function() { + socket.destroy(new Error('Oops')); + }); + + return socket; +} + +{ + const req = http.get({ createConnection }); + + req.on('error', common.expectsError({ name: 'Error', message: 'Oops' })); + req.abort(); +} + +{ + class CustomAgent extends http.Agent {} + CustomAgent.prototype.createConnection = createConnection; -const req = http.get('http://[2604:1380:45f1:3f00::1]:4002'); + const req = http.get({ agent: new CustomAgent() }); -req.on('error', common.mustCall((err) => { - assert.strictEqual(err.code, 'EHOSTUNREACH'); -})); -req.abort(); + req.on('error', common.expectsError({ name: 'Error', message: 'Oops' })); + req.abort(); +} From 774665ffa45978cc658c8617ce359eecb926d404 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 3 Feb 2021 12:20:20 +0100 Subject: [PATCH 5/7] fixup --- test/parallel/test-http-client-abort3.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-http-client-abort3.js b/test/parallel/test-http-client-abort3.js index 5e69802f8c0fd3..28ed365285107b 100644 --- a/test/parallel/test-http-client-abort3.js +++ b/test/parallel/test-http-client-abort3.js @@ -9,7 +9,7 @@ const assert = require('assert'); const req = http.get('http://[2604:1380:45f1:3f00::1]:4002'); req.on('error', common.mustCall((err) => { - assert.strictEqual(err.code, 'EHOSTUNREACH'); + assert(err.code === 'ENETUNREACH' || err.code === 'EHOSTUNREACH'); })); req.abort(); } From d103ec1b033d9051f7cef91423b039b0a09b62f2 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 3 Feb 2021 12:52:17 +0100 Subject: [PATCH 6/7] Update test/parallel/test-http-client-abort3.js Co-authored-by: Luigi Pinca --- test/parallel/test-http-client-abort3.js | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/parallel/test-http-client-abort3.js b/test/parallel/test-http-client-abort3.js index 28ed365285107b..396383334b0969 100644 --- a/test/parallel/test-http-client-abort3.js +++ b/test/parallel/test-http-client-abort3.js @@ -5,15 +5,6 @@ const http = require('http'); const net = require('net'); const assert = require('assert'); -{ - const req = http.get('http://[2604:1380:45f1:3f00::1]:4002'); - - req.on('error', common.mustCall((err) => { - assert(err.code === 'ENETUNREACH' || err.code === 'EHOSTUNREACH'); - })); - req.abort(); -} - function createConnection() { const socket = new net.Socket(); From a5cb23a6c10aa689891fd5633582f2f9a0e3863a Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 3 Feb 2021 12:56:52 +0100 Subject: [PATCH 7/7] Update test/parallel/test-http-client-abort3.js Co-authored-by: Luigi Pinca --- test/parallel/test-http-client-abort3.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/parallel/test-http-client-abort3.js b/test/parallel/test-http-client-abort3.js index 396383334b0969..4fb7d0fdba8d6b 100644 --- a/test/parallel/test-http-client-abort3.js +++ b/test/parallel/test-http-client-abort3.js @@ -3,7 +3,6 @@ const common = require('../common'); const http = require('http'); const net = require('net'); -const assert = require('assert'); function createConnection() { const socket = new net.Socket();