From 7a594494784a4c1c591ea4d855b5b27a63c3855b Mon Sep 17 00:00:00 2001
From: Ilkka Myller <ilkka.myller@nodefield.com>
Date: Sun, 18 Sep 2016 14:34:48 +0300
Subject: [PATCH] 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: https://github.com/nodejs/node/pull/8647
Fixes: https://github.com/nodejs/node/issues/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>
---
 lib/_http_agent.js                            |  8 +++
 .../parallel/test-https-agent-sockets-leak.js | 52 +++++++++++++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 test/parallel/test-https-agent-sockets-leak.js

diff --git a/lib/_http_agent.js b/lib/_http_agent.js
index bac10c4dab..6ea801c3ed 100644
--- a/lib/_http_agent.js
+++ b/lib/_http_agent.js
@@ -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] = [];
diff --git a/test/parallel/test-https-agent-sockets-leak.js b/test/parallel/test-https-agent-sockets-leak.js
new file mode 100644
index 0000000000..27b37dc4fe
--- /dev/null
+++ b/test/parallel/test-https-agent-sockets-leak.js
@@ -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);
+    }));
+  }));
+}));