Skip to content

Commit

Permalink
https: fix memory leak with https.request()
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
imyller authored and Myles Borins committed Nov 18, 2016
1 parent e97723b commit 12da258
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 0 deletions.
8 changes: 8 additions & 0 deletions lib/_http_agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,14 @@ Agent.prototype.addRequest = function(req, options) {
options = util._extend({}, options);
options = util._extend(options, this.options);

if (!options.servername) {
options.servername = options.host;
const hostHeader = req.getHeader('host');
if (hostHeader) {
options.servername = hostHeader.replace(/:.*$/, '');
}
}

var name = this.getName(options);
if (!this.sockets[name]) {
this.sockets[name] = [];
Expand Down
52 changes: 52 additions & 0 deletions test/parallel/test-https-agent-sockets-leak.js
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);
}));
}));
}));

0 comments on commit 12da258

Please sign in to comment.