tls_wrap: fix BIO leak on SSL error #1244

indutny · 2015-03-24T03:08:13Z

Fix: #1075

R=@bnoordhuis

Fix: nodejs#1075

indutny · 2015-03-24T03:08:16Z

cc @iojs/crypto

indutny · 2015-03-24T04:20:07Z

Cecil,

May ask you to gist a test case that your are using?

Thank you!

On Monday, March 23, 2015, Cecil Worsley notifications@github.com wrote:

I found a pretty simple way to reproduce this. I created an HTTPS hello
world server and another process that produces hundreds of HTTPS get
requests/sec. In io.js I found the memory leaking exponentially but not in
node 0.10.33. Perhaps this could make it easier to test this issue? I'm not
a C++ guy but seems like a simple way to test the hypotheses?

—
Reply to this email directly or view it on GitHub
#1244 (comment).

cworsley4 · 2015-03-24T04:26:57Z

Shoot, I accidentally deleted my comment. But sure, I can do that. I have been running the test for a few minutes and on node 0.10.33 memory climbs and then flattens out. io.js consumes multiple gigs after running for the same amount of time. I'll post the gist in a few.

cworsley4 · 2015-03-24T04:30:55Z

Example gist: https://gist.github.com/cworsley4/458aa035f97672794e9f
You'll have to generate your own SSL Cert but this should pretty clearly.

indutny · 2015-03-24T04:41:16Z

@cworsley4 running it locally now, will report back here. @bnoordhuis do not let this stop you, though :) This commit is surely fixing some leak.

indutny · 2015-03-24T04:42:37Z

@cworsley4 just to make sure, what io.js version are you using?

cworsley4 · 2015-03-24T12:25:35Z

I just duplicated the issue in 1.4.2, 1.5.1, and 1.6.2.

indutny · 2015-03-24T19:40:47Z

Interesting, I see some leaks in a client code... Will check it in a bit.

cworsley4 · 2015-03-24T22:30:26Z

@indutny Leaks in the code I provided?

indutny · 2015-03-24T22:34:01Z

Not sure about containment yet, but I am able to confirm it with your test
case. Thank you!

On Tuesday, March 24, 2015, Cecil Worsley notifications@github.com wrote:

@indutny https://github.com/indutny Leaks in the code I provided?

—
Reply to this email directly or view it on GitHub
#1244 (comment).

cworsley4 · 2015-03-24T22:39:41Z

I'm glad to hear it! Of course let us know what you find, or if I can provide anything else to the effort.

indutny · 2015-03-25T03:22:54Z

Update PR with a fix :) @cworsley4 thank you so much! You helped a lot.

@jasisk: this should fix your leak, guys ;)

cc @iojs/crypto

indutny · 2015-03-25T03:23:14Z

@jasisk: at least I have lots of hope for this

jasisk · 2015-03-25T03:24:22Z

Haha. 👍 Deploying.

Always call `Done` on the WriteWrap, and ensure that `EncOut` will consume all data in clear_in_ and invoke queued callbacks. Fix: nodejs#1075

cworsley4 · 2015-03-25T15:02:48Z

Do you guys think this PR will make it into #1252?

bnoordhuis · 2015-03-25T15:54:00Z

src/tls_wrap.cc

