-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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. Fixes: #6687
- Loading branch information
Showing
2 changed files
with
60 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
if (!common.hasCrypto) { | ||
common.skip('missing crypto'); | ||
return; | ||
} | ||
|
||
const fs = require('fs'); | ||
const https = require('https'); | ||
const assert = require('assert'); | ||
|
||
const options = { | ||
key: fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'), | ||
cert: fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem'), | ||
ca: fs.readFileSync(common.fixturesDir + '/keys/ca1-cert.pem') | ||
}; | ||
|
||
const server = https.Server(options, common.mustCall((req, res) => { | ||
res.writeHead(200); | ||
res.end('https\n'); | ||
})); | ||
|
||
const agent = new https.Agent({ | ||
keepAlive: false | ||
}); | ||
|
||
server.listen(0, common.mustCall(() => { | ||
https.get({ | ||
host: server.address().host, | ||
port: server.address().port, | ||
headers: {host: 'agent1'}, | ||
rejectUnauthorized: true, | ||
ca: options.ca, | ||
agent: agent | ||
}, common.mustCall((res) => { | ||
res.resume(); | ||
server.close(); | ||
|
||
// Only one entry should exist in agent.sockets pool | ||
// If there are more entries in agent.sockets, | ||
// removeSocket will not delete them resulting in a resource leak | ||
assert.strictEqual(Object.keys(agent.sockets).length, 1); | ||
|
||
res.req.on('close', common.mustCall(() => { | ||
// To verify that no leaks occur, check that no entries | ||
// exist in agent.sockets pool after both request and socket | ||
// has been closed. | ||
assert.strictEqual(Object.keys(agent.sockets).length, 0); | ||
})); | ||
})); | ||
})); |