tls_wrap: fix BIO leak on SSL error #1244
Conversation
|
cc @iojs/crypto |
mscdex
added
tls
|
Cecil, May ask you to gist a test case that your are using? Thank you! On Monday, March 23, 2015, Cecil Worsley [email protected] wrote:
|
|
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. |
|
Example gist: https://gist.github.com/cworsley4/458aa035f97672794e9f |
|
@cworsley4 running it locally now, will report back here. @bnoordhuis do not let this stop you, though :) This commit is surely fixing some leak. |
|
@cworsley4 just to make sure, what io.js version are you using? |
|
I just duplicated the issue in 1.4.2, 1.5.1, and 1.6.2. |
|
Interesting, I see some leaks in a client code... Will check it in a bit. |
|
@indutny Leaks in the code I provided? |
|
Not sure about containment yet, but I am able to confirm it with your test On Tuesday, March 24, 2015, Cecil Worsley [email protected] wrote:
|
|
I'm glad to hear it! Of course let us know what you find, or if I can provide anything else to the effort. |
|
Update PR with a fix :) @cworsley4 thank you so much! You helped a lot. @jasisk: this should fix your leak, guys ;) cc @iojs/crypto |
|
@jasisk: at least I have lots of hope for this |
|
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
|
Do you guys think this PR will make it into #1252? |
| @@ -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 | |||
There was a problem hiding this comment.
s/maed/made/ and can you end the sentence with a dot?
|
CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/373/ Thanks for the comments! |
|
Redeploying. 😀 |
|
The CI: appears to be blue. |
|
LGTM |
Fix: #1075 PR-URL: #1244 Reviewed-By: Ben Noordhuis <[email protected]>
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 <[email protected]>
Fix: #1075
R=@bnoordhuis