@@ -335,6 +339,9 @@ void TLSWrap::EncOutCb(WriteWrap* req_wrap, int status) {
  // Commit
  NodeBIO::FromBIO(wrap->enc_out_)->Read(nullptr, wrap->write_size_);

+  // Ensure that the progress will be maed and `InvokeQueued` will be called


s/maed/made/ and can you end the sentence with a dot?

Fixed, thanks.

indutny · 2015-03-25T20:11:41Z

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/373/

Thanks for the comments!

jasisk · 2015-03-25T20:36:55Z

Redeploying. 😀

indutny · 2015-03-25T21:56:49Z

The CI: appears to be blue.

bnoordhuis · 2015-03-25T22:44:47Z

LGTM

Fix: #1075 PR-URL: #1244 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Always call `Done` on the WriteWrap, and ensure that `EncOut` will consume all data in clear_in_ and invoke queued callbacks. Fix: #1075 PR-URL: #1244 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

indutny · 2015-03-25T22:51:36Z

Landed in e74b5d2, 2ccc8f3. Thank you!

tls_wrap: fix BIO leak on SSL error

16a13c7

Fix: nodejs#1075

indutny mentioned this pull request Mar 24, 2015

Possible memory leak in TLS(Wrap?) #1075

Closed

mscdex added tls Issues and PRs related to the tls subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Mar 24, 2015

tls_wrap: fix this incredibly stupid leak

3327932

Always call `Done` on the WriteWrap, and ensure that `EncOut` will consume all data in clear_in_ and invoke queued callbacks. Fix: nodejs#1075

indutny force-pushed the fix/gh-1075-3 branch from acffdd2 to 3327932 Compare March 25, 2015 03:25

bnoordhuis reviewed Mar 25, 2015
View reviewed changes

indutny added 3 commits March 25, 2015 15:09

fixup

be2d212

fixup

301cf94

invoke queued

ec2a2ba

indutny added a commit that referenced this pull request Mar 25, 2015

tls_wrap: fix BIO leak on SSL error

e74b5d2

Fix: #1075 PR-URL: #1244 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

indutny closed this Mar 25, 2015

indutny deleted the fix/gh-1075-3 branch March 25, 2015 22:51

Fishrock123 mentioned this pull request Mar 26, 2015

Release proposal: v1.6.3 #1252

Closed

Fishrock123 mentioned this pull request Apr 9, 2015

test: windows ci timeouts related to child_process #1383

Closed

brendanashworth mentioned this pull request Oct 13, 2015

Memory leak when hitting a site over https with invalid certificates. nodejs/node-v0.x-archive#25616

Closed

MarcelRaschke mentioned this pull request Nov 5, 2022

[Snyk] Security upgrade webpack-cli from 3.3.12 to 4.0.0 MarcelRaschke/node#18

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls_wrap: fix BIO leak on SSL error #1244

tls_wrap: fix BIO leak on SSL error #1244

indutny commented Mar 24, 2015

indutny commented Mar 24, 2015

indutny commented Mar 24, 2015

cworsley4 commented Mar 24, 2015

cworsley4 commented Mar 24, 2015

indutny commented Mar 24, 2015

indutny commented Mar 24, 2015

cworsley4 commented Mar 24, 2015

indutny commented Mar 24, 2015

cworsley4 commented Mar 24, 2015

indutny commented Mar 24, 2015

cworsley4 commented Mar 24, 2015

indutny commented Mar 25, 2015

indutny commented Mar 25, 2015

jasisk commented Mar 25, 2015

cworsley4 commented Mar 25, 2015

bnoordhuis Mar 25, 2015

indutny Mar 25, 2015

indutny commented Mar 25, 2015

jasisk commented Mar 25, 2015

indutny commented Mar 25, 2015

bnoordhuis commented Mar 25, 2015

indutny commented Mar 25, 2015

tls_wrap: fix BIO leak on SSL error #1244

tls_wrap: fix BIO leak on SSL error #1244

Conversation

indutny commented Mar 24, 2015

indutny commented Mar 24, 2015

indutny commented Mar 24, 2015

cworsley4 commented Mar 24, 2015

cworsley4 commented Mar 24, 2015

indutny commented Mar 24, 2015

indutny commented Mar 24, 2015

cworsley4 commented Mar 24, 2015

indutny commented Mar 24, 2015

cworsley4 commented Mar 24, 2015

indutny commented Mar 24, 2015

cworsley4 commented Mar 24, 2015

indutny commented Mar 25, 2015

indutny commented Mar 25, 2015

jasisk commented Mar 25, 2015

cworsley4 commented Mar 25, 2015

bnoordhuis Mar 25, 2015

Choose a reason for hiding this comment

indutny Mar 25, 2015

Choose a reason for hiding this comment

indutny commented Mar 25, 2015

jasisk commented Mar 25, 2015

indutny commented Mar 25, 2015

bnoordhuis commented Mar 25, 2015

indutny commented Mar 25, 2015