fix: ssl socket leak when keepalive is used #5713

alexpenev-s · 2016-03-15T08:06:12Z

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)

Description of change

Please provide a description of the change here.

-closes: #5699

benjamingr · 2016-03-15T10:35:22Z

LGTM.

benjamingr · 2016-03-15T10:39:17Z

@saperal I see this is your first contribution (based on your GH history, feel free to correct me).

First of all thank you :) This is a non-trivial fix of a memory leak, definitely a good contribution.

What happens now is that collaborators are given 48 hours to discuss the fix and its implications. assuming no changes are requested it will be landed by a collaborator in 48 hours.

Note that in general we like descriptive commit messages (under 70 lines) and we add the pull request URL and reviewers into the commit message. This will be done by a collaborator for you so don't worry about it - just for future reference.

cjihrig · 2016-03-15T13:16:35Z

lib/_http_agent.js

@@ -115,6 +115,9 @@ Agent.prototype.addRequest = function(req, options) {
    };
  }

+  options = util._extend({}, options);


This can be done as a single call like options = Object.assign({}, options, this.options);

I think it was done this way in order to remain consistent with the rest of the file. If we're refactoring this to an Object.assign we might as well refactor the one in createSocket to use Object.assign.

Refactoring the rest of the file isn't really necessary, and it will add unnecessary noise, making it a lot harder to use tools like git blame.

So can we keep the same syntax throughout the file (_extends) and refactor it to a Object.assign at a future PR?

Object.assign is not available in v0.12 in case the change has to be merged there.

@benjamingr we usually just update things like that when the code needs to be updated for some other reason.

@saperal ok, feel free to leave this as is.

cjihrig · 2016-03-15T14:24:38Z

LGTM if the CI is happy

benjamingr · 2016-03-15T14:44:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1929/

jasnell · 2016-03-15T15:37:25Z

@saperal ... would you mind adding a bit more detail to the commit log message describing the issue that is fixed?

jasnell · 2016-03-15T15:37:40Z

LGTM with additional info added to the commit message

SSL sockets leak whenever keep alive is enabled, ca option is set in the global agent, and requests are sent without the ca property. In the following case at Agent.prototype.createSocket a socket will be created with a hashtag name that includes data from the global agents’ ca property. On subsequent requests at Agent.prototype.addRequest we do not find the free socket, because the hashtag name generated there does not take into account the global agents’ ca property, thus creating a new socket and leaving the first socket to timeout. -closes: #5699

alexpenev-s · 2016-03-15T16:05:21Z

Done more info added to the comments of the PR.

whitlockjc · 2016-03-15T16:15:13Z

LGTM

SSL sockets leak whenever keep alive is enabled, ca option is set in the global agent, and requests are sent without the ca property. In the following case at Agent.prototype.createSocket a socket will be created with a hashtag name that includes data from the global agents’ ca property. On subsequent requests at Agent.prototype.addRequest we do not find the free socket, because the hashtag name generated there does not take into account the global agents’ ca property, thus creating a new socket and leaving the first socket to timeout. closes: #5699 PR-URL: #5713 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

benjamingr · 2016-03-17T12:35:58Z

Thanks for the contribution - landed in 2e1ae3e

SSL sockets leak whenever keep alive is enabled, ca option is set in the global agent, and requests are sent without the ca property. In the following case at Agent.prototype.createSocket a socket will be created with a hashtag name that includes data from the global agents’ ca property. On subsequent requests at Agent.prototype.addRequest we do not find the free socket, because the hashtag name generated there does not take into account the global agents’ ca property, thus creating a new socket and leaving the first socket to timeout. closes: #5699 PR-URL: #5713 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable Changes * https: - Under certain conditions ssl sockets may have been causing a memory leak when keepalive is enabled. This is no longer the case. - (Alexander Penev) #5713 * lib: - The way that we were internally passing arguments was causing a potential leak. By copying the arguments into an array we can avoid this - (Nathan Woltman) #4361 * repl: - Previously if you were using the repl in strict mode the column number would be wrong in a stack trace. This is no longer an issue. - (Prince J Wesley) #5416 PR-URL: #5961

