Commit 12da258
https: fix memory leak with https.request()
If calling `https.request()` with `options.headers.host` defined
and `options.servername` undefined, `https.Agent.createSocket` mutates
connection `options` after `https.Agent.addRequest` has created empty
socket pool array with mismatching connection name. This results in two
socket pool arrays being created and only the last one gets eventually
deleted by `removeSocket` - causing a memory leak.
This commit fixes the leak by making sure that `addRequest` does the
same modifications to `options` object as the `createSocket`.
`createSocket` is intentionally left unmodified to prevent userland
regressions.
Test case included.
PR-URL: #8647
Fixes: #6687
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jackson Tian <shvyo1987@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>1 parent e97723b commit 12da258
2 files changed
Lines changed: 60 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -123,6 +123,14 @@ Agent.prototype.addRequest = function(req, options) { | |||
| 123 | 123 | options = util._extend({}, options); | |
| 124 | 124 | options = util._extend(options, this.options); | |
| 125 | 125 | ||
| 126 | + if (!options.servername) { | ||
| 127 | + options.servername = options.host; | ||
| 128 | + const hostHeader = req.getHeader('host'); | ||
| 129 | + if (hostHeader) { | ||
| 130 | + options.servername = hostHeader.replace(/:.*$/, ''); | ||
| 131 | + } | ||
| 132 | + } | ||
| 133 | + | ||
| 126 | 134 | var name = this.getName(options); | |
| 127 | 135 | if (!this.sockets[name]) { | |
| 128 | 136 | this.sockets[name] = []; | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -0,0 +1,52 @@ | |||
| 1 | + 'use strict'; | ||
| 2 | + | ||
| 3 | + const common = require('../common'); | ||
| 4 | + if (!common.hasCrypto) { | ||
| 5 | + common.skip('missing crypto'); | ||
| 6 | + return; | ||
| 7 | + } | ||
| 8 | + | ||
| 9 | + const fs = require('fs'); | ||
| 10 | + const https = require('https'); | ||
| 11 | + const assert = require('assert'); | ||
| 12 | + | ||
| 13 | + const options = { | ||
| 14 | + key: fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'), | ||
| 15 | + cert: fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem'), | ||
| 16 | + ca: fs.readFileSync(common.fixturesDir + '/keys/ca1-cert.pem') | ||
| 17 | + }; | ||
| 18 | + | ||
| 19 | + const server = https.Server(options, common.mustCall((req, res) => { | ||
| 20 | + res.writeHead(200); | ||
| 21 | + res.end('https\n'); | ||
| 22 | + })); | ||
| 23 | + | ||
| 24 | + const agent = new https.Agent({ | ||
| 25 | + keepAlive: false | ||
| 26 | + }); | ||
| 27 | + | ||
| 28 | + server.listen(0, common.mustCall(() => { | ||
| 29 | + https.get({ | ||
| 30 | + host: server.address().host, | ||
| 31 | + port: server.address().port, | ||
| 32 | + headers: {host: 'agent1'}, | ||
| 33 | + rejectUnauthorized: true, | ||
| 34 | + ca: options.ca, | ||
| 35 | + agent: agent | ||
| 36 | + }, common.mustCall((res) => { | ||
| 37 | + res.resume(); | ||
| 38 | + server.close(); | ||
| 39 | + | ||
| 40 | + // Only one entry should exist in agent.sockets pool | ||
| 41 | + // If there are more entries in agent.sockets, | ||
| 42 | + // removeSocket will not delete them resulting in a resource leak | ||
| 43 | + assert.strictEqual(Object.keys(agent.sockets).length, 1); | ||
| 44 | + | ||
| 45 | + res.req.on('close', common.mustCall(() => { | ||
| 46 | + // To verify that no leaks occur, check that no entries | ||
| 47 | + // exist in agent.sockets pool after both request and socket | ||
| 48 | + // has been closed. | ||
| 49 | + assert.strictEqual(Object.keys(agent.sockets).length, 0); | ||
| 50 | + })); | ||
| 51 | + })); | ||
| 52 | + })); | ||
0 commit comments