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.

Sign up for GitHub

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

Jump to bottom

tls_wrap: fix BIO leak on SSL error #1244

Closed
wants to merge 5 commits into from
Closed

tls_wrap: fix BIO leak on SSL error #1244

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
16a13c7
tls_wrap: fix BIO leak on SSL error
indutny Mar 24, 2015
3327932
tls_wrap: fix this incredibly stupid leak
indutny Mar 25, 2015
be2d212
fixup
indutny Mar 25, 2015
301cf94
fixup
indutny Mar 25, 2015
ec2a2ba
invoke queued
indutny Mar 25, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/tls_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,10 @@ TLSWrap::~TLSWrap() {
MakePending();

// And destroy
while (WriteItem* wi = pending_write_items_.PopFront())
while (WriteItem* wi = pending_write_items_.PopFront()) {
wi->w_->Done(UV_ECANCELED);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me why this is needed. Can you add a comment explaining it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is there a reason why this can't be replaced with a call to InvokeQueued(UV_ECANCELED)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because WriteWraps are not destroyed in stream_base.cc (and previously in stream_wrap.cc) unless the callback is invoked.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add InvokeQueued here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for a suggestion.

delete wi;
}

ClearError();
}
Expand Down Expand Up @@ -310,10 +312,12 @@ void TLSWrap::EncOut() {
write_req->Dispatched();

// Ignore errors, this should be already handled in js
if (err)
if (err) {
write_req->Dispose();
else
InvokeQueued(err);
} else {
NODE_COUNT_NET_BYTES_SENT(write_size_);
}
}


Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks.

wrap->ClearIn();

// Try writing more data
wrap->write_size_ = 0;
wrap->EncOut();
Expand Down Expand Up @@ -375,6 +382,7 @@ Local<Value> TLSWrap::GetSSLError(int status, int* err, const char** msg) {
*msg = buf;
}
static_cast<void>(BIO_reset(bio));
BIO_free_all(bio);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean that all the BIO_new + BIO_reset calls in src/node_crypto.cc are leaking memory as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at deps/openssl/openssl/crypto/bio/bss_mem.c I'm guessing the answer is 'yes'. Instead of blindly adding BIO_free_all calls everywhere, I suggest introducing a small RAII class that fixes the issue once and for all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt we have any other places like this.


return scope.Escape(exception);
}
Expand Down