Notable Changes * https: - Under certain conditions ssl sockets may have been causing a memory leak when keepalive is enabled. This is no longer the case. - (Alexander Penev) #5713 * lib: - The way that we were internally passing arguments was causing a potential leak. By copying the arguments into an array we can avoid this - (Nathan Woltman) #4361 * npm: - Upgrade to v2.15.1. (Forrest L Norvell) * repl: - Previously if you were using the repl in strict mode the column number would be wrong in a stack trace. This is no longer an issue. - (Prince J Wesley) #5416 PR-URL: #5961

Notable Changes * https: - Under certain conditions ssl sockets may have been causing a memory leak when keepalive is enabled. This is no longer the case. - (Alexander Penev) #5713 * lib: - The way that we were internally passing arguments was causing a potential leak. By copying the arguments into an array we can avoid this - (Nathan Woltman) #4361 * npm: - Upgrade to v2.15.1. Fixes a security flaw in the use of authentication tokens in HTTP requests that would allow an attacker to set up a server that could collect tokens from users of the command-line interface. Authentication tokens have previously been sent with every request made by the CLI for logged-in users, regardless of the destination of the request. This update fixes this by only including those tokens for requests made against the registry or registries used for the current install. (Forrest L Norvell) * repl: - Previously if you were using the repl in strict mode the column number would be wrong in a stack trace. This is no longer an issue. - (Prince J Wesley) #5416 PR-URL: #5961

closes #32 nodejs/node@2e1ae3e nodejs/node#5713

alexpenev-s mentioned this pull request Mar 15, 2016

SSL sockets may leak when keepalive is enabled #5699

Closed

ChALkeR added https Issues or PRs related to the https subsystem. memory Issues and PRs related to the memory management or memory footprint. labels Mar 15, 2016

cjihrig reviewed Mar 15, 2016
View reviewed changes

jasnell added the lts-watch-v4.x label Mar 15, 2016

benjamingr closed this Mar 17, 2016

Fishrock123 mentioned this pull request Mar 22, 2016

v5.9.1 Release Proposal #5831

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 30, 2016

This was referenced Mar 30, 2016

v4.4.2 proposal #5961

Merged

V4.4.1 proposal #5835

Merged

Zarel mentioned this pull request Apr 1, 2016

Anomalous RAM use by TLSSocket #5469

Closed

fengmk2 pushed a commit to node-modules/agentkeepalive that referenced this pull request Apr 5, 2016

https: fix ssl socket leak when keepalive is used

88d1e1e

closes #32 nodejs/node@2e1ae3e nodejs/node#5713

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ssl socket leak when keepalive is used #5713

fix: ssl socket leak when keepalive is used #5713

alexpenev-s commented Mar 15, 2016

benjamingr commented Mar 15, 2016

benjamingr commented Mar 15, 2016

cjihrig Mar 15, 2016

benjamingr Mar 15, 2016

cjihrig Mar 15, 2016

benjamingr Mar 15, 2016

alexpenev-s Mar 15, 2016

cjihrig Mar 15, 2016

cjihrig commented Mar 15, 2016

benjamingr commented Mar 15, 2016

jasnell commented Mar 15, 2016

jasnell commented Mar 15, 2016

alexpenev-s commented Mar 15, 2016

whitlockjc commented Mar 15, 2016

benjamingr commented Mar 17, 2016

fix: ssl socket leak when keepalive is used #5713

fix: ssl socket leak when keepalive is used #5713

Conversation

alexpenev-s commented Mar 15, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

benjamingr commented Mar 15, 2016

benjamingr commented Mar 15, 2016

cjihrig Mar 15, 2016

Choose a reason for hiding this comment

benjamingr Mar 15, 2016

Choose a reason for hiding this comment

cjihrig Mar 15, 2016

Choose a reason for hiding this comment

benjamingr Mar 15, 2016

Choose a reason for hiding this comment

alexpenev-s Mar 15, 2016

Choose a reason for hiding this comment

cjihrig Mar 15, 2016

Choose a reason for hiding this comment

cjihrig commented Mar 15, 2016

benjamingr commented Mar 15, 2016

jasnell commented Mar 15, 2016

jasnell commented Mar 15, 2016

alexpenev-s commented Mar 15, 2016

whitlockjc commented Mar 15, 2016

benjamingr commented Mar 17, 2016