tls: do not leak WriteWrap objects #1090
Conversation
Kill WriteWrap instances that are allocated in `tls_wrap.cc` internally. Fix: nodejs#1075
|
Odd, looks like there were a lot of git fetch failures.. |
|
Yeah :( |
|
Weird. Can someone try again and double check that the branch actually exists? |
/tmp $ git clone --depth=1 git://github.com/indutny/io.js -b fix/paypal-leak-for-sure
Cloning into 'io.js'...
remote: Counting objects: 12288, done.
remote: Compressing objects: 100% (10161/10161), done.
remote: Total 12288 (delta 3046), reused 7068 (delta 1593), pack-reused 0
Receiving objects: 100% (12288/12288), 30.36 MiB | 2.65 MiB/s, done.
Resolving deltas: 100% (3046/3046), done.
Checking connectivity... done.
/tmp $ |
|
It probably gets confused by the slash in the branch name. |
|
@bnoordhuis I always do them in branches, seems to be very unlikely |
|
I did change the way it grabs branches recently so the slash thing is possible. Perhaps try without and see. |
|
I take that back .. I only changed it on the nightly and release jobs, the pr-multi one just checks out |
|
cleaned out the existing workspaces on most of the build machines and it seems happy now https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/260/ |
|
@rvagg : seems to be mostly green, except aborted ubuntu1410 build. |
|
LGTY @bnoordhuis ? |
|
14.10 slave back online and running |
| @@ -314,6 +315,8 @@ void TLSWrap::EncOut() { | |||
|
|
|||
| void TLSWrap::EncOutCb(WriteWrap* req_wrap, int status) { | |||
| TLSWrap* wrap = req_wrap->wrap()->Cast<TLSWrap>(); | |||
| req_wrap->~WriteWrap(); | |||
| delete[] reinterpret_cast<char*>(req_wrap); | |||
There was a problem hiding this comment.
I would suggest moving the placement new and delete logic into New and Dispose methods, like I did just now with FSReqWrap in src/node_file.cc. Having the same logic repeated five times throughout the file is not good software engineering.
| @@ -308,11 +303,9 @@ int StreamBase::WriteString(const FunctionCallbackInfo<Value>& args) { | |||
| CHECK_EQ(count, 1); | |||
| } | |||
|
|
|||
| storage = new char[sizeof(WriteWrap) + storage_size + 15]; | |||
There was a problem hiding this comment.
@bnoordhuis do you think this is still relevant?
There was a problem hiding this comment.
If not - we may want to skip this 15 byte thing above too.
There was a problem hiding this comment.
I think it is no relevant anymore. PTAL at follow-up commit.
|
@bnoordhuis and one more patch for your fun :) |
|
LGTM, the only thing I would perhaps change is the use of int instead of size_t. |
|
Thanks, will do. One more build, just to be sure: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/261/ |
Encapsulate allocation/disposal of `WriteWrap` instances into the `WriteWrap` class itself. PR-URL: #1090 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
Thank you, everyone! |
Kill WriteWrap instances that are allocated in
tls_wrap.ccinternally.Fix: #1075
cc @iojs/crypto @bnoordhuis
This should fix it once and forever :) (hopefully)