Skip to content

tls: fix leak of WriteWrap+TLSWrap combination #9586

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

Closed
indutny wants to merge 3 commits into from
Closed

tls: fix leak of WriteWrap+TLSWrap combination #9586

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d603f9
tls: fix leak of WriteWrap+TLSWrap combination
indutny Nov 12, 2016
e6869b8
fixes
indutny Nov 14, 2016
717f9bf
fix
indutny Nov 15, 2016
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
Prev Previous commit
Next Next commit
fixes
  • Loading branch information
@indutny
indutny committed Nov 14, 2016
commit e6869b87035949da59bbd2d0a9df44cc8f33f39f
2 changes: 1 addition & 1 deletion lib/_tls_wrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ tls_wrap.TLSWrap.prototype.close = function close(cb) {
ssl._secureContext.context = null;
}
}
if (cb)
if (typeof cb === 'function')
cb();
};
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Nov 15, 2016

Choose a reason for hiding this comment

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

I'm curious, what would happen when ssl.destroySSL() gets called synchronously, i.e., outside the callback?

Copy link
Copy Markdown
Member Author

@indutny indutny Nov 15, 2016

Choose a reason for hiding this comment

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

Crash, because of StreamBase callbacks.

Copy link
Copy Markdown
Member Author

@indutny indutny Nov 15, 2016

Choose a reason for hiding this comment

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

The reason is actually way simpler than this, I'll simplify the code.

Thank you for bringing my attention to this.

Copy link
Copy Markdown
Member Author

@indutny indutny Nov 15, 2016

Choose a reason for hiding this comment

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

It crashes if done in the same tick because it may be invoked within the OpenSSL's call stack. If invoked with setImmediate - it won't crash.

Copy link
Copy Markdown
Member Author

@indutny indutny Nov 15, 2016

Choose a reason for hiding this comment

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

Oh... just run the test suite after using setImmediate() and it confirms my initial assumption. Sorry, not going to fix it.

The write callbacks are invoked with UV_ECANCELED and thus things are getting back to tls_wrap.cc at places where it doesn't expect ssl_ to be nullptr. Therefore it is better to free it up after closing the underlying libuv handle.

Copy link
Copy Markdown
Member Author

@indutny indutny Nov 15, 2016

Choose a reason for hiding this comment

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

To be honest, I'm so unhappy with what we have right now (I know that I wrote most of it). I wish we would move to uv_ssl_t.


Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-tls-writewrap-leak.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const server = net.createServer(common.mustCall((c) => {
});

c.write('hello', common.mustCall((err) => {
assert.equal(err.code, 'ECANCELED');
assert.deepEqual(err.code, 'ECANCELED');
server.close();
}));
}));