Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSL sockets may leak when keepalive is enabled #5699

Closed
alexpenev-s opened this issue Mar 14, 2016 · 3 comments
Closed

SSL sockets may leak when keepalive is enabled #5699

alexpenev-s opened this issue Mar 14, 2016 · 3 comments
Labels
confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. memory Issues and PRs related to the memory management or memory footprint. net Issues and PRs related to the net subsystem.

Comments

@alexpenev-s
Copy link
Contributor

issue is in: lib/_http_agent.js
The issue is reproducible whenever the ca property is set globally in the agent and an https request is made with https.request(ops) where ops does not have the property ca

_http_agent.js @ Agent.prototype.createSocket
the options are extended with the global options object which contains ca. In getName ca is used in the name generation of the hash.

however _http_agent.js @ Agent.prototype.addRequest
The options (passed from the call https.request(ops) which don't have the property ca) are used to get a hashtag which is now different. The socket leaks and is closed when the timeout hits. Meanwhile a new socket is created for the request.

@Fishrock123 Fishrock123 added http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. net Issues and PRs related to the net subsystem. labels Mar 14, 2016
@benjamingr
Copy link
Member

It's worth mentioning that this happens because getName is overridden in prototypical inheritance in `https://github.com/nodejs/node/blob/master/lib/https.js#L114 - _http_agent is not aware of the ca property.

@benjamingr
Copy link
Member

Nice spotting and nice fix. Worth mentioning that the only place that addRequest uses options except in getName is when it calls createSocket itself - so this is a backwards compatibly.

Would you be willing to make a pull request of your branch against node @saperal ?

@alexpenev-s
Copy link
Contributor Author

Done #5713

@ChALkeR ChALkeR added the memory Issues and PRs related to the memory management or memory footprint. label Mar 15, 2016
@benjamingr benjamingr added the confirmed-bug Issues with confirmed bugs. label Mar 15, 2016
Fishrock123 pushed a commit that referenced this issue Mar 22, 2016
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>
MylesBorins pushed a commit that referenced this issue Mar 30, 2016
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>
MylesBorins pushed a commit that referenced this issue Mar 30, 2016
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. memory Issues and PRs related to the memory management or memory footprint. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants